Add UMA for window.external.AddSearchProvider (Chrome side).

Add UMA for window.external.AddSearchProvider (Chrome side).

See also https://codereview.chromium.org/1407163002/

BUG=542487, 521128
Committed: https://crrev.com/d7b05fb126ca30b98fa553abd7a8208a47024e1d
Cr-Commit-Position: refs/heads/master@{#355150}

[Evan Stade, 2015-10-17 00:58:38]

estade@chromium.org changed reviewers:
+ mpearson@chromium.org, pkasting@chromium.org

[Evan Stade, 2015-10-17 00:58:44]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-17 00:58:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410823003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410823003/1

[commit-bot: I haz the power, 2015-10-17 02:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-17 02:25:38]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2015-10-17 02:27:57]

LGTM

[Mark P, 2015-10-19 21:53:43]

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
File chrome/browser/ui/search_engines/edit_search_engine_controller.h (right):

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
chrome/browser/ui/search_engines/edit_search_engine_controller.h:36: // An enum
to match the AddSearchProvider UMA enum.
please explicitly do = 0, = 1, etc. (except for NUM_ACTIONS)
and also state that these items should not be reordered or renumbered; all new
items should be added just before NUM_ACTIONS

https://codereview.chromium.org/1410823003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1410823003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:52277: +  <int value="3" label="The user
saw the dialog but didn't add an engine"/>
but didn't add an engine
->
can you instead say something about cancelling or dismissing the window?

(For example, if the user sees the window and the chrome loses power, we don't
record the histogram even though the current label implies we should emit a
value of 3 to it.)

[Evan Stade, 2015-10-19 22:55:15]

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
File chrome/browser/ui/search_engines/edit_search_engine_controller.h (right):

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
chrome/browser/ui/search_engines/edit_search_engine_controller.h:36: // An enum
to match the AddSearchProvider UMA enum.
On 2015/10/19 21:53:43, Mark P wrote:
> please explicitly do = 0, = 1, etc. (except for NUM_ACTIONS)
> and also state that these items should not be reordered or renumbered; all new
> items should be added just before NUM_ACTIONS

Done.

Out of curiosity, what's the reasoning for being explicit about the values?
Doesn't the C++ standard guarantee these values anyway (even the = 0 should be
unnecessary)?

https://codereview.chromium.org/1410823003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1410823003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:52277: +  <int value="3" label="The user
saw the dialog but didn't add an engine"/>
On 2015/10/19 21:53:43, Mark P wrote:
> but didn't add an engine
> ->
> can you instead say something about cancelling or dismissing the window?
> 
> (For example, if the user sees the window and the chrome loses power, we don't
> record the histogram even though the current label implies we should emit a
> value of 3 to it.)

Done.

[esprehn, 2015-10-20 17:44:09]

On 2015/10/19 at 22:55:15, estade wrote:
>
https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
> File chrome/browser/ui/search_engines/edit_search_engine_controller.h (right):
> 
>
https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
> chrome/browser/ui/search_engines/edit_search_engine_controller.h:36: // An
enum to match the AddSearchProvider UMA enum.
> On 2015/10/19 21:53:43, Mark P wrote:
> > please explicitly do = 0, = 1, etc. (except for NUM_ACTIONS)
> > and also state that these items should not be reordered or renumbered; all
new
> > items should be added just before NUM_ACTIONS
> 
> Done.
> 
> Out of curiosity, what's the reasoning for being explicit about the values?
Doesn't the C++ standard guarantee these values anyway (even the = 0 should be
unnecessary)?

You're reporting them to UMA, that means if someone ever inserted a new value in
the middle all the historical reporting (or new reporting) would be wrong. We
always explicitly enumerate the values for things reported so you don't
accidentally screw up the reported enums.

[Evan Stade, 2015-10-20 18:34:37]

On 2015/10/20 17:44:09, esprehn wrote:
> You're reporting them to UMA, that means if someone ever inserted a new value
in
> the middle all the historical reporting (or new reporting) would be wrong. We
> always explicitly enumerate the values for things reported so you don't
> accidentally screw up the reported enums.

I get why the order should not change but that doesn't necessitate explicit
numbering, and I don't see how the explicit numbering prevents screwups.

[Mark P, 2015-10-20 19:37:28]

histograms.xml lgtm

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
File chrome/browser/ui/search_engines/edit_search_engine_controller.h (right):

https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
chrome/browser/ui/search_engines/edit_search_engine_controller.h:36: // An enum
to match the AddSearchProvider UMA enum.
On 2015/10/19 22:55:15, Evan Stade wrote:
> On 2015/10/19 21:53:43, Mark P wrote:
> > please explicitly do = 0, = 1, etc. (except for NUM_ACTIONS)
> > and also state that these items should not be reordered or renumbered; all
new
> > items should be added just before NUM_ACTIONS
> 
> Done.
> 
> Out of curiosity, what's the reasoning for being explicit about the values?
> Doesn't the C++ standard guarantee these values anyway (even the = 0 should be
> unnecessary)?

Yes, you're right.  However in my experience, being explicit about the values
makes it more obvious when someone else modifies the code to do something wrong.
 e.g., without explicit =whatever, I've seen people add new enums in the middle
(even with the UMA comment warning), and the code reviewer hasn't caught the
issue.  I've never seen this happen with explicit =whatever all the way down.

[Evan Stade, 2015-10-20 19:45:47]

On 2015/10/20 19:37:28, Mark P wrote:
> histograms.xml lgtm
> 
>
https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
> File chrome/browser/ui/search_engines/edit_search_engine_controller.h (right):
> 
>
https://codereview.chromium.org/1410823003/diff/1/chrome/browser/ui/search_en...
> chrome/browser/ui/search_engines/edit_search_engine_controller.h:36: // An
enum
> to match the AddSearchProvider UMA enum.
> On 2015/10/19 22:55:15, Evan Stade wrote:
> > On 2015/10/19 21:53:43, Mark P wrote:
> > > please explicitly do = 0, = 1, etc. (except for NUM_ACTIONS)
> > > and also state that these items should not be reordered or renumbered; all
> new
> > > items should be added just before NUM_ACTIONS
> > 
> > Done.
> > 
> > Out of curiosity, what's the reasoning for being explicit about the values?
> > Doesn't the C++ standard guarantee these values anyway (even the = 0 should
be
> > unnecessary)?
> 
> Yes, you're right.  However in my experience, being explicit about the values
> makes it more obvious when someone else modifies the code to do something
wrong.
>  e.g., without explicit =whatever, I've seen people add new enums in the
middle
> (even with the UMA comment warning), and the code reviewer hasn't caught the
> issue.  I've never seen this happen with explicit =whatever all the way down.

Fair enough. Sounds like a candidate for a presubmit check.

[Evan Stade, 2015-10-20 19:45:59]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-20 19:45:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1410823003/#ps20001
(title: "mpearson review")

[commit-bot: I haz the power, 2015-10-20 19:46:09]

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

[commit-bot: I haz the power, 2015-10-20 21:06:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-20 21:07:06]

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