Add UMA histogram DomDistiller.Time.RunJavaScript

Add UMA histogram DomDistiller.Time.RunJavaScript

This measures time spent in ExecuteJavaScript() in DomDistiller. It
includes JavaScript code transmission through IPC, parsing, compilation,
execution, and returning the JSON back through IPC.

We already have DomDistiller.Time.DistillationTotal, but it is measured
within JavaScript.

In DomDistiller.Time.*, DistillationTotal < RunJavaScript < DistillPage.

BUG=497430
Committed: https://crrev.com/96a824763c2cbb50b4b0bbad2b7531c3fdd5ce6e
Cr-Commit-Position: refs/heads/master@{#333670}

[wychen, 2015-05-29 21:33:44]

wychen@chromium.org changed reviewers:
+ cjhopman@chromium.org

[wychen, 2015-05-29 21:33:44]

PTAL

[cjhopman, 2015-06-04 20:38:57]

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents.cc:198: if
(javascript_start > base::TimeTicks()) {
should this be "> base::TimeTicks::Now()"?

[wychen, 2015-06-05 00:54:19]

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents.cc:198: if
(javascript_start > base::TimeTicks()) {
On 2015/06/04 20:38:57, cjhopman wrote:
> should this be "> base::TimeTicks::Now()"?

This "if" condition is only to exclude the DidFailLoad() case, so I'd say ">" or
"!= base::TimeTicks()" is more correct. "> base::TimeTicks::Now()" seems to be
an always-false condition.

[cjhopman, 2015-06-09 22:39:20]

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents.cc:198: if
(javascript_start > base::TimeTicks()) {
On 2015/06/05 00:54:19, wychen wrote:
> On 2015/06/04 20:38:57, cjhopman wrote:
> > should this be "> base::TimeTicks::Now()"?
> 
> This "if" condition is only to exclude the DidFailLoad() case, so I'd say ">"
or
> "!= base::TimeTicks()" is more correct. "> base::TimeTicks::Now()" seems to be
> an always-false condition.

"!= base::TimeTicks::Now()" is more clear that you aren't really comparing two
times. Even better is "if (!javascript_start.is_null())"

[cjhopman, 2015-06-09 22:41:14]

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents.cc:198: if
(javascript_start > base::TimeTicks()) {
On 2015/06/09 22:39:20, cjhopman wrote:
> On 2015/06/05 00:54:19, wychen wrote:
> > On 2015/06/04 20:38:57, cjhopman wrote:
> > > should this be "> base::TimeTicks::Now()"?
> > 
> > This "if" condition is only to exclude the DidFailLoad() case, so I'd say
">"
> or
> > "!= base::TimeTicks()" is more correct. "> base::TimeTicks::Now()" seems to
be
> > an always-false condition.
> 
> "!= base::TimeTicks::Now()" is more clear that you aren't really comparing two
> times. Even better is "if (!javascript_start.is_null())"

Sorry, that should start with:

"!= base::TimeTicks()" is more clear...

[wychen, 2015-06-10 00:31:25]

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/1143173009/diff/20001/components/dom_distille...
components/dom_distiller/content/distiller_page_web_contents.cc:198: if
(javascript_start > base::TimeTicks()) {
On 2015/06/09 22:41:14, cjhopman wrote:
> On 2015/06/09 22:39:20, cjhopman wrote:
> > On 2015/06/05 00:54:19, wychen wrote:
> > > On 2015/06/04 20:38:57, cjhopman wrote:
> > > > should this be "> base::TimeTicks::Now()"?
> > > 
> > > This "if" condition is only to exclude the DidFailLoad() case, so I'd say
> ">"
> > or
> > > "!= base::TimeTicks()" is more correct. "> base::TimeTicks::Now()" seems
to
> be
> > > an always-false condition.
> > 
> > "!= base::TimeTicks::Now()" is more clear that you aren't really comparing
two
> > times. Even better is "if (!javascript_start.is_null())"
> 
> Sorry, that should start with:
> 
> "!= base::TimeTicks()" is more clear...

Done.

[wychen, 2015-06-10 00:55:31]

wychen@chromium.org changed reviewers:
+ asvitkine@chromium.org

[wychen, 2015-06-10 00:55:31]

+asvitkine as histograms.xml owner.

[Alexei Svitkine (slow), 2015-06-10 01:01:54]

lgtm

[cjhopman, 2015-06-10 01:04:07]

lgtm

[wychen, 2015-06-10 01:06:41]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2015-06-10 01:10:09]

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

[commit-bot: I haz the power, 2015-06-10 03:13:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-10 03:14:46]

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