Don't start the hotword extension on Chrome startup if the user doesn't have hotwording enabled.

Don't start the hotword extension on Chrome startup if the user doesn't have hotwording enabled.

BUG=488357
Committed: https://crrev.com/b8d26cffe9ffb2c403116993f68ae246a85ab359
Cr-Commit-Position: refs/heads/master@{#330322}

[Anand Mistry (off Chromium), 2015-05-15 05:15:37]

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

[Matt Giuca, 2015-05-15 06:54:42]

Can you file a bug (if one doesn't exist already) describing the problem you're
trying to solve, and link it here?

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword/state_manager.js:149:
chrome.runtime.onStartup.addListener(this.startupListener_);
Why not just not remove it if it hasn't been added?

if (chrome.runtime.onStartup.hasListener(this.startupListener_))
  chrome.runtime.onStartup.removeListener(this.startupListener_);

[Anand Mistry (off Chromium), 2015-05-15 07:08:36]

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword/state_manager.js:149:
chrome.runtime.onStartup.addListener(this.startupListener_);
On 2015/05/15 06:54:42, Matt Giuca wrote:
> Why not just not remove it if it hasn't been added?
> 
> if (chrome.runtime.onStartup.hasListener(this.startupListener_))
>   chrome.runtime.onStartup.removeListener(this.startupListener_);

Ah, undocumented and poorly understood behaviour of extension API events. Even
if there's no listener registered from the JS perspective, there could still be
a wake-up listener registered to re-start the event page. If order to clear
that, you first need to add one. Then when you remove it, JS sees the refcount
go to 0 and remove the wake-up event. So this forces one to be added so that the
JS part of the API implementation does the right thing. This pattern is used
heavily in this extension since it it's the only way to achieve deterministic
results.

[Matt Giuca, 2015-05-15 08:32:44]

LGTM with a bit more explanation in the comments.

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword/state_manager.js:149:
chrome.runtime.onStartup.addListener(this.startupListener_);
Gross :(

Could you explain that a bit in the comment (otherwise it just begs the
question: "why not just check if it's removed?")

(Not the whole thing, but just enough to let readers know that there could be
one registered even if you can't see it.)

[Matt Giuca, 2015-05-15 08:32:52]

PS thanks for doing this.

[Anand Mistry (off Chromium), 2015-05-18 01:24:23]

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/1130763005/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword/state_manager.js:149:
chrome.runtime.onStartup.addListener(this.startupListener_);
On 2015/05/15 08:32:43, Matt Giuca wrote:
> Gross :(
> 
> Could you explain that a bit in the comment (otherwise it just begs the
> question: "why not just check if it's removed?")
> 
> (Not the whole thing, but just enough to let readers know that there could be
> one registered even if you can't see it.)

Done.

[Anand Mistry (off Chromium), 2015-05-18 01:24:39]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-05-18 01:24:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/1130763005/#ps20001
(title: "Update comment.")

[commit-bot: I haz the power, 2015-05-18 01:24:49]

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

[commit-bot: I haz the power, 2015-05-18 02:20:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-18 11:33:24]

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