Final batch adding real histogram values for extension events.

Final batch adding real histogram values for extension events.

BUG=503402
R=rdevlin.cronin@chromium.org, asvitkine@chromium.org

Committed: https://crrev.com/47451623f615a4905eed5719a424f599b72b57a3
Cr-Commit-Position: refs/heads/master@{#339905}

[Devlin, 2015-07-20 21:28:26]

I assume the UMA logging itself will happen in a subsequent patch?

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc
(right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc:149: new
Event(events::TYPES_PRIVATE_CHROME_DIRECT_SETTING_ON_CHANGE,
This lumps multiple events into a single histogram, right?  I mean, given it's
private it's not as interesting, but it's odd that this is the only histogram
that does this.  Of course, there's a potentially large amount of possible
"events" here.  Gah.

If you can't think of anything better, I'd settle for an obvious comment here
saying why hardcoding the histogram value makes sense, even though it's a
dynamic event.

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:146:
events::WEB_NAVIGATION_ON_DOMCONTENT_LOADED,
nit: DOM_CONTENT?

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/event_router_forwarder_unittest.cc (right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/event_router_forwarder_unittest.cc:28: const
events::HistogramValue kHistogramValue = events::UNKNOWN;
An idea:

Can we make a histogram value FOR_TESTING and, in this patch (since it should be
the final one), DCHECK in Event construction that the value is not UNKNOWN? 
That way, we catch any existing funny places, and make sure other people don't
accidentally add any (because UNKNOWN is *not* a valid value).

https://codereview.chromium.org/1236493004/diff/1/extensions/browser/api/vpn_...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1236493004/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:165:
extensions::events::VPN_PROVIDER_ON_UIEVENT,
nit: UI_EVENT

[not at google - send to devlin, 2015-07-21 18:43:21]

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc
(right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc:149: new
Event(events::TYPES_PRIVATE_CHROME_DIRECT_SETTING_ON_CHANGE,
On 2015/07/20 21:28:26, Devlin wrote:
> This lumps multiple events into a single histogram, right?  I mean, given it's
> private it's not as interesting, but it's odd that this is the only histogram
> that does this.  Of course, there's a potentially large amount of possible
> "events" here.  Gah.
> 
> If you can't think of anything better, I'd settle for an obvious comment here
> saying why hardcoding the histogram value makes sense, even though it's a
> dynamic event.

Yes I'd added a similar comment to the TYPES_CHROME_SETTING_ON_CHANGE which has
the same problem. I've expanded that comment and copied it here.

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:146:
events::WEB_NAVIGATION_ON_DOMCONTENT_LOADED,
On 2015/07/20 21:28:26, Devlin wrote:
> nit: DOM_CONTENT?

Done in pre-patch.

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/event_router_forwarder_unittest.cc (right):

https://codereview.chromium.org/1236493004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/event_router_forwarder_unittest.cc:28: const
events::HistogramValue kHistogramValue = events::UNKNOWN;
On 2015/07/20 21:28:26, Devlin wrote:
> An idea:
> 
> Can we make a histogram value FOR_TESTING and, in this patch (since it should
be
> the final one), DCHECK in Event construction that the value is not UNKNOWN? 
> That way, we catch any existing funny places, and make sure other people don't
> accidentally add any (because UNKNOWN is *not* a valid value).

Good idea. I've added FOR_TEST in a pre-patch, fixed this up, and added a DCHECK
to the Event constructor.

https://codereview.chromium.org/1236493004/diff/1/extensions/browser/api/vpn_...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1236493004/diff/1/extensions/browser/api/vpn_...
extensions/browser/api/vpn_provider/vpn_service.cc:165:
extensions::events::VPN_PROVIDER_ON_UIEVENT,
On 2015/07/20 21:28:26, Devlin wrote:
> nit: UI_EVENT

Damn missed this one in my pre-patch. I'll do this in parallel, it'll either
land as a pre- or post-patch.

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
extensions/browser/event_router.cc:829: DCHECK_NE(events::UNKNOWN,
histogram_value)
DCHECK added, and changed this to use delegated constructors obviously so that I
don't need to DCHECK in 3 places.

[not at google - send to devlin, 2015-07-21 18:43:50]

kalman@chromium.org changed reviewers:
+ asvitkine@chromium.org
- isherman@chromium.org

[not at google - send to devlin, 2015-07-21 18:44:12]

Also changing reviewers isherman -> asvitkine. Let me know if you need more
details, but hopefully the bug has enough background.

[Alexei Svitkine (slow), 2015-07-21 19:09:40]

lgtm with a couple suggestions for the C++

https://codereview.chromium.org/1236493004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_helpers.h (right):

https://codereview.chromium.org/1236493004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_helpers.h:42:
base::ListValue* args,
Nit: Seems like this param can be const. Maybe even const ref?

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:351:
extensions::events::HistogramValue histogram_value =
Nit: Put the anon namespace this is in inside the extensions namespace that the
rest of the code is in, so you can avoid the extensions:: prefix in this code.

[Devlin, 2015-07-21 19:36:25]

lgtm

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
extensions/browser/event_router.cc:828: DCHECK(this->event_args.get());
nit: seeing this-> hurts my eyes enough to write this comment.  If it hurts
yours enough to change it, great. :)

[not at google - send to devlin, 2015-07-21 20:37:39]

https://codereview.chromium.org/1236493004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_helpers.h (right):

https://codereview.chromium.org/1236493004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_helpers.h:42:
base::ListValue* args,
On 2015/07/21 19:09:40, Alexei Svitkine wrote:
> Nit: Seems like this param can be const. Maybe even const ref?

Looking at the implementation of this method, it should actually be a
scoped_ptr<ListValue> - it takes a copy, which is really silly.

Because you've brought it up - and DeepCopies make me sad - I'll fix it up in a
follow-up. I want this patch to be as straightforward as possible, no chance of
introducing a bug, which a scoped_ptr<ListValue> change may cause.

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:351:
extensions::events::HistogramValue histogram_value =
On 2015/07/21 19:09:40, Alexei Svitkine wrote:
> Nit: Put the anon namespace this is in inside the extensions namespace that
the
> rest of the code is in, so you can avoid the extensions:: prefix in this code.

Done. I just moved the whole-damn-file into the extensions namespace. It's weird
that only part of it is.

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1236493004/diff/20001/extensions/browser/even...
extensions/browser/event_router.cc:828: DCHECK(this->event_args.get());
On 2015/07/21 19:36:25, Devlin wrote:
> nit: seeing this-> hurts my eyes enough to write this comment.  If it hurts
> yours enough to change it, great. :)

Done, because it bothered me too - but in such a way that will work.

[not at google - send to devlin, 2015-07-21 20:37:53]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-21 20:37:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1236493004/#ps40001
(title: "more stuff")

[commit-bot: I haz the power, 2015-07-21 20:38:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236493004/40001

[commit-bot: I haz the power, 2015-07-21 20:50:26]

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

[commit-bot: I haz the power, 2015-07-21 20:50:26]

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...)

[Lei Zhang, 2015-07-21 20:54:13]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-07-21 20:54:13]

remaining chrome/ files that need owners -> rs lgtm

[not at google - send to devlin, 2015-07-21 20:55:05]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-21 20:55:14]

On 2015/07/21 20:54:13, Lei Zhang wrote:
> remaining chrome/ files that need owners -> rs lgtm

thanks

[commit-bot: I haz the power, 2015-07-21 20:55:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236493004/40001

[commit-bot: I haz the power, 2015-07-21 20:58:40]

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

[commit-bot: I haz the power, 2015-07-21 20:58:41]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not at google - send to devlin, 2015-07-21 23:05:37]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-21 23:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rdevlin.cronin@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1236493004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-21 23:06:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236493004/60001

[commit-bot: I haz the power, 2015-07-21 23:27:56]

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

[commit-bot: I haz the power, 2015-07-21 23:27:57]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[not at google - send to devlin, 2015-07-22 16:20:25]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-07-22 16:20:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rdevlin.cronin@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1236493004/#ps80001
(title: "android compile")

[commit-bot: I haz the power, 2015-07-22 16:20:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236493004/80001

[commit-bot: I haz the power, 2015-07-22 16:29:36]

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

[commit-bot: I haz the power, 2015-07-22 16:29:37]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[not at google - send to devlin, 2015-07-22 16:32:59]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2015-07-22 16:33:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236493004/80001

[commit-bot: I haz the power, 2015-07-22 17:42:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-22 17:42:52]

Patchset 5 (id:??) landed as
https://crrev.com/47451623f615a4905eed5719a424f599b72b57a3
Cr-Commit-Position: refs/heads/master@{#339905}