Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 6 months ago by Matt Giuca Modified:
5 years, 6 months ago Reviewers:
Nico, Anand Mistry (off Chromium) CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, rlp+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hotword-compiletimedisable Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable "Ok Google" hotwording in open source builds by default.
The compile-time flag "enable_hotwording" is now tied to
branding=Chrome (false by default unless making a Google Chrome build).
Note: Chromium will no longer download/install the Hotword Shared
Module, and will automatically remove the Hotword Shared Module on
startup if it was previously installed. To keep this functionality, add
"enable_hotwording=1" to GYP_DEFINES.
BUG=500922
Committed: https://crrev.com/0366a5184a70b3eefb5fcef2c2e13721669f00d8
Cr-Commit-Position: refs/heads/master@{#335874}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Trigger on branding=Chrome, not buildtype=Official. #
Total comments: 2
Created: 5 years, 6 months ago
Messages
Total messages: 19 (5 generated)
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
mgiuca@chromium.org changed reviewers: + amistry@chromium.org, thakis@chromium.org
The intention is to keep official builds the same as before, but disable this flag at compile-time for the default open source build.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200413003/1
https://codereview.chromium.org/1200413003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1200413003/diff/1/build/common.gypi#newcode839 build/common.gypi:839: 'enable_hotwording%': 1, You want branding=="Chrome", not buildtype. buildtype controls compiler optimisations. https://codereview.chromium.org/1200413003/diff/1/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1200413003/diff/1/chrome/browser/BUILD.gn#new... chrome/browser/BUILD.gn:26: enable_hotwording = is_official_build s/is_official_build/is_chrome_branded/
https://codereview.chromium.org/1200413003/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1200413003/diff/1/build/common.gypi#newcode839 build/common.gypi:839: 'enable_hotwording%': 1, On 2015/06/24 01:10:33, Anand Mistry wrote: > You want branding=="Chrome", not buildtype. buildtype controls compiler > optimisations. Done. https://codereview.chromium.org/1200413003/diff/1/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1200413003/diff/1/chrome/browser/BUILD.gn#new... chrome/browser/BUILD.gn:26: enable_hotwording = is_official_build On 2015/06/24 01:10:33, Anand Mistry wrote: > s/is_official_build/is_chrome_branded/ Done.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200413003/20001
https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi#newco... build/common.gypi:841: ['branding=="Chrome"', { Isn't buildtype=Official a better check?
https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi#newco... build/common.gypi:841: ['branding=="Chrome"', { On 2015/06/24 01:46:30, Nico wrote: > Isn't buildtype=Official a better check? See Anand's previous comment. (I initially had buildtype==Official.) From looking at other examples (e.g. bundling Flash), it looks like branding==Chrome is the correct check for proprietary stuff. buildtype==Official is about optimizations; branding controls both the iconography/titling, and the inclusion of proprietary components.
On 2015/06/24 01:59:47, Matt Giuca wrote: > https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/1200413003/diff/20001/build/common.gypi#newco... > build/common.gypi:841: ['branding=="Chrome"', { > On 2015/06/24 01:46:30, Nico wrote: > > Isn't buildtype=Official a better check? > > See Anand's previous comment. (I initially had buildtype==Official.) > > From looking at other examples (e.g. bundling Flash), it looks like > branding==Chrome is the correct check for proprietary stuff. buildtype==Official > is about optimizations; branding controls both the iconography/titling, and the > inclusion of proprietary components. The way I look at it is that branding controls the branding and/or things that require a src-internal checkout (since that's needed for the chrome branding resource files anyways), and buildtype controls compiler flags that might be interesting for benchmarking and that don't require src-internal. Downloading the hotword thingy doesn't require src-internal, but it has performance implications (the download, and having hotwording active on the NTP). So buildtype seems more appropriate to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> The way I look at it is that branding controls the branding and/or things that > require a src-internal checkout (since that's needed for the chrome branding > resource files anyways), and buildtype controls compiler flags that might be > interesting for benchmarking and that don't require src-internal. Downloading > the hotword thingy doesn't require src-internal, but it has performance > implications (the download, and having hotwording active on the NTP). So > buildtype seems more appropriate to me. I don't have a strong opinion about this, but I think the idea is that if buildtype==Official is just about performance, then theoretically* a downstream should be able to build a Chromium with buildtype==Official but not branding==Chrome, and get the performance benefits of buildtype==Official. The other thing that I think is important here is that we *want* proprietary material to be associated with the Chrome brand, not Chromium. If our stated goal is "Chrome can have proprietary material, Chromium cannot," then branding==Chrome is semantically the correct flag to gate this by. *Maybe it's not possible right now because people are misusing buildtype==Official, but we should be making things better not worse.
On 2015/06/24 03:18:21, Matt Giuca wrote: > > The way I look at it is that branding controls the branding and/or things that > > require a src-internal checkout (since that's needed for the chrome branding > > resource files anyways), and buildtype controls compiler flags that might be > > interesting for benchmarking and that don't require src-internal. Downloading > > the hotword thingy doesn't require src-internal, but it has performance > > implications (the download, and having hotwording active on the NTP). So > > buildtype seems more appropriate to me. > > I don't have a strong opinion about this, but I think the idea is that if > buildtype==Official is just about performance, then theoretically* a downstream > should be able to build a Chromium with buildtype==Official but not > branding==Chrome, and get the performance benefits of buildtype==Official. > > The other thing that I think is important here is that we *want* proprietary > material to be associated with the Chrome brand, not Chromium. If our stated > goal is "Chrome can have proprietary material, Chromium cannot," then > branding==Chrome is semantically the correct flag to gate this by. > > *Maybe it's not possible right now because people are misusing > buildtype==Official, but we should be making things better not worse. I too don't have a strong opinion (I've stated my weak opinion above). So lgtm either way I suppose.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200413003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0366a5184a70b3eefb5fcef2c2e13721669f00d8 Cr-Commit-Position: refs/heads/master@{#335874} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
