Add extension event histogram values for messaging, webRequest, and webview.

Add extension event histogram values for messaging, webRequest, and webview.

These didn't make it into my last 5 patches because they needed to be hand
written. Now, everything is plumbed through to the UMA_HISTOGRAM_ENUMERATION
calls - which I will add in a follow-up - so this should be the last of it.

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

Committed: https://crrev.com/a9f6e67ad01169650c9627bba46e54d3ab0e1933
Cr-Commit-Position: refs/heads/master@{#342749}

[not at google - send to devlin, 2015-08-05 20:23:43]

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:777: // TODO(kalman): UMA specifically for
component extensions.
I will add the UMA_HISTOGRAM_ENUMERATION calls in a follow up, to reduce the
size of this change, and isolate the conversation about that. As you can see all
the info that I plan on using has been plumbed into here.

[not at google - send to devlin, 2015-08-05 20:24:44]

Oh, rdcronin - everything, asvitkine - extension_event_histogram_value and
histograms.xml.

[Devlin, 2015-08-05 21:34:09]

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:531: event, listener->filter(), false);
there are a few more of the anonymous bools than I'd like.  Can you document
them with the typical /* didn't enqueue */?

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:765: if (!url.SchemeIs(kExtensionScheme))
Doesn't this mean we won't log any events for messaging?  Or webRequest?

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:772: NOTREACHED() << "Reporting event for
non-existent extension at " << url;
Doing a NOTREACHED(); return; is typically a discouraged (anti)pattern, with the
exception of IPCs.  Is there a reason not to just DCHECK (or CHECK, if you like)
here?

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:801: if (!extension)
When could this happen?

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:198: // can be determined by the |process_id|
and |url| tuple. If the URL is
What process id?

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/guest_view_events.cc (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/guest_view_events.cc:86: std::map<std::string,
events::HistogramValue> values_;
Do we not support map brace initialization?  It's in C++0x...

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/guest_view_events.cc:87: };
DISALLOW_COPY_AND_ASSIGN

[not at google - send to devlin, 2015-08-07 20:55:04]

ping @ Alexei

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:531: event, listener->filter(), false);
On 2015/08/05 21:34:09, Devlin wrote:
> there are a few more of the anonymous bools than I'd like.  Can you document
> them with the typical /* didn't enqueue */?

Sure.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:765: if (!url.SchemeIs(kExtensionScheme))
On 2015/08/05 21:34:09, Devlin wrote:
> Doesn't this mean we won't log any events for messaging?  Or webRequest?

The URL was the target URL which assuming messaging works fine will be correct.
I've changed this to be an extension now though, so moot.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:772: NOTREACHED() << "Reporting event for
non-existent extension at " << url;
On 2015/08/05 21:34:09, Devlin wrote:
> Doing a NOTREACHED(); return; is typically a discouraged (anti)pattern, with
the
> exception of IPCs.  Is there a reason not to just DCHECK (or CHECK, if you
like)
> here?

Deliberate choice - I don't trust call sites, especially webview, but I want the
tests to fail at least. Maybe once this patch has baked this could be upgraded
to a CHECK.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:801: if (!extension)
On 2015/08/05 21:34:09, Devlin wrote:
> When could this happen?

In a previous version of the patch, I guess - and it was a great find, because
in removing it, it led to nice CL-wide change. Instead of passing the URL in
here, I'm just going to pass the Extension, which makes this simpler and less
error-prone since it's explicitly only logging Extension events.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:198: // can be determined by the |process_id|
and |url| tuple. If the URL is
On 2015/08/05 21:34:09, Devlin wrote:
> What process id?

Fixed.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/guest_view_events.cc (right):

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/guest_view_events.cc:86: std::map<std::string,
events::HistogramValue> values_;
On 2015/08/05 21:34:09, Devlin wrote:
> Do we not support map brace initialization?  It's in C++0x...

Neat! But sadly not allowed.

https://codereview.chromium.org/1272373003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/guest_view_events.cc:87: };
On 2015/08/05 21:34:09, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

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

https://codereview.chromium.org/1272373003/diff/20001/extensions/browser/even...
extensions/browser/event_router.cc:766: // event page, not that the event is
being dispatched *to* such a page.
This is new as well, I had forgotten that for event dispatch we just send to the
process, not a frame, then it's the process's job to figure out where to deliver
the event.

[Alexei Svitkine (slow), 2015-08-07 21:02:17]

lgtm

[Devlin, 2015-08-07 21:25:47]

lgtm

https://codereview.chromium.org/1272373003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/1272373003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:557:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK_EQ(target_extension != nullptr, params->extension_id.empty())?

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

https://codereview.chromium.org/1272373003/diff/20001/extensions/browser/even...
extensions/browser/event_router.h:94: const GURL& url,
We can kill the urls, now, right?

[not at google - send to devlin, 2015-08-10 20:34:35]

Patchset #3 (id:40001) has been deleted

[not at google - send to devlin, 2015-08-10 20:48:33]

https://codereview.chromium.org/1272373003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/1272373003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:557:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2015/08/07 21:25:47, Devlin (OOO til 8-13) wrote:
> DCHECK_EQ(target_extension != nullptr, params->extension_id.empty())?

Alright, though it's !empty() not empty. I.e. presence of a target extension
must correlate with there being a target extension ID.

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

https://codereview.chromium.org/1272373003/diff/20001/extensions/browser/even...
extensions/browser/event_router.h:94: const GURL& url,
On 2015/08/07 21:25:47, Devlin (OOO til 8-13) wrote:
> We can kill the urls, now, right?

Good point.

[not at google - send to devlin, 2015-08-10 20:49:16]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-08-10 20:49:16]

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/1272373003/#ps60001
(title: "devlin2")

[commit-bot: I haz the power, 2015-08-10 20:50:05]

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

[commit-bot: I haz the power, 2015-08-10 21:33:02]

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

[commit-bot: I haz the power, 2015-08-10 21:33:03]

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

[not at google - send to devlin, 2015-08-10 23:12:47]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-08-10 23:12:48]

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/1272373003/#ps80001
(title: "last")

[commit-bot: I haz the power, 2015-08-10 23:13:24]

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

[commit-bot: I haz the power, 2015-08-11 00:22:57]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-08-11 00:23:36]

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