Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
460113005:
Adds the audio history and always on hotwording settings. (Closed)
|
Created:
6 years, 4 months ago by kcarattini Modified:
6 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, rlp+watch_chromium.org, Jered, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdds the audio history and always on hotwording settings.
BUG=390086
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289577
Patch Set 1 #Patch Set 2 : fixed errors #
Total comments: 27
Patch Set 3 : Review Comment Modifications #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Responded to Review Comments #
Total comments: 2
Patch Set 6 : Alphabetical Reordering #
Total comments: 2
Patch Set 7 : Review Comments #
Messages
Total messages: 15 (0 generated)
First stab at adding the checkboxes to the settings page.
https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6625: + <message name="IDS_HOTWORD_SEARCH_PREF_CHKBOX" desc="A checkbox under Voice on the Settings page to allow audio capture devices to initiate searches."> It's probably not ideal to change a desc without changing the string, since it *might* trigger a re-translation. I discussed with Ben and he said "go ahead and change it, but remove the mention of the heading entirely so it's independent." (So just "a checkbox on the Settings page"). Similarly, drop "under Voice" in your new descriptions. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6632: + "Ok Google" detection from any screen See my comment below; I think you should collapse the two labels into one. How about: Enable "Ok Google" detection from anywhere, when the screen is on (Note: I changed "any screen" to "anywhere" since it's confusing having two usages of "screen" in the one sentence, and also users don't really think in terms of "screens".) https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6638: + Audio History Again, I think this should be collapsed. Since this is the same pref as the checkbox in the confirmation dialog, I think this should have the same text ("Improve voice search by ...") which is IDS_HOTWORD_AUDIO_LOGGING_ENABLE. So probably don't want a new string here. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:173: <div id="hotword-always-on-search" hidden> These do not have the logic we discussed where they would become disabled if the previous ones were unchecked. Do you plan to do that in a follow-up CL? (I'm happy with that btw, especially since the exact behaviour is unclear, but you should note it in the CL description.) There's also a bit more confusion I have with regards to the third checkbox -- in that it should grey out the second one (so maybe we need to swap those two), and also that it is duplicated on the confirmation screen of the first checkbox. Let's discuss the user flows and maybe we can simplify this tomorrow. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:182: <label for="hotword-always-on-search-enable" Having two labels for a checkbox feels weird. I think you should condense the text into a single label for each checkbox. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/search/ho... File chrome/browser/search/hotword_service_factory.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/search/ho... chrome/browser/search/hotword_service_factory.cc:109: prefs->RegisterBooleanPref(prefs::kHotwordAlwaysOnSearchEnabled, Does not compile without OS_CHROMEOS. Should be guarded by #ifdef (or not -- see my comment in pref_names.cc). https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Don't know where to put this comment but: I have a crash opening the settings page on Linux (non-ChromeOS) after compiling this patch. Please investigate. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1601: web_ui()->CallJavascriptFunction("BrowserOptions.showHotwordSection"); While you're here: there's a little "bug" here that |enabled| is not passed to showHotwordSection in the non-error case. Technically it doesn't matter, because showHotwordSection ignores |enabled| if |error_message| is undefined, but it's still good to pass the correct argument values. Could you move GetBoolean(prefs::kHotwordSearchEnabled) up above this if statement, and pass |enabled| as an argument here. Thanks. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1602: if (CommandLine::ForCurrentProcess()->HasSwitch( This should not be here -- move it after this if/else statement. (It should be shown in the error case too.) Note that *before* my CL (https://codereview.chromium.org/463823002/) these new checkboxes should be greyed out in the error case, but after my CL, it is fine to leave them enabled. https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names... chrome/common/pref_names.cc:1783: const char kHotwordAlwaysOnSearchEnabled[] = "hotword.always_on_search_enabled"; This needs to be in OS_CHROMEOS only (doesn't compile on non-CrOS build). You should move it to just before kConsumerManagementEnrollmentState, so it matches the order in pref_names.h. Alternatively, maybe this pref's existence should not be guarded by OS_CHROMEOS. Rather, just ignore it if OS_CHROMEOS is false (so that we have the possibility of enabling it later). Either way, make sure the .h and .cc files give things in the same order.
https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names... chrome/common/pref_names.cc:1783: const char kHotwordAlwaysOnSearchEnabled[] = "hotword.always_on_search_enabled"; On 2014/08/12 08:37:39, Matt Giuca wrote: > This needs to be in OS_CHROMEOS only (doesn't compile on non-CrOS build). You > should move it to just before kConsumerManagementEnrollmentState, so it matches > the order in pref_names.h. > > Alternatively, maybe this pref's existence should not be guarded by OS_CHROMEOS. > Rather, just ignore it if OS_CHROMEOS is false (so that we have the possibility > of enabling it later). Either way, make sure the .h and .cc files give things in > the same order. Please don't guard this with OS_CHROMEOS. It should work on regular chrome as well.
Can you also send screenshots? I'm not able to easily patch this to test right now. Thanks! https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names... chrome/common/pref_names.cc:1783: const char kHotwordAlwaysOnSearchEnabled[] = "hotword.always_on_search_enabled"; On 2014/08/12 08:46:35, Anand Mistry wrote: > On 2014/08/12 08:37:39, Matt Giuca wrote: > > This needs to be in OS_CHROMEOS only (doesn't compile on non-CrOS build). You > > should move it to just before kConsumerManagementEnrollmentState, so it > matches > > the order in pref_names.h. > > > > Alternatively, maybe this pref's existence should not be guarded by > OS_CHROMEOS. > > Rather, just ignore it if OS_CHROMEOS is false (so that we have the > possibility > > of enabling it later). Either way, make sure the .h and .cc files give things > in > > the same order. > > Please don't guard this with OS_CHROMEOS. It should work on regular chrome as > well. +1 My understanding is that although we'll be launching on ChromeOS this will be a feature available to power users on Chrome.
I've added a screenshot to the bug. PTAL, Kendra https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6625: + <message name="IDS_HOTWORD_SEARCH_PREF_CHKBOX" desc="A checkbox under Voice on the Settings page to allow audio capture devices to initiate searches."> On 2014/08/12 08:37:39, Matt Giuca wrote: > It's probably not ideal to change a desc without changing the string, since it > *might* trigger a re-translation. I discussed with Ben and he said "go ahead and > change it, but remove the mention of the heading entirely so it's independent." > (So just "a checkbox on the Settings page"). > > Similarly, drop "under Voice" in your new descriptions. Done. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6632: + "Ok Google" detection from any screen On 2014/08/12 08:37:39, Matt Giuca wrote: > See my comment below; I think you should collapse the two labels into one. How > about: > > Enable "Ok Google" detection from anywhere, when the screen is on > > (Note: I changed "any screen" to "anywhere" since it's confusing having two > usages of "screen" in the one sentence, and also users don't really think in > terms of "screens".) I've updated the strings to reflect the new Mocks the Hwi sent out today. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6638: + Audio History On 2014/08/12 08:37:39, Matt Giuca wrote: > Again, I think this should be collapsed. > > Since this is the same pref as the checkbox in the confirmation dialog, I think > this should have the same text ("Improve voice search by ...") which is > IDS_HOTWORD_AUDIO_LOGGING_ENABLE. So probably don't want a new string here. Same as above. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:173: <div id="hotword-always-on-search" hidden> On 2014/08/12 08:37:39, Matt Giuca wrote: > These do not have the logic we discussed where they would become disabled if the > previous ones were unchecked. Do you plan to do that in a follow-up CL? (I'm > happy with that btw, especially since the exact behaviour is unclear, but you > should note it in the CL description.) > > There's also a bit more confusion I have with regards to the third checkbox -- > in that it should grey out the second one (so maybe we need to swap those two), > and also that it is duplicated on the confirmation screen of the first checkbox. > Let's discuss the user flows and maybe we can simplify this tomorrow. This is just adding the pref and checkbox. Disabling behaviour will be added in another cl once we've decided on a course of action. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:182: <label for="hotword-always-on-search-enable" On 2014/08/12 08:37:39, Matt Giuca wrote: > Having two labels for a checkbox feels weird. I think you should condense the > text into a single label for each checkbox. Done. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/search/ho... File chrome/browser/search/hotword_service_factory.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/search/ho... chrome/browser/search/hotword_service_factory.cc:109: prefs->RegisterBooleanPref(prefs::kHotwordAlwaysOnSearchEnabled, On 2014/08/12 08:37:39, Matt Giuca wrote: > Does not compile without OS_CHROMEOS. Should be guarded by #ifdef (or not -- see > my comment in pref_names.cc). Acknowledged. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/08/12 08:37:39, Matt Giuca wrote: > Don't know where to put this comment but: I have a crash opening the settings > page on Linux (non-ChromeOS) after compiling this patch. Please investigate. Is this still happening for you? I can't reproduce after the updates. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1601: web_ui()->CallJavascriptFunction("BrowserOptions.showHotwordSection"); On 2014/08/12 08:37:39, Matt Giuca wrote: > While you're here: there's a little "bug" here that |enabled| is not passed to > showHotwordSection in the non-error case. Technically it doesn't matter, because > showHotwordSection ignores |enabled| if |error_message| is undefined, but it's > still good to pass the correct argument values. > > Could you move GetBoolean(prefs::kHotwordSearchEnabled) up above this if > statement, and pass |enabled| as an argument here. Thanks. Done. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1602: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/08/12 08:37:39, Matt Giuca wrote: > This should not be here -- move it after this if/else statement. (It should be > shown in the error case too.) > > Note that *before* my CL (https://codereview.chromium.org/463823002/) these new > checkboxes should be greyed out in the error case, but after my CL, it is fine > to leave them enabled. Done. https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/common/pref_names... chrome/common/pref_names.cc:1783: const char kHotwordAlwaysOnSearchEnabled[] = "hotword.always_on_search_enabled"; On 2014/08/12 18:21:57, rpetterson wrote: > On 2014/08/12 08:46:35, Anand Mistry wrote: > > On 2014/08/12 08:37:39, Matt Giuca wrote: > > > This needs to be in OS_CHROMEOS only (doesn't compile on non-CrOS build). > You > > > should move it to just before kConsumerManagementEnrollmentState, so it > > matches > > > the order in pref_names.h. > > > > > > Alternatively, maybe this pref's existence should not be guarded by > > OS_CHROMEOS. > > > Rather, just ignore it if OS_CHROMEOS is false (so that we have the > > possibility > > > of enabling it later). Either way, make sure the .h and .cc files give > things > > in > > > the same order. > > > > Please don't guard this with OS_CHROMEOS. It should work on regular chrome as > > well. > > +1 My understanding is that although we'll be launching on ChromeOS this will be > a feature available to power users on Chrome. Done. Sorry, everyone! Leftover from before I merged with Anand's change that moved the flag out of ChromeOS.
lgtm with 2 nits. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6632: + "Ok Google" detection from any screen On 2014/08/13 03:51:53, kcarattini wrote: > On 2014/08/12 08:37:39, Matt Giuca wrote: > > See my comment below; I think you should collapse the two labels into one. How > > about: > > > > Enable "Ok Google" detection from anywhere, when the screen is on > > > > (Note: I changed "any screen" to "anywhere" since it's confusing having two > > usages of "screen" in the one sentence, and also users don't really think in > > terms of "screens".) > I've updated the strings to reflect the new Mocks the Hwi sent out today. Acknowledged. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6638: + Audio History On 2014/08/13 03:51:53, kcarattini wrote: > On 2014/08/12 08:37:39, Matt Giuca wrote: > > Again, I think this should be collapsed. > > > > Since this is the same pref as the checkbox in the confirmation dialog, I > think > > this should have the same text ("Improve voice search by ...") which is > > IDS_HOTWORD_AUDIO_LOGGING_ENABLE. So probably don't want a new string here. > > Same as above. Nit: "Enable audio history" (no full-stop, no Title Case). https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:173: <div id="hotword-always-on-search" hidden> On 2014/08/13 03:51:53, kcarattini wrote: > On 2014/08/12 08:37:39, Matt Giuca wrote: > > These do not have the logic we discussed where they would become disabled if > the > > previous ones were unchecked. Do you plan to do that in a follow-up CL? (I'm > > happy with that btw, especially since the exact behaviour is unclear, but you > > should note it in the CL description.) > > > > There's also a bit more confusion I have with regards to the third checkbox -- > > in that it should grey out the second one (so maybe we need to swap those > two), > > and also that it is duplicated on the confirmation screen of the first > checkbox. > > Let's discuss the user flows and maybe we can simplify this tomorrow. > > This is just adding the pref and checkbox. Disabling behaviour will be added in > another cl once we've decided on a course of action. Acknowledged. https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/460113005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Yep, it's fixed; it was probably my fault (when I quick-fixed it yesterday to get it compiling on non-ChromeOS builds, I disabled the pref registration but something was still looking for that pref -- now the pref is available on all builds). https://codereview.chromium.org/460113005/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/460113005/diff/40001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:192: type="checkbox"> nit: this got indented 3 spaces unnecessarily.
+jhawkins for OWNERS approval. https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/460113005/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:6638: + Audio History On 2014/08/13 05:21:08, Matt Giuca wrote: > On 2014/08/13 03:51:53, kcarattini wrote: > > On 2014/08/12 08:37:39, Matt Giuca wrote: > > > Again, I think this should be collapsed. > > > > > > Since this is the same pref as the checkbox in the confirmation dialog, I > > think > > > this should have the same text ("Improve voice search by ...") which is > > > IDS_HOTWORD_AUDIO_LOGGING_ENABLE. So probably don't want a new string here. > > > > Same as above. > > Nit: "Enable audio history" (no full-stop, no Title Case). Done. https://codereview.chromium.org/460113005/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/460113005/diff/40001/chrome/browser/resources... chrome/browser/resources/options/browser_options.html:192: type="checkbox"> On 2014/08/13 05:21:09, Matt Giuca wrote: > nit: this got indented 3 spaces unnecessarily. Done.
drive by comment (don't wait for my lg) https://codereview.chromium.org/460113005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/460113005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:1940: 'showHotwordAlwaysOnSection', Nit: I think this should be alphabetical.
https://codereview.chromium.org/460113005/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/460113005/diff/80001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:1940: 'showHotwordAlwaysOnSection', On 2014/08/13 05:57:48, benwells wrote: > Nit: I think this should be alphabetical. Done.
LGTM with nit. https://codereview.chromium.org/460113005/diff/100001/chrome/browser/search/h... File chrome/browser/search/hotword_service_factory.cc (right): https://codereview.chromium.org/460113005/diff/100001/chrome/browser/search/h... chrome/browser/search/hotword_service_factory.cc:101: // Google unless they have opted into Hotwording at which point they must Technically this was more correct as 'in to', since it should really be: they have opted-in to Hotwording
https://codereview.chromium.org/460113005/diff/100001/chrome/browser/search/h... File chrome/browser/search/hotword_service_factory.cc (right): https://codereview.chromium.org/460113005/diff/100001/chrome/browser/search/h... chrome/browser/search/hotword_service_factory.cc:101: // Google unless they have opted into Hotwording at which point they must On 2014/08/13 14:16:55, James Hawkins wrote: > Technically this was more correct as 'in to', since it should really be: > > they have opted-in to Hotwording Done.
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcarattini@chromium.org/460113005/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Message was sent while issue was closed.
Committed patchset #7 (120001) as 289577 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
