Add build flag to disable hotwording.

Add build flag to disable hotwording.

Hotwording downloads a shared module from the web store containing a NaCl module. There is a desire to build and distribute Chromium without this happening. This change adds an "enable_hotwording" build flag that is enabled by default, but can be disabled at compile time.

BUG=491435
Committed: https://crrev.com/f269d3b548203e217e8c0080c2e22e7ae3efb51e
Cr-Commit-Position: refs/heads/master@{#333548}

[Anand Mistry (off Chromium), 2015-06-03 04:58:39]

amistry@chromium.org changed reviewers:
+ scottmg@chromium.org, thestig@chromium.org

[Anand Mistry (off Chromium), 2015-06-03 04:58:39]

thestig@chromium.org: Please review changes in chrome/

scottmg@chromium.org: Please review changes in build/

[scottmg, 2015-06-03 05:16:13]

https://codereview.chromium.org/1160243004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/20001/build/common.gypi#newco...
build/common.gypi:2426: ['branding=="Chrome" and buildtype=="Official"', {
This should not be "and buildtype==Official", only branding==Chrome.

(buildtype is an optimization setting, not a product configuration setting)

[Anand Mistry (off Chromium), 2015-06-03 05:43:53]

https://codereview.chromium.org/1160243004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/20001/build/common.gypi#newco...
build/common.gypi:2426: ['branding=="Chrome" and buildtype=="Official"', {
On 2015/06/03 05:16:13, scottmg wrote:
> This should not be "and buildtype==Official", only branding==Chrome.
> 
> (buildtype is an optimization setting, not a product configuration setting)

Done.

[scottmg, 2015-06-03 05:48:09]

lgtm

https://codereview.chromium.org/1160243004/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/40001/build/common.gypi#newco...
build/common.gypi:2428: 'enable_hotwording%' : 1,
nit; no space before :

[Anand Mistry (off Chromium), 2015-06-03 22:54:08]

https://codereview.chromium.org/1160243004/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/40001/build/common.gypi#newco...
build/common.gypi:2428: 'enable_hotwording%' : 1,
On 2015/06/03 05:48:08, scottmg wrote:
> nit; no space before :

Done.

[Lei Zhang, 2015-06-04 23:11:00]

Shouldn't the CL description say "Chrome builds" instead of "official builds" ?

Does Hotwords work for Chromium builds?

https://codereview.chromium.org/1160243004/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/60001/build/common.gypi#newco...
build/common.gypi:2426: ['branding=="Chrome"', {
I'm not sure if this is what hangouts want. Maybe leave their conditonal alone?

[Anand Mistry (off Chromium), 2015-06-05 00:12:50]

Edited description. And yes, hotwording does work for chromium builds, but it
needs to download a component from the web store, hence the disabling. The
"enable_hotwording" build flag is for those of that that need to develop and
test the feature locally.

https://codereview.chromium.org/1160243004/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/60001/build/common.gypi#newco...
build/common.gypi:2426: ['branding=="Chrome"', {
On 2015/06/04 23:11:00, Lei Zhang wrote:
> I'm not sure if this is what hangouts want. Maybe leave their conditonal
alone?

Done. I've split hotwording into it's own section that's only determined by
branding.

[Lei Zhang, 2015-06-05 00:16:41]

On 2015/06/05 00:12:50, Anand Mistry wrote:
> Edited description. And yes, hotwording does work for chromium builds, but it
> needs to download a component from the web store, hence the disabling. The
> "enable_hotwording" build flag is for those of that that need to develop and
> test the feature locally.

So what's wrong with downloading a component from the web store? If a Ubuntu
Linux user chooses to use the chromium-browser package instead of
google-chrome-stable, wouldn't the user still want this feature?

[Anand Mistry (off Chromium), 2015-06-05 00:25:25]

On 2015/06/05 00:16:41, Lei Zhang wrote:
> On 2015/06/05 00:12:50, Anand Mistry wrote:
> > Edited description. And yes, hotwording does work for chromium builds, but
it
> > needs to download a component from the web store, hence the disabling. The
> > "enable_hotwording" build flag is for those of that that need to develop and
> > test the feature locally.
> 
> So what's wrong with downloading a component from the web store? If a Ubuntu
> Linux user chooses to use the chromium-browser package instead of
> google-chrome-stable, wouldn't the user still want this feature?

Sure, they could. But the current implementation always downloads that component
regardless of whether they use it or not. I know, we should download this
on-demand if the user enables it, but the effort required to do that and have a
good user experience is not one that we wish to do at this moment. This build
option at least gives distros some control.

[Lei Zhang, 2015-06-05 01:09:44]

On 2015/06/05 00:25:25, Anand Mistry wrote:
> On 2015/06/05 00:16:41, Lei Zhang wrote:
> > On 2015/06/05 00:12:50, Anand Mistry wrote:
> > > Edited description. And yes, hotwording does work for chromium builds, but
> it
> > > needs to download a component from the web store, hence the disabling. The
> > > "enable_hotwording" build flag is for those of that that need to develop
and
> > > test the feature locally.
> > 
> > So what's wrong with downloading a component from the web store? If a Ubuntu
> > Linux user chooses to use the chromium-browser package instead of
> > google-chrome-stable, wouldn't the user still want this feature?
> 
> Sure, they could. But the current implementation always downloads that
component
> regardless of whether they use it or not. I know, we should download this
> on-demand if the user enables it, but the effort required to do that and have
a
> good user experience is not one that we wish to do at this moment. This build
> option at least gives distros some control.

I thought we had plans to fix this across the board. i.e. Chrome users shouldn't
have to download the component either if they are not using hotwording. That
feels like the best general solution, though it may be harder to do.

[Lei Zhang, 2015-06-05 03:13:47]

Now that I saw https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=786909 and
better understand the issue here, please mention the fact that we don't we don't
want Chromium to download a closed-source binary blob by default either in the
CL or in a comment in the code.

https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi#newco...
build/common.gypi:394: # 'Ok Google' hotwording is disabled by default. Set to 1
to enable.
I think you should just check for GOOGLE_CHROME_BUILD in the code, rather than
adding this variable and #defining ENABLE_HOTWORDING. Usually, having something
like this variable and the corresponding ENABLE_HOTWORDING #define implies all
hotwording code will be build IFF this variable is set to 1.

[Anand Mistry (off Chromium), 2015-06-05 03:58:19]

https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi#newco...
build/common.gypi:394: # 'Ok Google' hotwording is disabled by default. Set to 1
to enable.
On 2015/06/05 03:13:46, Lei Zhang wrote:
> I think you should just check for GOOGLE_CHROME_BUILD in the code, rather than
> adding this variable and #defining ENABLE_HOTWORDING. Usually, having
something
> like this variable and the corresponding ENABLE_HOTWORDING #define implies all
> hotwording code will be build IFF this variable is set to 1.

Doesn't that require checking out the internal repo? I've never done it, can't
find the instructions on how to do it, and would prefer not to. But if you
insist...

[Lei Zhang, 2015-06-05 06:32:46]

On 2015/06/05 03:58:19, Anand Mistry wrote:
> https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi#newco...
> build/common.gypi:394: # 'Ok Google' hotwording is disabled by default. Set to
1
> to enable.
> On 2015/06/05 03:13:46, Lei Zhang wrote:
> > I think you should just check for GOOGLE_CHROME_BUILD in the code, rather
than
> > adding this variable and #defining ENABLE_HOTWORDING. Usually, having
> something
> > like this variable and the corresponding ENABLE_HOTWORDING #define implies
all
> > hotwording code will be build IFF this variable is set to 1.
> 
> Doesn't that require checking out the internal repo? I've never done it, can't
> find the instructions on how to do it, and would prefer not to. But if you
> insist...

No, you can use GOOGLE_CHROME_BUILD in the C++ code where you are currently
using ENABLE_HOTWORDING. I suppose that does make developing a bit harder. Hmm.

[Lei Zhang, 2015-06-05 06:44:47]

On 2015/06/05 06:32:46, Lei Zhang wrote:
> On 2015/06/05 03:58:19, Anand Mistry wrote:
> > https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi#newco...
> > build/common.gypi:394: # 'Ok Google' hotwording is disabled by default. Set
to
> 1
> > to enable.
> > On 2015/06/05 03:13:46, Lei Zhang wrote:
> > > I think you should just check for GOOGLE_CHROME_BUILD in the code, rather
> than
> > > adding this variable and #defining ENABLE_HOTWORDING. Usually, having
> > something
> > > like this variable and the corresponding ENABLE_HOTWORDING #define implies
> all
> > > hotwording code will be build IFF this variable is set to 1.
> > 
> > Doesn't that require checking out the internal repo? I've never done it,
can't
> > find the instructions on how to do it, and would prefer not to. But if you
> > insist...
> 
> No, you can use GOOGLE_CHROME_BUILD in the C++ code where you are currently
> using ENABLE_HOTWORDING. I suppose that does make developing a bit harder.
Hmm.

For development, you can just change the two #ifdefs to be true locally. It's a
bit of work, but not too bad.

[Anand Mistry (off Chromium), 2015-06-09 01:13:10]

On 2015/06/05 06:44:47, Lei Zhang wrote:
> On 2015/06/05 06:32:46, Lei Zhang wrote:
> > On 2015/06/05 03:58:19, Anand Mistry wrote:
> > > https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi
> > > File build/common.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1160243004/diff/80001/build/common.gypi#newco...
> > > build/common.gypi:394: # 'Ok Google' hotwording is disabled by default.
Set
> to
> > 1
> > > to enable.
> > > On 2015/06/05 03:13:46, Lei Zhang wrote:
> > > > I think you should just check for GOOGLE_CHROME_BUILD in the code,
rather
> > than
> > > > adding this variable and #defining ENABLE_HOTWORDING. Usually, having
> > > something
> > > > like this variable and the corresponding ENABLE_HOTWORDING #define
implies
> > all
> > > > hotwording code will be build IFF this variable is set to 1.
> > > 
> > > Doesn't that require checking out the internal repo? I've never done it,
> can't
> > > find the instructions on how to do it, and would prefer not to. But if you
> > > insist...
> > 
> > No, you can use GOOGLE_CHROME_BUILD in the C++ code where you are currently
> > using ENABLE_HOTWORDING. I suppose that does make developing a bit harder.
> Hmm.
> 
> For development, you can just change the two #ifdefs to be true locally. It's
a
> bit of work, but not too bad.

Done, but I'm going to voice my opposition to this. When I took over the
hotwording code, there were a number of obscure hoops to jump though to get this
to work locally. They've been eliminated. Having to find these pair of #defines
and changing them is more obscure to a newcomer than a build flag (that can
easily be traced through the code).

[Lei Zhang, 2015-06-09 01:34:04]

On 2015/06/09 01:13:10, Anand Mistry wrote:
> Done, but I'm going to voice my opposition to this. When I took over the
> hotwording code, there were a number of obscure hoops to jump though to get
this
> to work locally. They've been eliminated. Having to find these pair of
#defines
> and changing them is more obscure to a newcomer than a build flag (that can
> easily be traced through the code).

What about having a run time flag for enabling hotwording? We would always
enable hotwording for Google Chrome builds, and check for it in Chromium builds.
That way, Chromium users can make the decision to use the feature, rather than
the Chromium distributor. And it won't require hotwording developers to build
with GOOGLE_CHROME_BUILD or do any #ifdef hacking.

[Lei Zhang, 2015-06-09 01:34:47]

+pkasting for discussion on adding a new switch. ^

[Peter Kasting, 2015-06-09 02:17:42]

On 2015/06/09 01:34:04, Lei Zhang wrote:
> On 2015/06/09 01:13:10, Anand Mistry wrote:
> > Done, but I'm going to voice my opposition to this. When I took over the
> > hotwording code, there were a number of obscure hoops to jump though to get
> this
> > to work locally. They've been eliminated. Having to find these pair of
> #defines
> > and changing them is more obscure to a newcomer than a build flag (that can
> > easily be traced through the code).
> 
> What about having a run time flag for enabling hotwording? We would always
> enable hotwording for Google Chrome builds, and check for it in Chromium
builds.
> That way, Chromium users can make the decision to use the feature, rather than
> the Chromium distributor. And it won't require hotwording developers to build
> with GOOGLE_CHROME_BUILD or do any #ifdef hacking.

We shouldn't be exposing feature-enables to end users as runtime switches.

Honestly, I think the right thing to do is for Chromium users to get this by
default but have a build switch that can be used to disable it for projects like
Debian that feel more strongly about not having any external binary blobs used
for anything.  I don't think simply disabling this in general for Chromium is
correct.  It's not true that Chromium by definition should not make use of such
blobs, and I think the damage to developers and to more pragmatic users of
Chromium-based projects is greater than we should be willing to pay.

[Lei Zhang, 2015-06-09 02:48:57]

On 2015/06/09 02:17:42, Peter Kasting wrote:
> On 2015/06/09 01:34:04, Lei Zhang wrote:
> > On 2015/06/09 01:13:10, Anand Mistry wrote:
> > > Done, but I'm going to voice my opposition to this. When I took over the
> > > hotwording code, there were a number of obscure hoops to jump though to
get
> > this
> > > to work locally. They've been eliminated. Having to find these pair of
> > #defines
> > > and changing them is more obscure to a newcomer than a build flag (that
can
> > > easily be traced through the code).
> > 
> > What about having a run time flag for enabling hotwording? We would always
> > enable hotwording for Google Chrome builds, and check for it in Chromium
> builds.
> > That way, Chromium users can make the decision to use the feature, rather
than
> > the Chromium distributor. And it won't require hotwording developers to
build
> > with GOOGLE_CHROME_BUILD or do any #ifdef hacking.
> 
> We shouldn't be exposing feature-enables to end users as runtime switches.
> 
> Honestly, I think the right thing to do is for Chromium users to get this by
> default but have a build switch that can be used to disable it for projects
like
> Debian that feel more strongly about not having any external binary blobs used
> for anything.  I don't think simply disabling this in general for Chromium is
> correct.  It's not true that Chromium by definition should not make use of
such
> blobs, and I think the damage to developers and to more pragmatic users of
> Chromium-based projects is greater than we should be willing to pay.

I would be fine with having a build flag to disable hotwording that's enabled by
default. In which case, we'd go back to patch set 5, but flip the bits and
remove the check for for branding==chrome. With the flag set to 0, there will be
some dead hotwording code in the build, but someone can clean that up later.

[Anand Mistry (off Chromium), 2015-06-09 03:59:55]

On 2015/06/09 02:48:57, Lei Zhang wrote:
> On 2015/06/09 02:17:42, Peter Kasting wrote:
> > On 2015/06/09 01:34:04, Lei Zhang wrote:
> > > On 2015/06/09 01:13:10, Anand Mistry wrote:
> > > > Done, but I'm going to voice my opposition to this. When I took over the
> > > > hotwording code, there were a number of obscure hoops to jump though to
> get
> > > this
> > > > to work locally. They've been eliminated. Having to find these pair of
> > > #defines
> > > > and changing them is more obscure to a newcomer than a build flag (that
> can
> > > > easily be traced through the code).
> > > 
> > > What about having a run time flag for enabling hotwording? We would always
> > > enable hotwording for Google Chrome builds, and check for it in Chromium
> > builds.
> > > That way, Chromium users can make the decision to use the feature, rather
> than
> > > the Chromium distributor. And it won't require hotwording developers to
> build
> > > with GOOGLE_CHROME_BUILD or do any #ifdef hacking.
> > 
> > We shouldn't be exposing feature-enables to end users as runtime switches.
> > 
> > Honestly, I think the right thing to do is for Chromium users to get this by
> > default but have a build switch that can be used to disable it for projects
> like
> > Debian that feel more strongly about not having any external binary blobs
used
> > for anything.  I don't think simply disabling this in general for Chromium
is
> > correct.  It's not true that Chromium by definition should not make use of
> such
> > blobs, and I think the damage to developers and to more pragmatic users of
> > Chromium-based projects is greater than we should be willing to pay.
> 
> I would be fine with having a build flag to disable hotwording that's enabled
by
> default. In which case, we'd go back to patch set 5, but flip the bits and
> remove the check for for branding==chrome. With the flag set to 0, there will
be
> some dead hotwording code in the build, but someone can clean that up later.

Done. PTAL.

[Lei Zhang, 2015-06-09 18:50:03]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-06-09 18:50:06]

lgtm

[Lei Zhang, 2015-06-09 18:50:07]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1160243004/#ps120001
(title: "Back to flag and enable by default.")

[commit-bot: I haz the power, 2015-06-09 18:50:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160243004/120001

[commit-bot: I haz the power, 2015-06-09 19:18:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-09 19:19:49]

Patchset 7 (id:??) landed as
https://crrev.com/f269d3b548203e217e8c0080c2e22e7ae3efb51e
Cr-Commit-Position: refs/heads/master@{#333548}

[Lei Zhang, 2015-06-24 08:40:41]

On 2015/06/09 02:17:42, Peter Kasting wrote:
> On 2015/06/09 01:34:04, Lei Zhang wrote:
> > On 2015/06/09 01:13:10, Anand Mistry wrote:
> > > Done, but I'm going to voice my opposition to this. When I took over the
> > > hotwording code, there were a number of obscure hoops to jump though to
get
> > this
> > > to work locally. They've been eliminated. Having to find these pair of
> > #defines
> > > and changing them is more obscure to a newcomer than a build flag (that
can
> > > easily be traced through the code).
> > 
> > What about having a run time flag for enabling hotwording? We would always
> > enable hotwording for Google Chrome builds, and check for it in Chromium
> builds.
> > That way, Chromium users can make the decision to use the feature, rather
than
> > the Chromium distributor. And it won't require hotwording developers to
build
> > with GOOGLE_CHROME_BUILD or do any #ifdef hacking.
> 
> We shouldn't be exposing feature-enables to end users as runtime switches.
> 
> Honestly, I think the right thing to do is for Chromium users to get this by
> default but have a build switch that can be used to disable it for projects
like
> Debian that feel more strongly about not having any external binary blobs used
> for anything.  I don't think simply disabling this in general for Chromium is
> correct.  It's not true that Chromium by definition should not make use of
such
> blobs, and I think the damage to developers and to more pragmatic users of
> Chromium-based projects is greater than we should be willing to pay.

FTR, there was a second bug about this and another CL disabled the feature for
Chromium by default. I'll email chromium-packagers so the various Linux distros
can figure out what they want to do.
https://code.google.com/p/chromium/issues/detail?id=500922