-
Notifications
You must be signed in to change notification settings - Fork 892
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.
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
Remove usage of BRAVE_CHROMIUM_BUILD and brave_chromium_build #7969
Conversation
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for future work: all of these potentitally can be redefined via chromium_src?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no suggestions, let's leave them alone :)
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
candidate for chromium_src?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, will work on this (and other cases) in follow-up PRs if that's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked in #7996
|
||
content::BrowserContext* TemplateURLServiceFactory::GetBrowserContextToUse( | ||
content::BrowserContext* context) const { | ||
+#if defined(BRAVE_CHROMIUM_BUILD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto (re: doing this in a follow-up PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked in #7980
} | ||
|
||
bool NetErrorTabHelper::ProbesAllowed() const { | ||
+#if defined(BRAVE_CHROMIUM_BUILD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could develop a way to avoid creation of certain tabhelpers, because it seems the whole thing is not needed for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I lack context to properly understand what you mean here. Could you please elaborate a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean now, thanks for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled in brave/brave-browser#14219
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
@mariospr I'm fine with this one, but it is patch masters who decide :) |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but we should probably include all headers from the original file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll prepare a separate PR for these changes. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, unrelated to this PR, but we should probably include all headers from the original file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto (i.e. will do on a separate PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As clarified above, the change is small enough (and only affects these files) that I'm including it in this PR in the end.
patches/chrome-BUILD.gn.patch
Outdated
+ sources -= [ "app/chrome_exe.rc", ] | ||
+ sources += [ "//brave/app/brave_exe.rc", ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can collapse into a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... how do we do that? Note that one it's an addition and the other one is a removal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok... you mean putting the two statements in the same line, not merging the two lists. Ok
patches/chrome-BUILD.gn.patch
Outdated
+ deps += [ "//brave:framework_bundle_data" ] | ||
+ if (defined(chrome_framework_version)) { framework_contents += [ "Frameworks", ] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can collapse into a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
#endif | ||
content::kChromeUIScheme, | ||
content::kChromeUIUntrustedScheme, | ||
+#if defined(BRAVE_CHROMIUM_BUILD) | ||
+ kBraveUIScheme, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, this can probably be done via a chromium_src
override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I better understood what you meant and got a PR already proposing a change, see #7995
+ FILE_PATH_LITERAL("brave.exe"); | ||
+#else | ||
+#if 0 | ||
FILE_PATH_LITERAL("chrome.exe"); | ||
+#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we can simplify this by using something like true ? FILE_PATH_LITERAL("brave.exe") :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would certainly work too. I'll change it if that's preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
Need to update https://github.com/brave/brave-browser/wiki/Patching-Chromium re: using BRAVE_CHROMIUM_BUILD.
7b4211d
to
704fa8b
Compare
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
704fa8b
to
b39991f
Compare
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. |
Merged and updated the wiki on https://github.com/brave/brave-browser/wiki/Patching-Chromium (i.e. removed paragraph referencing |
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:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
N/A