Fix hotword module constantly restarting on Pixel 2.

Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.

Committed: https://crrev.com/9115bb0067e799534808890b56728e412814d8e3
Cr-Commit-Position: refs/heads/master@{#378177}

[Anand Mistry (off Chromium), 2016-02-24 03:42:25]

amistry@chromium.org changed reviewers:
+ mgiuca@chromium.org, tommi@chromium.org

[Anand Mistry (off Chromium), 2016-02-24 03:42:26]

tommi: Because you reviewed https://codereview.chromium.org/1581103002
mgiuca: As hotword owner

[Matt Giuca, 2016-02-24 04:00:14]

I don't understand this so will defer to Tommi (who is OWNER anyway). But in
principle I agree with this and it is fairly urgent (need to merge it into M49).

Note that the code has changed a lot since M49 (due to
https://crrev.com/370008), so this will not be a trivial merge to M49. But Anand
has prepared an equivalent patch to the M49 branch
(https://codereview.chromium.org/1731593002/) which we will manually land after
this is in trunk.

[Matt Giuca, 2016-02-25 03:23:07]

tommi: Ping. This is urgent (needs merge to M49).

[Matt Giuca, 2016-02-25 22:53:29]

mgiuca@chromium.org changed reviewers:
+ perkj@chromium.org

[Matt Giuca, 2016-02-25 22:53:29]

Hi Per,

I haven't gotten a response from Tommi. Would you be able to look at this?

If you get time, we also need the same fix applied in M49 where the code is very
different:
https://codereview.chromium.org/1731593002/

As I said previously, this is urgent (merge to M49 ASAP).

[perkj_chrome, 2016-02-26 11:25:15]

Description was changed from

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
==========

to

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
==========

[perkj_chrome, 2016-02-26 11:25:15]

perkj@chromium.org changed reviewers:
+ hta@chromium.org

[hta - Chromium, 2016-02-26 11:35:44]

Not quite right. Right should be simple.

Test it!

https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled =
true;
1) put a break in this loop.

2) instead use
  if (audio_basic.hasExact()) {
     controls->hotword_enabled =    
        audio_advanced.hotwordEnabled.exact();
  } else {
    if(audio_advanced.hotwordEnabled.hasExact()) {
     controls->hotword_enabled =      
         audio_advanced.hotwordEnabled.exact();
     break;
  }

Yes, there's an utility function to do this, but it's in a CL that hasn't been
submitted yet :-(

3) write an unit test!

[hta - Chromium, 2016-02-26 11:42:42]

On 2016/02/26 11:35:44, hta - Chromium wrote:
> Not quite right. Right should be simple.
> 
> Test it!
> 
>
https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
> File content/renderer/media/user_media_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
> content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled
=
> true;
> 1) put a break in this loop.
> 
> 2) instead use
>   if (audio_basic.hasExact()) {
>      controls->hotword_enabled =    
>         audio_advanced.hotwordEnabled.exact();
>   } else {
>     if(audio_advanced.hotwordEnabled.hasExact()) {
>      controls->hotword_enabled =      
>          audio_advanced.hotwordEnabled.exact();
>      break;
>   }
> 
> Yes, there's an utility function to do this, but it's in a CL that hasn't been
> submitted yet :-(
> 
> 3) write an unit test!

Arg, forgot the loop.

  if (audio_basic.hasExact()) {
    controls->hotword_enabled =    
       audio_basic.hotwordEnabled.exact();
  } else {
     for (const auto& audio_advanced: constraints) // like the loop you have
     if(audio_advanced.hotwordEnabled.hasExact()) {
      controls->hotword_enabled =      
          audio_advanced.hotwordEnabled.exact();
      break;
   }

The code you wrote may fail if the hotwords constraint is not the last
constraint in the advanced list (I think).

[hta - Chromium, 2016-02-26 11:42:45]

On 2016/02/26 11:35:44, hta - Chromium wrote:
> Not quite right. Right should be simple.
> 
> Test it!
> 
>
https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
> File content/renderer/media/user_media_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
> content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled
=
> true;
> 1) put a break in this loop.
> 
> 2) instead use
>   if (audio_basic.hasExact()) {
>      controls->hotword_enabled =    
>         audio_advanced.hotwordEnabled.exact();
>   } else {
>     if(audio_advanced.hotwordEnabled.hasExact()) {
>      controls->hotword_enabled =      
>          audio_advanced.hotwordEnabled.exact();
>      break;
>   }
> 
> Yes, there's an utility function to do this, but it's in a CL that hasn't been
> submitted yet :-(
> 
> 3) write an unit test!

Arg, forgot the loop.

  if (audio_basic.hasExact()) {
    controls->hotword_enabled =    
       audio_basic.hotwordEnabled.exact();
  } else {
     for (const auto& audio_advanced: constraints) // like the loop you have
     if(audio_advanced.hotwordEnabled.hasExact()) {
      controls->hotword_enabled =      
          audio_advanced.hotwordEnabled.exact();
      break;
   }

The code you wrote may fail if the hotwords constraint is not the last
constraint in the advanced list (I think).

[tommi (sloooow) - chröme, 2016-02-26 12:12:40]

Sorry about the delay. I've been heads down on other things in the past few
days.  Looks like Harald has suggested an alternate way of doing this, can you
give that a go and ping back? feel free to use IM.

[Matt Giuca, 2016-02-26 12:47:20]

On 2016/02/26 12:12:40, tommi-chromium wrote:
> Sorry about the delay. I've been heads down on other things in the past few
> days.  Looks like Harald has suggested an alternate way of doing this, can you
> give that a go and ping back? feel free to use IM.

We will have a look at doing this on Monday. Probably a good idea to add a unit
test, but I don't want it to hold us up (remember, we are in an urgent situation
here since we need to merge it back to M49). I also don't want to block on
unsubmitted CLs. I'll see what we can do.

[tommi (sloooow) - chröme, 2016-02-26 12:49:34]

On 2016/02/26 12:47:20, Matt Giuca wrote:
> On 2016/02/26 12:12:40, tommi-chromium wrote:
> > Sorry about the delay. I've been heads down on other things in the past few
> > days.  Looks like Harald has suggested an alternate way of doing this, can
you
> > give that a go and ping back? feel free to use IM.
> 
> We will have a look at doing this on Monday. Probably a good idea to add a
unit
> test, but I don't want it to hold us up (remember, we are in an urgent
situation
> here since we need to merge it back to M49). I also don't want to block on
> unsubmitted CLs. I'll see what we can do.

Understood.  I'm OK with doing the unit test separately and please file a bug
for that.  Just ping me when you want me to take a look.

[Matt Giuca, 2016-02-28 23:35:26]

On 2016/02/26 12:49:34, tommi-chromium wrote:
> Understood.  I'm OK with doing the unit test separately and please file a bug
> for that.  Just ping me when you want me to take a look.


OK.

Anand, can you please update the CL as suggested in #9--#11.

[Anand Mistry (off Chromium), 2016-02-29 02:10:07]

ptal

https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled =
true;
On 2016/02/26 11:35:44, hta - Chromium wrote:
> 1) put a break in this loop.

Done.

> 
> 2) instead use
>   if (audio_basic.hasExact()) {
>      controls->hotword_enabled =    
>         audio_advanced.hotwordEnabled.exact();
>   } else {
>     if(audio_advanced.hotwordEnabled.hasExact()) {
>      controls->hotword_enabled =      
>          audio_advanced.hotwordEnabled.exact();
>      break;
>   }
> 
> Yes, there's an utility function to do this, but it's in a CL that hasn't been
> submitted yet :-(

Done.

> 
> 3) write an unit test!

And there's the problem. It looks like this code doesn't have any unit tests. In
fact, I'm not sure I can write any unit tests given this nugget of comment in
UserMediaClientImpl::requestUserMedia():
// |user_media_request| can't be mocked. So in order to test at all we check
// if it isNull.

[Anand Mistry (off Chromium), 2016-02-29 02:26:52]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 02:26:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1725213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1725213002/20001

[hta - Chromium, 2016-02-29 05:56:45]

lgtm

assume tests are coming in a later CL
Note - still need tommi for owner lgtm

[commit-bot: I haz the power, 2016-02-29 06:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-29 06:07:28]

Dry run: This issue passed the CQ dry run.

[perkj_chrome, 2016-02-29 06:35:36]

rs lgtm

[Anand Mistry (off Chromium), 2016-02-29 06:37:57]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-02-29 06:38:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1725213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1725213002/20001

[commit-bot: I haz the power, 2016-02-29 06:43:19]

Description was changed from

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
==========

to

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
==========

[commit-bot: I haz the power, 2016-02-29 06:43:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-29 06:44:33]

Description was changed from

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
==========

to

==========
Fix hotword module constantly restarting on Pixel 2.

|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.

BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.

Committed: https://crrev.com/9115bb0067e799534808890b56728e412814d8e3
Cr-Commit-Position: refs/heads/master@{#378177}
==========

[commit-bot: I haz the power, 2016-02-29 06:44:35]

Patchset 2 (id:??) landed as
https://crrev.com/9115bb0067e799534808890b56728e412814d8e3
Cr-Commit-Position: refs/heads/master@{#378177}