Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove check_includes = false for brave/browser/profiles #24737

Merged
merged 10 commits into from
Sep 24, 2024
Merged

remove check_includes = false for brave/browser/profiles #24737

merged 10 commits into from
Sep 24, 2024

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented Jul 18, 2024 •
edited
Loading

Resolves brave/brave-browser#10648

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@bridiver bridiver requested review from iefremov and a team as code owners July 18, 2024 17:16
sangwoo108
sangwoo108 approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

browser/sources.gni Outdated
@@ -493,6 +493,7 @@ brave_chrome_browser_sources += brave_browser_themes_sources
brave_chrome_browser_sources += brave_browser_tor_sources
brave_chrome_browser_sources += brave_browser_url_sanitizer_sources
brave_chrome_browser_sources += brave_browser_web_discovery_sources
brave_chrome_browser_sources += brave_browser_profiles_sources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but could it be better to add sources and deps to //chrome/browser/profiles target?

SergeyZhukovsky
SergeyZhukovsky reviewed Jul 22, 2024
View reviewed changes
browser/profiles/brave_renderer_updater.cc Outdated
@@ -80,14 +77,12 @@ BraveRendererUpdater::BraveRendererUpdater(
base::Unretained(this)));
#endif

#if BUILDFLAG(ENABLE_WIDEVINE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's been added to remove a crash on linux arm64 409b7a0 as we didn't support widevine on that platform. From my understanding we support it currently right? In that case we can remove it in that place as well 409b7a0#diff-ee85b0dca2867e754c68f6e4b21fa42452a7cf72ff50a9a7e15380e93d43608aR198

@bridiver bridiver requested review from a team as code owners September 21, 2024 23:16
goodov
goodov reviewed Sep 23, 2024
View reviewed changes
browser/brave_app_controller_mac.mm Show resolved Hide resolved
browser/brave_app_controller_mac_browsertest.mm Show resolved Hide resolved
browser/profiles/profile_util.cc
@@ -81,31 +34,4 @@ void SetDefaultThirdPartyCookieBlockValue(Profile* profile) {
content_settings::CookieControlsMode::kBlockThirdParty)));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should either move these two left functions out of this file and get rid of it entirely or rename it into profile_migration_helpers.h/cc and keep all migration code here explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the only two that had tests for them so they couldn't be easily moved like the others, but I'll rename the file for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I'm going to leave it for now because I'm not sure migration_helper makes sense here and these methods should definitely move somewhere else in a follow-up

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brave/brave-browser#41231

@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24737

Description

This PR makes significant changes to the Brave browser codebase, primarily focusing on restructuring and refactoring profile-related functionality. The changes aim to improve code organization, reduce dependencies, and enhance the modularity of the codebase.

Changes

Changes

  1. browser/profiles/BUILD.gn:

    • Removed the "profiles" source set and its dependencies.
    • Kept the "util" source set with modified dependencies.

  2. browser/profiles/brave_profile_manager.cc:

    • Moved RecordInitialP3AValues and MigrateHttpsUpgradeSettings functions from profile_util.cc into this file.
    • Updated include statements and removed unused includes.

  3. browser/profiles/profile_util.cc and profile_util.h:

    • Removed several functions including IsTorDisabledForProfile, RecordInitialP3AValues, and MigrateHttpsUpgradeSettings.
    • Kept SetDefaultSearchVersion and SetDefaultThirdPartyCookieBlockValue functions.

  4. browser/profiles/sources.gni:

    • New file added to define brave_browser_profiles_sources.

  5. browser/sources.gni:

    • Updated to include the new brave_browser_profiles_sources.

  6. Various other files:

    • Updated include statements and dependencies to reflect the changes in the profiles directory.
    • Removed references to deleted functions and updated function calls where necessary.

  7. test/BUILD.gn:

    • Updated dependencies for various test targets.

Possible Issues

  1. The removal of IsTorDisabledForProfile function might affect functionality related to Tor browsing if not properly replaced or handled elsewhere.
  2. The relocation of RecordInitialP3AValues and MigrateHttpsUpgradeSettings functions may impact their accessibility or usage in other parts of the codebase.

Security Hotspots

No significant security hotspots were identified in this change. The modifications appear to be primarily structural and do not introduce new security vulnerabilities.

@bridiver
Copy link
Collaborator Author

Linux test failing intermittently unrelated to this PR

@bridiver bridiver merged commit 3b60bf0 into master Sep 24, 2024
17 checks passed
@bridiver bridiver deleted the profiles-check-includes branch September 24, 2024 15:23
@github-actions github-actions bot added this to the 1.72.x - Nightly milestone Sep 24, 2024
@bridiver bridiver mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky left review comments

@goodov goodov goodov left review comments

@darkdh darkdh darkdh approved these changes

@sangwoo108 sangwoo108 sangwoo108 approved these changes

@iefremov iefremov iefremov approved these changes

@cdesouza-chromium cdesouza-chromium Awaiting requested review from cdesouza-chromium

Assignees

@bridiver bridiver

@thypon thypon

Labels
CI/run-network-audit Run network-audit needs-security-review puLL-Merge
Projects
None yet
Milestone
1.73.x - Release
Development

Successfully merging this pull request may close these issues.

[Desktop] Fix deps in //brave/browser/profiles/
7 participants
@bridiver @iefremov @sangwoo108 @goodov @darkdh @SergeyZhukovsky @thypon