Remove usage of BRAVE_CHROMIUM_BUILD and brave_chromium_build #7969

mariospr · 2021-02-16T12:19:29Z

It will assumed from now on that all of Brave's own code plus patches
to upstream Chromium will always be built and run as part of a Brave
build, so we can drop these guards and reduce the patching surface.

Note that github.com/brave/brave-browser/wiki/Patching-Chromium should
also be updated to reflect the change implemented in this patch.

Resolves brave/brave-browser#12740

Submitter Checklist:

There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
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, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)
Requested a security/privacy review as needed

Reviewer Checklist:

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:

N/A

iefremov · 2021-02-17T08:59:37Z

patches/components-update_client-updater_state_win.cc.patch

     L"Update{8A69D345-D564-463C-AFF1-A69D9E530F96}";
 +#endif

 // Don't allow update periods longer than six weeks (Chrome release cadence).
 const int kCheckPeriodOverrideMinutesMax = 60 * 24 * 7 * 6;

 // Google Update registry settings.
-+#if defined(BRAVE_CHROMIUM_BUILD)
 +const wchar_t kRegPathGoogleUpdate[] = L"Software\\BraveSoftware\\Update";


for future work: all of these potentitally can be redefined via chromium_src?

Yes, I had the same thought exactly, but for this patch I think it's probably better to keep it like this so that it's more clear what the change is about?

@iefremov I've been looking into this today in the context of other similar situations where I could actually remove the definitions from the .patch file and put them into an override (i.e. #8063, #8061 and #8062), but in this particular case I'm not sure it's a good idea.

My reasoning is that in this case using a chromium_src override to define Brave-specific values for these variables won't work as expected, since we'd need first to redefine the ones from upstream into something like <variable_name>_ChromiumImpl or <variable_name>_Unused (depending on whether we will actually use them -e.g. for the !defined(BRAVE_CHROMIUM_BUILD) case- or not), but doing that will also change the places where such variables are already being referenced inside this updater_state_win.cc file.

If this file didn't contain any logic referencing those variables and only contained definitions (as it's the case for the PRs linked above), I'd say this approach would work. But considering that's not the case and they are indeed being referenced, I'm not sure how to do this in a way that is actually better / less bad than using a .patch file.

WDYT? Do you have any suggestion?

no suggestions, let's leave them alone :)

iefremov · 2021-02-17T09:12:15Z

patches/chrome-common-channel_info_win.cc.patch

@@ -1,13 +1,13 @@
 diff --git a/chrome/common/channel_info_win.cc b/chrome/common/channel_info_win.cc
-index 8a9e2d3335b6cf945864a88d1644e1935234990b..cae0e9f8f7e1d9dc3d2489111cbb8f5d75330928 100644
+index 8a9e2d3335b6cf945864a88d1644e1935234990b..f442eabe6ecad373f11eb84eb91fcd2ae5dfc538 100644
 --- a/chrome/common/channel_info_win.cc
 +++ b/chrome/common/channel_info_win.cc
 @@ -13,7 +13,7 @@
 namespace chrome {

 std::string GetChannelName() {


candidate for chromium_src?

Indeed, will work on this (and other cases) in follow-up PRs if that's ok

Tracked in #7996

iefremov · 2021-02-17T09:15:18Z

patches/chrome-browser-search_engines-template_url_service_factory.cc.patch


 content::BrowserContext* TemplateURLServiceFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
-+#if defined(BRAVE_CHROMIUM_BUILD)


chromium_src

Ditto (re: doing this in a follow-up PR)

Tracked in #7980

iefremov · 2021-02-17T09:19:57Z

patches/chrome-browser-net-net_error_tab_helper.cc.patch

 }

 bool NetErrorTabHelper::ProbesAllowed() const {
-+#if defined(BRAVE_CHROMIUM_BUILD)


maybe we could develop a way to avoid creation of certain tabhelpers, because it seems the whole thing is not needed for us

I think I lack context to properly understand what you mean here. Could you please elaborate a bit more?

It appears to me that the whole point of this class is sending some sort of "probes" which we disable in this method. So instead of patching we could maybe avoid instantiating the helper (it's unclear to me how exactly, just a raw idea). And probably there are more unneeded helpers (or keyed services) like this.

I see what you mean now, thanks for the clarification.

Handled in brave/brave-browser#14219

iefremov

Looks good to me, and it seems that there are few patches that can be converted to chormium_src files, especially the ones with constants (I've highlighted a few, but obviously there are more)

mariospr · 2021-02-17T09:58:02Z

Looks good to me, and it seems that there are few patches that can be converted to chormium_src files, especially the ones with constants (I've highlighted a few, but obviously there are more)

@iefremov Thanks for the review! I understand you're ok with doing the chromium_src suggested changes in a separate PR, so I'll work on that next, but let me know whether there's something else you want me to change in this particular one. Thanks!

iefremov · 2021-02-17T10:24:46Z

@mariospr I'm fine with this one, but it is patch masters who decide :)

mkarolin · 2021-02-17T21:42:52Z

chromium_src/chrome/browser/ui/webui/settings/metrics_reporting_handler.cc

@@ -3,12 +3,6 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

-#if defined(BRAVE_CHROMIUM_BUILD)
 #define GOOGLE_CHROME_BUILD


Unrelated to this PR, but we should probably include all headers from the original file here.

I'll prepare a separate PR for these changes. Thanks

On a second thought, I think the change is small enough we can include it here (I thought there would be many other cases defining GOOGLE_CHROME_BUILD in other chromium_src overrides, but turns out it's only these two files). Let me know if you prefer it on a separate PR, though, but otherwise I'm merging that into this PR.

mkarolin · 2021-02-17T21:43:45Z

chromium_src/chrome/browser/ui/webui/settings/metrics_reporting_handler.h

@@ -6,14 +6,8 @@
 #ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_WEBUI_SETTINGS_METRICS_REPORTING_HANDLER_H_
 #define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_WEBUI_SETTINGS_METRICS_REPORTING_HANDLER_H_

-#if defined(BRAVE_CHROMIUM_BUILD)
 #define GOOGLE_CHROME_BUILD


Same, unrelated to this PR, but we should probably include all headers from the original file here.

Ditto (i.e. will do on a separate PR)

As clarified above, the change is small enough (and only affects these files) that I'm including it in this PR in the end.

mkarolin · 2021-02-17T21:46:07Z

patches/chrome-BUILD.gn.patch

+      sources -= [ "app/chrome_exe.rc", ]
+      sources += [ "//brave/app/brave_exe.rc", ]


Can collapse into a single line.

Hmmm... how do we do that? Note that one it's an addition and the other one is a removal :)

Ah, ok... you mean putting the two statements in the same line, not merging the two lists. Ok

mkarolin · 2021-02-17T21:46:58Z

patches/chrome-BUILD.gn.patch

+    deps += [ "//brave:framework_bundle_data" ]
+    if (defined(chrome_framework_version)) { framework_contents += [ "Frameworks", ] }


Can collapse into a single line.

mkarolin · 2021-02-17T21:52:16Z

patches/chrome-browser-profiles-profile_io_data.cc.patch

 #endif
     content::kChromeUIScheme,
     content::kChromeUIUntrustedScheme,
-+#if defined(BRAVE_CHROMIUM_BUILD)
 +    kBraveUIScheme,


Unrelated to this PR, this can probably be done via a chromium_src override.

I actually looked into doing that yesterday, but it was not clear to me how we would do it besides creating a define in the chromium_src override and then patching this file to include that define. It would be 1 line vs 3 lines in the patch, though, so perhaps still worth it.

@mkarolin Could you clarify what you mean?

I think I better understood what you meant and got a PR already proposing a change, see #7995

mkarolin · 2021-02-17T21:56:56Z

patches/chrome-chrome_proxy-chrome_proxy_main_win.cc.patch

 +    FILE_PATH_LITERAL("brave.exe");
-+#else
+#if 0
     FILE_PATH_LITERAL("chrome.exe");
 +#endif


Wonder if we can simplify this by using something like true ? FILE_PATH_LITERAL("brave.exe") :

That would certainly work too. I'll change it if that's preferred.

Actually, I think I'll leave it as I had it for now if you don't mind, since doing what you suggest would go against some of the rules we have in the "Patching Chromium" wiki page:

"If possible write the patch to add a new line vs appending/prepending to an existing line."

I'm aware though that your suggestion means less lines added in the patch, but my hesitation comes from the fact that, if we're going to modify the existing line, we could very well then simply replace the string doing something like this:

- FILE_PATH_LITERAL("chrome.exe"); + FILE_PATH_LITERAL("brave.exe");

WDYT?

@mkarolin It doesn't seem this is blocking the merge, so I'm going to merge this PR today if it's ok to prevent it from getting conflicts (had to resolve some today already) if it's ok.

mkarolin

++
Need to update https://github.com/brave/brave-browser/wiki/Patching-Chromium re: using BRAVE_CHROMIUM_BUILD.

It will assumed from now on that all of Brave's own code plus patches to upstream Chromium will always be built and run as part of a Brave build, so we can drop these guards and reduce the patching surface. Note that github.com/brave/brave-browser/wiki/Patching-Chromium should also be updated to reflect the change implemented in this patch. Fix brave/brave-browser#12740

mariospr · 2021-02-22T14:55:51Z

The error in the post-init bot (re: npm run audit_deps) is unrelated to this change and this PR can be safely merged as discussed with @mihaiplesa in the morning.

mariospr · 2021-02-22T15:05:09Z

Merged and updated the wiki on https://github.com/brave/brave-browser/wiki/Patching-Chromium (i.e. removed paragraph referencing BRAVE_CHROMIUM_BUILD)

mariospr added this to the 1.22.x - Nightly milestone Feb 16, 2021

mariospr requested a review from iefremov February 16, 2021 12:19

mariospr requested review from a team as code owners February 16, 2021 12:19

mariospr self-assigned this Feb 16, 2021

mariospr requested review from emerick and zenparsing February 16, 2021 12:19

iefremov reviewed Feb 17, 2021

View reviewed changes

iefremov approved these changes Feb 17, 2021

View reviewed changes

mkarolin reviewed Feb 17, 2021

View reviewed changes

mkarolin approved these changes Feb 17, 2021

View reviewed changes

mariospr force-pushed the issues/12740 branch 4 times, most recently from 7b4211d to 704fa8b Compare February 18, 2021 15:16

mariospr force-pushed the issues/12740 branch from 704fa8b to b39991f Compare February 22, 2021 11:19

zenparsing approved these changes Feb 22, 2021

View reviewed changes

mariospr merged commit fc13ad8 into master Feb 22, 2021

mariospr deleted the issues/12740 branch February 22, 2021 15:03

		+ sources -= [ "app/chrome_exe.rc", ]
		+ sources += [ "//brave/app/brave_exe.rc", ]

		+ deps += [ "//brave:framework_bundle_data" ]
		+ if (defined(chrome_framework_version)) { framework_contents += [ "Frameworks", ] }

Remove usage of BRAVE_CHROMIUM_BUILD and brave_chromium_build #7969

Remove usage of BRAVE_CHROMIUM_BUILD and brave_chromium_build #7969

Conversation

mariospr commented Feb 16, 2021

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov left a comment

Choose a reason for hiding this comment

mariospr commented Feb 17, 2021

iefremov commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

mariospr commented Feb 22, 2021

mariospr commented Feb 22, 2021