MD Settings: Add Hotword (OK Google) section to search_page

MD Settings: Add Hotword (OK Google) section to search_page

The Hotword/OK Google section in the existing UI is... fragile.
This is an attempt to simplify and implement the UI without breaking
any existing functionality.
It would benefit from a re-implementation, ideally ising the
existing hotwordingPrivate API (which would may need to be expanded).

BUG=614439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1855b444e1246227907fdae45be098f2a1543982
Cr-Commit-Position: refs/heads/master@{#434830}

[stevenjb, 2016-11-17 01:30:01]

Description was changed from

==========
MD Settings: Add Hotword (OK Google) section to search_page

The Hotword/OK Google section in the existing UI is... fragile.
This is an attempt to simplify and implement the UI without breaking
any existing functionality.
It would benefit from a re-implementation, ideally ising the
existing hotwordingPrivate API (which would may need to be expanded).

BUG=614439
==========

to

==========
MD Settings: Add Hotword (OK Google) section to search_page

The Hotword/OK Google section in the existing UI is... fragile.
This is an attempt to simplify and implement the UI without breaking
any existing functionality.
It would benefit from a re-implementation, ideally ising the
existing hotwordingPrivate API (which would may need to be expanded).

BUG=614439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-11-21 23:39:14]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-11-27 22:25:37]

It could be helpful to sprinkle in a few comments about when "enabled" refers to
the service vs the setting vs the always-on setting, etc.

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1584: <message
name="IDS_SETTINGS_SEARCH_OK_GOOGLE_SUBTEXT_NO_DSP" desc="Subtext describing
when the OK Google hotword may be used for devices with no DSP.">
nit: explain what DSP is or leave it out if it isn't relevant

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1599: <message
name="IDS_SETTINGS_SEARCH_OK_GOOGLE_RETRAIN" desc="Text for button to retrain
the OG Google hotword voice model.">
OG => Ok

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_checkbox.html (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_checkbox.html:37:
<content></content>
Performance alarm bells are going off in my head... did you investigate perf
impact? (could be very minimal, just dunno)

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:55:
$i18n{learnMore}.
"." should be part of i18n if it's required. "Learn more" + "." doesn't
translate well.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:71: <div
inner-h-t-m-l="[[hotwordInfo_.errorMessage]]"</div>
missing >

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:79:
<div>[[i18n('searchOkGoogleAudioHistoryLabel',
nit: if [[]] has to break, can those be separate lines from the <div> </div>?

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/search_engines_handler.cc (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:386: // TODO: Update
hotword enabled and call DisableHotwordPreferences.
TODO(name)

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:409: //
OnGetHotwordAudioHistoryEnabled will call HotwordInfoComplete()..
remove '.'

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:502: std::string
pref_name = always_on ? prefs::kHotwordAlwaysOnSearchEnabled
but, always_on cannot be false due to the early return branch, above

https://codereview.chromium.org/2507363002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/search_page_test.js (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/search_page_test.js:122: // does not fire a
'change' event.
o_O There are some mock events that fire async, but tap is not one of them.

Are you sure the checkbox is enabled and not "pointer-events: none" for some
reason?

[stevenjb, 2016-11-28 20:53:37]

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1584: <message
name="IDS_SETTINGS_SEARCH_OK_GOOGLE_SUBTEXT_NO_DSP" desc="Subtext describing
when the OK Google hotword may be used for devices with no DSP.">
On 2016/11/27 22:25:31, michaelpg wrote:
> nit: explain what DSP is or leave it out if it isn't relevant
Done.

https://codereview.chromium.org/2507363002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:1599: <message
name="IDS_SETTINGS_SEARCH_OK_GOOGLE_RETRAIN" desc="Text for button to retrain
the OG Google hotword voice model.">
On 2016/11/27 22:25:32, michaelpg wrote:
> OG => Ok

Done.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_checkbox.html (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_checkbox.html:37:
<content></content>
On 2016/11/27 22:25:33, michaelpg wrote:
> Performance alarm bells are going off in my head... did you investigate perf
> impact? (could be very minimal, just dunno)

Alas, I am still unaware of an metric that is anywhere near precise enough to
measure any such impact.

You're not wrong to be concerned, but it sucks that web UI is so slow that we
have to worry about this sort of thing.

I removed the <span> and it looks the same, hopefully just inserting <content>
shouldn't have any real impact when it is empty?

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:55:
$i18n{learnMore}.
On 2016/11/27 22:25:36, michaelpg wrote:
> "." should be part of i18n if it's required. "Learn more" + "." doesn't
> translate well.

Switched to a hotword specific string.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:71: <div
inner-h-t-m-l="[[hotwordInfo_.errorMessage]]"</div>
On 2016/11/27 22:25:35, michaelpg wrote:
> missing >

<grumble>sloppy html parsing</grumble>

Done.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_page/search_page.html:79:
<div>[[i18n('searchOkGoogleAudioHistoryLabel',
On 2016/11/27 22:25:34, michaelpg wrote:
> nit: if [[]] has to break, can those be separate lines from the <div> </div>?

Done.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/search_engines_handler.cc (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:386: // TODO: Update
hotword enabled and call DisableHotwordPreferences.
On 2016/11/27 22:25:36, michaelpg wrote:
> TODO(name)

Oops. I meant to do this in this CL. I've added the logic to
HandleSetDefaultSearchEngine, along with an update push. (Previously the logic
was in HandleRequestHotwordAvaialble which was weird).

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:409: //
OnGetHotwordAudioHistoryEnabled will call HotwordInfoComplete()..
On 2016/11/27 22:25:36, michaelpg wrote:
> remove '.'

Done.

https://codereview.chromium.org/2507363002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/search_engines_handler.cc:502: std::string
pref_name = always_on ? prefs::kHotwordAlwaysOnSearchEnabled
On 2016/11/27 22:25:37, michaelpg wrote:
> but, always_on cannot be false due to the early return branch, above

Good point. Removed |pref_name|.

https://codereview.chromium.org/2507363002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/search_page_test.js (right):

https://codereview.chromium.org/2507363002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/search_page_test.js:122: // does not fire a
'change' event.
On 2016/11/27 22:25:37, michaelpg wrote:
> o_O There are some mock events that fire async, but tap is not one of them.
> 
> Are you sure the checkbox is enabled and not "pointer-events: none" for some
> reason?

Yes, the checkbox is enabled. I'm not sure how to check for "pointer-events:
none".

Apparently others have had issues with settings-checkbox change propagation
also. However, I also tried triggering the call to setHotwordSearchEnabled on a
property reflecting the checkbox.checked property (which worked in manual
testing) but MockInteractions.tap didn't work for that either. I've spent
another half day at this and I still have no idea why this doesn't work. Sigh.

[stevenjb, 2016-11-28 21:10:11]

OK, I tried one more thing, which I thought I had already tried, again and it
worked: tap(checkbox.$.checkbox) instead of tap(checkbox).

PTAL

[michaelpg, 2016-11-28 22:47:26]

On 2016/11/28 21:10:11, stevenjb wrote:
> OK, I tried one more thing, which I thought I had already tried, again and it
> worked: tap(checkbox.$.checkbox) instead of tap(checkbox).

OH.

paper-checkbox process "tap" events to toggle itself (or technically whichever
iron behavior it has).

settings-checkbox doesn't process any events. Which is fine, because in the UI,
the actual target of a click on a settings-checkbox is always the
<paper-checkbox> or the <cr-policy-pref-indicator>. So we have to simulate that
in tests -- click on the thing the user will click on (checkbox.$.checkbox, or
checkbox.$$('cr-policy-pref-indicator')). Good find!

> 
> PTAL

[michaelpg, 2016-11-28 22:57:07]

LGTM w/ a "developer usability" comment

https://codereview.chromium.org/2507363002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_checkbox.html (right):

https://codereview.chromium.org/2507363002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_checkbox.html:35:
<div>[[label]]<content></content></div>
let's at least add a space between [[label]] and <content></content>:

  <div>[[label]] <content></content></div>

so we never have whitespace between the label and content when both are present.
otherwise,

  <settings-checkbox label="foo">bar</settings-checkbox>

would display as "foobar" not "foo bar" (as in existing version).

[stevenjb, 2016-11-28 23:05:17]

https://codereview.chromium.org/2507363002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_checkbox.html (right):

https://codereview.chromium.org/2507363002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_checkbox.html:35:
<div>[[label]]<content></content></div>
On 2016/11/28 22:57:07, michaelpg wrote:
> let's at least add a space between [[label]] and <content></content>:
> 
>   <div>[[label]] <content></content></div>
> 
> so we never have whitespace between the label and content when both are
present.
> otherwise,
> 
>   <settings-checkbox label="foo">bar</settings-checkbox>
> 
> would display as "foobar" not "foo bar" (as in existing version).

Sure, that looks the same with the <a> tag I'm using. Done.

[stevenjb, 2016-11-28 23:05:31]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-11-28 23:05:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2507363002/#ps180001
(title: "Nit")

[commit-bot: I haz the power, 2016-11-28 23:07:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 02:08:12]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480374331448070,
"parent_rev": "c2d47ea9b74723b6150ba3d972ceec126a6f1208", "commit_rev":
"d116a1920867410228022ba7ad09e5c49761b9df"}

[commit-bot: I haz the power, 2016-11-29 02:09:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-29 02:12:07]

Description was changed from

==========
MD Settings: Add Hotword (OK Google) section to search_page

The Hotword/OK Google section in the existing UI is... fragile.
This is an attempt to simplify and implement the UI without breaking
any existing functionality.
It would benefit from a re-implementation, ideally ising the
existing hotwordingPrivate API (which would may need to be expanded).

BUG=614439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Add Hotword (OK Google) section to search_page

The Hotword/OK Google section in the existing UI is... fragile.
This is an attempt to simplify and implement the UI without breaking
any existing functionality.
It would benefit from a re-implementation, ideally ising the
existing hotwordingPrivate API (which would may need to be expanded).

BUG=614439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1855b444e1246227907fdae45be098f2a1543982
Cr-Commit-Position: refs/heads/master@{#434830}
==========

[commit-bot: I haz the power, 2016-11-29 02:12:08]

Patchset 10 (id:??) landed as
https://crrev.com/1855b444e1246227907fdae45be098f2a1543982
Cr-Commit-Position: refs/heads/master@{#434830}