[Hotword] Add error message display back in. Add error messages for NaCl. Add metrics for errors.

[Hotword] Add error message display back in. Add error messages for NaCl. Add metrics for errors.

BUG=345804, 360246
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272625

[rpetterson, 2014-05-16 23:47:20]

jhawkins -- webui, generated_resources
jered -- c/b/search/*
mpearson -- histograms (and related code in hotword_service.cc)

[James Hawkins, 2014-05-19 02:54:28]

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.js
(right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:91:
text.innerHTML = this.errorText_;
text.textContent

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:94:
help.innerHTML = this.helpLink_;
Same here.  But this should probably be an anchor element.

[Jered, 2014-05-19 16:45:51]

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:75: // Enum describing the types of
errors that can arise when determing
determing -> determining

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:77: // percentage compared to
successes.
I don't understand this sentence.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:122: error = GENERIC_ERROR;
Missing "break"s between these case labels.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:76: // no error;
nit; ; -> .

[rpetterson, 2014-05-19 17:39:17]

James, a specific question for you below.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.js
(right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:91:
text.innerHTML = this.errorText_;
On 2014/05/19 02:54:28, James Hawkins wrote:
> text.textContent

When I use textContent rather than innerHTML, the html of the text comes out
looking like this (basically verbatim rather than evaluating the html first):

<a target="_blank" href="chrome://plugins">Native Client plug-in</a> needs to be
enabled to use this feature.

<a target="_blank"
href="https://support.google.com/chrome/?p=ui_hotword_search">Help</a>

As opposed to just text with links. Any idea what I'm doing wrong?

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:75: // Enum describing the types of
errors that can arise when determing
On 2014/05/19 16:45:52, Jered wrote:
> determing -> determining

Done.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:77: // percentage compared to
successes.
On 2014/05/19 16:45:52, Jered wrote:
> I don't understand this sentence.

I've reworded it. Hopefully it's clearer.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:122: error = GENERIC_ERROR;
On 2014/05/19 16:45:52, Jered wrote:
> Missing "break"s between these case labels.

Whoops. Done.

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.h (right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.h:76: // no error;
On 2014/05/19 16:45:52, Jered wrote:
> nit; ; -> .

Done.

[Jered, 2014-05-19 20:47:33]

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:127: default:
You should probably hook up the other two errors, here, right? Otherwise they
will be reported as NO_ERROR.

[rpetterson, 2014-05-19 21:04:16]

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:127: default:
On 2014/05/19 20:47:34, Jered wrote:
> You should probably hook up the other two errors, here, right? Otherwise they
> will be reported as NO_ERROR.

There's only one other error right now -- the microphone. It's going to be added
as a separate CL since detecting it was more complicated. It's fine if it shows
up as NO_ERROR for now since we can just be aware of when we're looking at the
errors from (i.e., anything past some date will include the mic, before that
will not).

[Jered, 2014-05-19 21:15:42]

On 2014/05/19 21:04:16, rpetterson wrote:
>
https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
> File chrome/browser/search/hotword_service.cc (right):
> 
>
https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
> chrome/browser/search/hotword_service.cc:127: default:
> On 2014/05/19 20:47:34, Jered wrote:
> > You should probably hook up the other two errors, here, right? Otherwise
they
> > will be reported as NO_ERROR.
> 
> There's only one other error right now -- the microphone. It's going to be
added
> as a separate CL since detecting it was more complicated. It's fine if it
shows
> up as NO_ERROR for now since we can just be aware of when we're looking at the
> errors from (i.e., anything past some date will include the mic, before that
> will not).

ok, search lgtm,

[Mark P, 2014-05-19 22:01:31]

only one concern about the histograms
otherwise, histograms lgtm

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:127: default:
On 2014/05/19 21:04:17, rpetterson wrote:
> On 2014/05/19 20:47:34, Jered wrote:
> > You should probably hook up the other two errors, here, right? Otherwise
they
> > will be reported as NO_ERROR.
> 
> There's only one other error right now -- the microphone. It's going to be
added
> as a separate CL since detecting it was more complicated. It's fine if it
shows
> up as NO_ERROR for now since we can just be aware of when we're looking at the
> errors from (i.e., anything past some date will include the mic, before that
> will not).

This is a fine answer if you're pretty sure the new change will go in soon.  If
you're likely to have a stable release that includes this code without the
microphone error code, that could end up much more confusing, making it less
likely that people remember the changing interpretation by date.

[rpetterson, 2014-05-19 22:04:19]

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/289063012/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:127: default:
On 2014/05/19 22:01:31, Mark P wrote:
> On 2014/05/19 21:04:17, rpetterson wrote:
> > On 2014/05/19 20:47:34, Jered wrote:
> > > You should probably hook up the other two errors, here, right? Otherwise
> they
> > > will be reported as NO_ERROR.
> > 
> > There's only one other error right now -- the microphone. It's going to be
> added
> > as a separate CL since detecting it was more complicated. It's fine if it
> shows
> > up as NO_ERROR for now since we can just be aware of when we're looking at
the
> > errors from (i.e., anything past some date will include the mic, before that
> > will not).
> 
> This is a fine answer if you're pretty sure the new change will go in soon. 
If
> you're likely to have a stable release that includes this code without the
> microphone error code, that could end up much more confusing, making it less
> likely that people remember the changing interpretation by date.

It will be soon, probably this week. The CL is mostly done with one
implementation and we're just investigating if there's a better way to do it.

[James Hawkins, 2014-05-21 15:07:21]

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.js
(right):

https://codereview.chromium.org/289063012/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:91:
text.innerHTML = this.errorText_;
On 2014/05/19 17:39:17, rpetterson wrote:
> On 2014/05/19 02:54:28, James Hawkins wrote:
> > text.textContent
> 
> When I use textContent rather than innerHTML, the html of the text comes out
> looking like this (basically verbatim rather than evaluating the html first):
> 
> <a target="_blank" href="chrome://plugins">Native Client plug-in</a> needs to
be
> enabled to use this feature.
> 
> <a target="_blank"
> href="https://support.google.com/chrome/?p=ui_hotword_search">Help</a>
> 
> As opposed to just text with links. Any idea what I'm doing wrong?

Ah, I see.  The message from generated resources has an embedded anchor.  We'll
have to leave this as innerHTML then.

We could break up the message and build the UI using two spans with the second
being an anchor.  I don't think there's a lot of precedence in WebUI for doing
that, so I won't push it.

[James Hawkins, 2014-05-21 17:38:40]

https://codereview.chromium.org/289063012/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.js
(right):

https://codereview.chromium.org/289063012/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:94:
help.textContent = this.helpLink_;
You needed this to be innerHTML, right?

[rpetterson, 2014-05-21 17:54:30]

https://codereview.chromium.org/289063012/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/hotword_search_setting_indicator.js
(right):

https://codereview.chromium.org/289063012/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/hotword_search_setting_indicator.js:94:
help.textContent = this.helpLink_;
On 2014/05/21 17:38:41, James Hawkins wrote:
> You needed this to be innerHTML, right?

Yes. Done.

[James Hawkins, 2014-05-21 17:55:27]

lgtm

[rpetterson, 2014-05-22 17:07:41]

Jered, please take another look. I needed to add a #define in hotword_service.cc
to deal with some android compile errors.

[Jered, 2014-05-23 21:58:37]

On 2014/05/22 17:07:41, rpetterson wrote:
> Jered, please take another look. I needed to add a #define in
hotword_service.cc
> to deal with some android compile errors.

slgtm

[rpetterson, 2014-05-23 21:59:11]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-05-23 22:01:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlp@chromium.org/289063012/120001

[commit-bot: I haz the power, 2014-05-23 22:55:43]

Change committed as 272625

[Lei Zhang, 2014-06-11 07:23:57]

https://codereview.chromium.org/289063012/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/289063012/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6680: +      <message
name="IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE" desc="The error message text when
hotwording fails because the microphone isn't working.">
IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE isn't actually used anywhere. I'm going to
remove it unless someone objects.

[rpetterson, 2014-06-11 07:48:58]

On 2014/06/11 07:23:57, Lei Zhang wrote:
>
https://codereview.chromium.org/289063012/diff/120001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/289063012/diff/120001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:6680: +      <message
> name="IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE" desc="The error message text when
> hotwording fails because the microphone isn't working.">
> IDS_HOTWORD_MICROPHONE_ERROR_MESSAGE isn't actually used anywhere. I'm going
to
> remove it unless someone objects.

It's used in CL 293053013 which is near being committed except for a unit test
failure I plan to fix this week. If you so feel strongly, I can just add it back
in that CL.