Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1196243003:
chrome://voicesearch: Change some labels to clarify their meaning. (Closed)
|
Created:
5 years, 6 months ago by Matt Giuca Modified:
5 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchrome://voicesearch: Change some labels to clarify their meaning.
Also added a new field, "Hotword Module Installable", which shows
whether Chrome can install the hotword module at all. This will be false
if hotwording is disabled at compile time, or if the user's language is
not supported.
BUG=503496
Committed: https://crrev.com/142111475c337af89b7645eed66194cd36d93120
Cr-Commit-Position: refs/heads/master@{#335871}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Change strings per pkasting review. #Messages
Total messages: 22 (9 generated)
mgiuca@chromium.org changed reviewers: + amistry@chromium.org
Amistry: For initial review (no OWNERS).
lgtm
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196243003/1
mgiuca@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: For OWNERS, thanks.
https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... File chrome/browser/ui/webui/voice_search_ui.cc (right): https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:310: hotword_allowed = "Yes"; Nit: Why not just do these like we do for "microphone present"?: AddPair(list, ..., hotword_service && hotword_service->IsHotwordAllowed() ? "Yes" : "No"); https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:311: AddPair(list, "Hotword Allowed", hotword_allowed); Seems like this might be clearer as "Hotword Module Installable". https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:316: AddPair(list, "Hotword Search Opt-In", search_enabled); I don't know what "opt-in" means here. Perhaps you mean for it to mean "has the user opted in", but it reads more like "is the feature opt-in or not" (with the alternative being something like "no, it's opt-out"). If my guess at what you meant is correct, I think "enabled" is actually clearer and less scary. Otherwise, look for a different word to convey what you want, or else a different status than "yes"/"no".
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... File chrome/browser/ui/webui/voice_search_ui.cc (right): https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:310: hotword_allowed = "Yes"; Just consistency with the surrounding code. https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:311: AddPair(list, "Hotword Allowed", hotword_allowed); On 2015/06/24 02:03:32, Peter Kasting wrote: > Seems like this might be clearer as "Hotword Module Installable". Done. https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:316: AddPair(list, "Hotword Search Opt-In", search_enabled); Changed back to Enabled.
LGTM https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... File chrome/browser/ui/webui/voice_search_ui.cc (right): https://codereview.chromium.org/1196243003/diff/1/chrome/browser/ui/webui/voi... chrome/browser/ui/webui/voice_search_ui.cc:310: hotword_allowed = "Yes"; On 2015/06/24 03:12:19, Matt Giuca wrote: > Just consistency with the surrounding code. Can we make all the instances in this file consistent with each other if we're going for consistency? Personally I prefer the route I mentioned, but another option that's still more compact and efficient than this long form is: std::string hotword_allowed( hotword_service && hotword_service->IsHotwordAllowed() ? "Yes" : "No"); Yet another would be: std::string AsString(bool b) { return b ? "Yes" : "No"; } ... AddPair(list, "Hotword Allowed", AsString(hotword_service && hotword_service->IsHotwordAllowed()));
SGTM, since you're proposing a refactor much bigger than this CL, I'm punting to a follow-up: https://codereview.chromium.org/1203923002
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org Link to the patchset: https://codereview.chromium.org/1196243003/#ps20001 (title: "Change strings per pkasting review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196243003/20001
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org Link to the patchset: https://codereview.chromium.org/1196243003/#ps20001 (title: "Change strings per pkasting review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196243003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/142111475c337af89b7645eed66194cd36d93120 Cr-Commit-Position: refs/heads/master@{#335871} |
