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

Update wrong GOOGLE_CHROME_BUILD usage #3718

Merged
merged 2 commits into from
Oct 22, 2019
Merged

Update wrong GOOGLE_CHROME_BUILD usage #3718

merged 2 commits into from
Oct 22, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 16, 2019 •
edited by mkarolin
Loading

So far, we define explicitly GOOGLE_CHROME_BUILD in some specific source
files to use some code in that define guard.
However, some of them are replaced with
BUILDFLAG(GOOGLE_CHROME_BRANDING).
So, explicit define doesn't work after this change.

fix brave/brave-browser#6497
fix brave/brave-browser#6496

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@simonhong simonhong added this to the 0.73.x - Nightly milestone Oct 16, 2019
@simonhong simonhong self-assigned this Oct 16, 2019
@simonhong
Copy link
Member Author

simonhong commented Oct 16, 2019 •
edited
Loading

@kjozwiak, I confirmed that dmg from this PR plays well with netflix.

@kjozwiak
Copy link
Member

@simonhong quickly checked as well, looks like it's generally working. However, noticed that when you initially try playing something on Netflix on a fresh profile, you'll get the following:

Screen Shot 2019-10-16 at 10 51 30 AM

Once you refresh the page, everything works as expected. Is there a way to have the video play right away without the above error appearing first?

@simonhong
Copy link
Member Author

simonhong commented Oct 16, 2019 •
edited
Loading

@kjozwiak It's a known issue and the reason is reloading and installing are trigged in parallel when we click the install button. So, it can happen if reloading is done before install finishes. Maybe we already have issue for it.

@mkarolin
Copy link
Collaborator

Sadly, there are other places where I missed GOOGLE_CHROME_BUILD usage in our code, though they don't seem to actually affect anything. But mostly, I am not sure if we just want to flip the BUILDFLAG check. Since we are patching anyway, I think, it'd be better to use defined(BRAVE_CHROMIUM_BUILD) and such, just so we aren't dependent on chromium/google flags. What do you think?
Other places: 2b09e42

@simonhong
Copy link
Member Author

simonhong commented Oct 16, 2019 •
edited
Loading

@mkarolin yeah, I agree. it seems better to free from upstream flags.
Feel free to force push your changes to this PR or open new PR :)

@simonhong
Copy link
Member Author

@kjozwiak brave/brave-browser#4646 is the reloading issue after installing widevine.

@kjozwiak
Copy link
Member

@kjozwiak brave/brave-browser#4646 is the reloading issue after installing widevine.

yup 👍Definitely a known issue and shouldn't block this particular fix. Would be nice to get fixed eventually but as mentioned above, refreshing fixes the problem.

@mkarolin mkarolin force-pushed the fix_vmp_not_working branch 2 times, most recently from 04a108e to c81d5e7 Compare October 16, 2019 21:26
bridiver
bridiver reviewed Oct 17, 2019
View reviewed changes
patches/components-metrics-metrics_service_accessor.cc.patch Outdated
PrefService* pref_service) {
-#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING) || defined(BRAVE_CHROMIUM_BUILD)
return IsMetricsReportingEnabledForOfficialBuild(pref_service);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this change, we always want metrics reporting to be false

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has always been enabled for crash reporting.

bridiver
bridiver reviewed Oct 17, 2019
View reviewed changes
patches/chrome-installer-util-google_update_settings.cc.patch Outdated

return result == ERROR_SUCCESS;
-#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING) || (defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD)) // NOLINT
Copy link
Collaborator

Choose a reason for hiding this comment

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

we do not want to enable google updates, I don't understand this change

Copy link
Collaborator

@mkarolin mkarolin Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

These aren't functional changes, we had it enabled before.
The only difference is that before C78 this was under
#ifdef GOOGLE_CHROME_BUILD
and we used to define it just for the file (either via patch or via chromium_src).
Now, in C78 they are replacing #ifdef GOOGLE_CHROME_BUILD with
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
If we wanted to make that enabled for the file, we'd have to expand their macros and do something like

#if defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD)
#define BUILDFLAG_INTERNAL_GOOGLE_CHROME_BRANDING 1
#endif

like we did before.
In other places we do patch with the || (defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD)) and that gets us away from dependency on their future macro changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

fyi, we use open source omaha for windows update.
omaha is opensource version of google update.
And we reuse most of google update integration logic in chromium.

bridiver
bridiver requested changes Oct 17, 2019
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

see comments

@mkarolin mkarolin force-pushed the fix_vmp_not_working branch 2 times, most recently from aadaf6f to 852b273 Compare October 18, 2019 17:52
bridiver
bridiver reviewed Oct 18, 2019
View reviewed changes
chromium_src/chrome/installer/util/google_update_settings.cc Outdated
@@ -0,0 +1,51 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem right, we only added GOOGLE_CHROME_BUILD for part of the file, I don't think we want it defined for the whole file

Copy link
Collaborator

Choose a reason for hiding this comment

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

The patch had it defined in 41 (after the headers and the other patch) and undefined in line 836 at the end of the file, as far as I can tell.

bridiver
bridiver reviewed Oct 21, 2019
View reviewed changes
browser/component_updater/brave_component_updater_configurator.cc
@@ -185,11 +185,7 @@ BraveConfigurator::GetProtocolHandlerFactory() const {

update_client::RecoveryCRXElevator BraveConfigurator::GetRecoveryCRXElevator()
const {
#if defined(GOOGLE_CHROME_BUILD) && defined(OS_WIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like this was wrong from the start and should have be IS_OFFICIAL_BUILD for us?

Copy link
Collaborator

@mkarolin mkarolin Oct 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

GOOGLE_CHROME_BUILD was never defined for this file, as far as I can see, so the code inside was never used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bbondy is going to look into this for a potential follow-up

bsclifton
bsclifton approved these changes Oct 21, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

@bsclifton
Copy link
Member

Waiting for CI, then can merge

@bridiver @bsclifton
Fixes our use of GOOGLE_CHROME_BUILD
cf9cd47 
Fixes brave/brave-browser#6496
Fixes brave/brave-browser#6497

In places we reused GOOGLE_CHROME_BUILD by defining it in particular
files (either via chromium_src or by patching). GOOGLE_CHROME_BUILD is
being phased out in favor of BUILDFLAG(GOOGLE_CHROME_BRANDING). This fix
updates our uses where the transition has already occurred.

Chromium changes:

Overall issue tracker:
https://bugs.chromium.org/p/chromium/issues/detail?id=961769&can=2&q=961769

Some specific changes:

https://chromium.googlesource.com/chromium/src/+/0cc7112944131e379a141d8734a77cb50b97e1d5

commit 0cc7112944131e379a141d8734a77cb50b97e1d5
Author: Nico Weber <thakis@chromium.org>
Date:   Mon Jul 29 17:30:40 2019 +0000

    Add BUILDFLAG(GOOGLE_CHROME_BRANDING) and start using it in a few places.

    Also fix the definition of CHROMIUM_BRANDING; putting a negated condition
    there doesn't work. Thanks to Yngve Petterson for pointing that out.

    No behavior change.

    Bug: 961769

https://chromium.googlesource.com/chromium/src/+/356b304474c643e8064db763b97101100597bddd

commit 356b304474c643e8064db763b97101100597bddd
Author: Nico Weber <thakis@chromium.org>
Date:   Fri Aug 23 15:30:41 2019 +0000

    Remove all references to GOOGLE_CHROME_BUILD in components/

    Bug: 961769
@bsclifton
Copy link
Member

Has a lint error... otherwise looks fine. Going to push a fix for that and verify lint passes, then will merge 👍

@bsclifton bsclifton merged commit f3b4aab into master Oct 22, 2019
@bsclifton bsclifton deleted the fix_vmp_not_working branch October 22, 2019 00:03
brave-builds pushed a commit that referenced this pull request Oct 22, 2019
@brave-browser-releases
Uplift of #3718 (squashed) to dev
0a74ea0
brave-builds pushed a commit that referenced this pull request Oct 22, 2019
@brave-browser-releases
Uplift of #3718 (squashed) to beta
98666bb
brave-builds pushed a commit that referenced this pull request Oct 22, 2019
@brave-browser-releases
Uplift of #3718 (squashed) to release
05e235d
This was referenced Oct 22, 2019
Merged
Merged
Merged
@bsclifton
Copy link
Member

bsclifton commented Oct 22, 2019 •
edited
Loading

I don't think this is working as expected ☹️ When trying with an official build on Windows (Nightly version 0.73.17), I get the following when Widevine is installed and I try to view a video:
image

cc: @bridiver @mkarolin

@bsclifton
Copy link
Member

Tested again on Windows and macOS using https://github.com/brave/brave-browser/releases/tag/v0.73.19 and it worked great 👍 Please ignore the above (maybe the draft build was bad)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@bridiver bridiver bridiver approved these changes

@mkarolin mkarolin Awaiting requested review from mkarolin

Assignees

@simonhong simonhong

@mkarolin mkarolin

Labels
None yet
Projects
None yet
Milestone
1.2.x - Release
5 participants
@simonhong @kjozwiak @mkarolin @bsclifton @bridiver