Collect tab timing information for use in telementry-based startup tests

Collect tab timing information for use in telementry-based startup tests

Motivation: Data collection exposed in this CL is needed by upcoming startup tests we're writing using Telemtry.

Expose a new window.statsCollectionController object to JS and move existing histogram reading code into it since that seemed misplaced in DOMAutomationController.

Add a new --enable-stats-collection-bindings to activate said object.

Example usage in telemtry:
with browser.Create() as b:
  b.tabs[0].Navigate("http://www.google.com")
  b.tabs[0].WaitForDocumentReadyStateToBeComplete()
  print b.tabs[0].EvaluateJavaScript('statsCollectionController.tabLoadTiming()')

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202620

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202955

[jeremy, 2013-03-04 11:45:27]

tonyg/nat: review
avi: content/ owner

See CL description for open questions.

Would appreciate your thoughts.

[Avi (use Gerrit), 2013-03-05 18:44:03]

random owner comments; not a full review

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:21: // Singleton that listens
for tab liftime events (load/unload/crash) and records
lifetime

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:44: // Caller owns the return
value and is responsible for deleting it.
Then return it in a scoped_ptr to force the issue.

https://codereview.chromium.org/12389073/diff/2001/content/renderer/dom_autom...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/12389073/diff/2001/content/renderer/dom_autom...
content/renderer/dom_automation_controller.cc:224: CppVariant* result) {
indent

https://codereview.chromium.org/12389073/diff/2001/content/renderer/dom_autom...
content/renderer/dom_automation_controller.cc:242: CppVariant* result) {
indent

[marja, 2013-03-06 08:22:10]

I don't think I can review this meaningfully, I don't know enough about the
design of the components you're modifying, to say if this is the right thing or
not.

Since this CL doesn't contain Telemetry files, it's not clear how exactly are
you planning to integrate this into Telemetry. For waiting for navigations, can
you use the same approach that e.g., the memory benchmark uses, i.e., defining
actions that wait for other actions (wait_for_href_change, wait_for_navigation
and so on)?

[bulach, 2013-03-06 16:32:29]

this is great jeremy, thanks for taking the lead here!

from my understand of telemetry and your snippet:
==
b.tabs[0].EvaluateJavaScript('domAutomationController.enableTabLoadTiming(1)') 
==
I think this would be executed "too late", i.e., the browser would have already
been started, established the devtools connection, etc...
perhaps we should add a "command line flag" so we ensure this this would be
enabled super early on? or is it just for tab loading itself, rather than the
outer startup?

[tonyg, 2013-03-07 00:47:37]

With the exception of bulach's question, this looks reasonable to me. 

Thinking out loud, two questions come to mind:
1. Rather than exposing a new method to JS, should these load timings instead be
stored as a histogram and retrieved from telemetry that way? How would this fit
with our other startup histograms?
2. If we want to keep a new JS method rather than using histograms, would this
fit better in chrome_benchmarking_extension.cc than dom_automation_controller?

[jeremy, 2013-03-07 14:25:19]

Thanks everyone for your comments, very much appreciated!!

Promoting this from WIP to full review so would appreciate another look.

Tony: I think the central question is what information we need out of the test. 
The current tests in chrome/test/perf/startup_test.cc print out a bunch of stats
based on the individual tab loads (e.g. the time the first tab finishes loading,
end to end times etc.). 

Other comments:
I've removed enableTiming() in favor of always recording timing information when
the DOMAutomationController is active.  Simpler and I think the overhead of
always collecting is negligible.

marja: I may be mistaken, but the way I understand it - in Telmetry the test
needs to poll the browser rather than the browser calling back into the test so
I think that using WaitForDocumentReadyStateToBeComplete() to block rather than
blocking on another event is basically the same.  I don't think we get much out
of another event.

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:21: // Singleton that listens
for tab liftime events (load/unload/crash) and records
On 2013/03/05 18:44:03, Avi wrote:
> lifetime

Done.

https://codereview.chromium.org/12389073/diff/2001/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:44: // Caller owns the return
value and is responsible for deleting it.
On 2013/03/05 18:44:03, Avi wrote:
> Then return it in a scoped_ptr to force the issue.

Done.

https://codereview.chromium.org/12389073/diff/2001/content/renderer/dom_autom...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/12389073/diff/2001/content/renderer/dom_autom...
content/renderer/dom_automation_controller.cc:242: CppVariant* result) {
On 2013/03/05 18:44:03, Avi wrote:
> indent

Done.

[bulach, 2013-03-11 19:11:36]

good stuff, thanks Jeremy! I have some general comments below, but echoing
tonyg's question: I guess in general Telemetry is using more of "dev tools" (and
timeline, histograms, etc..) rather than "dom automation", so maybe moving to a
"startup_benchmark_extensions.cc" would be better?

tonyg: it'd be nice to have at the content layer rather than chrome though, so
that android could still benefit from it via content shell, wdyt?

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:34: // Overridden from
content::NotificationObserver:
nit: for consistency with the previous file:
"content::NotificationObserver implementation:"

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:67: class
InitialLoadObserver::TabTime {
since this is private to a class inside a .cc :) would a struct be sufficient?

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:124: crashed_tab_count_++;
nit: ++crashed_tab_count_;

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:177: const IPC::Message&
message,
nit: indent by +4 from beginning of line

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:194:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
just checking that the notifications also happen on this thread?

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.h (right):

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.h:24: // BrowserMessageFilter
implementation.
nit: I think you can remove 23-24 so it relies on the comment from 21

[nduca, 2013-03-12 19:37:39]

How does this patch relate to https://codereview.chromium.org/12770005/?

[jeremy, 2013-03-13 12:07:37]

nduca: Thanks for looking, this is only related to the other cl in that we need
these stats for future telemetry-based startup tests.

New iteration based on talk with Tony: Created a new
window.statsCollectionController JS object and moved histogram reading and the
new stat there.

The CL has grown in size but it's mostly renaming
domAutomationController->statsCollectionController.

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:34: // Overridden from
content::NotificationObserver:
On 2013/03/11 19:11:37, bulach wrote:
> nit: for consistency with the previous file:
> "content::NotificationObserver implementation:"

Done.

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:67: class
InitialLoadObserver::TabTime {
On 2013/03/11 19:11:37, bulach wrote:
> since this is private to a class inside a .cc :) would a struct be sufficient?

Done.

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:124: crashed_tab_count_++;
I prefer keeping it this way if that's ok.

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:177: const IPC::Message&
message,
On 2013/03/11 19:11:37, bulach wrote:
> nit: indent by +4 from beginning of line

Done.

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.cc:194:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
yep

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
File content/browser/stats_collection_message_filter.h (right):

https://codereview.chromium.org/12389073/diff/5002/content/browser/stats_coll...
content/browser/stats_collection_message_filter.h:24: // BrowserMessageFilter
implementation.
On 2013/03/11 19:11:37, bulach wrote:
> nit: I think you can remove 23-24 so it relies on the comment from 21

Done.

[nduca, 2013-03-13 16:52:25]

so, is this really a "controller"? And is stats a bit overgeneral? Like, why do
we have gpu_benchmarking_extension, memory_blah_extension, net_bench_extension.

I could see a histogram extension. I dont quite get what tab timings info is
part of... browser stats? That puzzles me. Is this misc_stats? General_Stats? I
give up. If we want to call it stats_extension, I can live with it.

https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:192: void
StatsCollectionMessageFilter::OnGetTabLoadTimingInformation(
Why is this all-tabs? How do we tell them apart? We have a list of all tabs in
telemtry, can't we ask the tabs we care about for their timing information
instead of having to get back a bulk pile of them?

https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:197:
base::JSONWriter::Write(return_value.get(), tab_timing_json);
I'm a little uneasy that we turn this into json at the browser side. It seems
like the message should be returning some struct, and then the jsonization
should really be a an absolute last second sort ofthing.

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
File content/renderer/stats_collection_controller.cc (right):

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:43: if (!histogram) {
one line ifs get formatted differently no?

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:52: CppVariant* result) {
funky indentation

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:71: const CppArgumentList& args,
funky indentation

[jeremy, 2013-03-13 18:50:07]

Thanks for the quick review Nat! Greatly appreciated!! Comments inline.

On Wed, Mar 13, 2013 at 6:52 PM, <nduca@chromium.org> wrote:

> so, is this really a "controller"? And is stats a bit overgeneral? Like,
> why do
> we have gpu_benchmarking_extension, memory_blah_extension,
> net_bench_extension.
>
> I could see a histogram extension. I dont quite get what tab timings info
> is
> part of... browser stats? That puzzles me. Is this misc_stats?
> General_Stats? I
> give up. If we want to call it stats_extension, I can live with it.
>

Does renaming the object as it exists in the current patch to
StatsCollectionExtension make sense to you?  If not, can you talk to Tony
and reach consensus about how you guys want these calls grouped and I'll
implement whatever you think is best.


> https://codereview.chromium.**org/12389073/diff/18001/**
>
content/browser/stats_**collection_message_filter.cc<https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_collection_message_filter.cc>
> File content/browser/stats_**collection_message_filter.cc (right):
>
> https://codereview.chromium.**org/12389073/diff/18001/**
>
content/browser/stats_**collection_message_filter.cc#**newcode192<https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_collection_message_filter.cc#newcode192>
> content/browser/stats_**collection_message_filter.cc:**192: void
> StatsCollectionMessageFilter::**OnGetTabLoadTimingInformation(
> Why is this all-tabs? How do we tell them apart? We have a list of all
> tabs in telemtry, can't we ask the tabs we care about for their timing
> information instead of having to get back a bulk pile of them?
>

At the moment I'm just duplicating the behavior of the current startup
tests for Telemetry so you can see the full usage in startup_tests.cc which
I linked to in the code review summary.

In short, we don't need to tell individual tabs apart (If we ever do then
hanging this off tabs as you mention makes total sense) the usage is to
record first/last tab load and averages on the numbers we have.

IMHO the current approach is simple and well encapsulated, I'm wary of
overengineering this and I'm willing to refactor should we need more
advanced usage - do you want me to file a bug to re-examine this once I'm
done porting all the startup tests?


> https://codereview.chromium.**org/12389073/diff/18001/**
>
content/browser/stats_**collection_message_filter.cc#**newcode197<https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_collection_message_filter.cc#newcode197>
> content/browser/stats_**collection_message_filter.cc:**197:
> base::JSONWriter::Write(**return_value.get(), tab_timing_json);
> I'm a little uneasy that we turn this into json at the browser side. It
> seems like the message should be returning some struct, and then the
> jsonization should really be a an absolute last second sort ofthing.
>

Are you concerned with security, if so, if I put this call behind the
existing "reduced security" flag will that address this?

[nduca, 2013-03-13 20:00:52]

Can you ping tonyg on the naming stuff? I'm  fine with whatever you two agree on
given my feedback.

Its a bit untelemetric to do a bulk tab dump. You could do the same thing ou
describe with a 1 tab call and then just say tabs[-1] and tabs[0]. I dont really
buy arguments that go "this is all we need and its how it was done before"... it
doesn't seem like we're talknig about a lot of code changes here right?

I'm not concerned with security about the location of the jsonization. Its a
design thing. The only place that cares that this is json is the extension... by
making this json higher up, you're making it hard to use this in any other way
than via the extension.

[jeremy, 2013-03-13 21:00:17]

On Wed, Mar 13, 2013 at 10:00 PM, <nduca@chromium.org> wrote:

> Can you ping tonyg on the naming stuff? I'm  fine with whatever you two
> agree on
> given my feedback.
>
Will do.

Its a bit untelemetric to do a bulk tab dump. You could do the same thing ou
> describe with a 1 tab call and then just say tabs[-1] and tabs[0]. I dont
> really
> buy arguments that go "this is all we need and its how it was done
> before"... it
> doesn't seem like we're talknig about a lot of code changes here right?
>

What you're describing is how I originally wanted to implement this. The
added complexity comes from tracking which tab in telemetry-land is
associated with which time.  The object doing the tracking has no knowledge
of the order of tab creation and adding that bit is what's significant (at
least that's what it seemed when I looked at this a couple of weeks ago).

If you feel strongly I can take another look.

I'm not concerned with security about the location of the jsonization. Its a
> design thing. The only place that cares that this is json is the
> extension... by
> making this json higher up, you're making it hard to use this in any other
> way
> than via the extension.
>
Sure, makes total sense.

[nduca, 2013-03-13 21:32:23]

Thanks for clarifying! So we can get the stats for creation time for all the
tabs, right?

How about just asking for the stats for each tab, one after another. Then do the
min of creation time python side to get the stats for the specific tab?

I know that technically this seems like overkill, but at least at that point,
we've got the ability to get data on the specific tab down the road.

[tonyg, 2013-03-13 23:00:14]

The name stats_collection_extension sounds good to me. Agree with Nat that
Controller doesn't seem to fit quite right. I'm not super-opinionated here
though.

[jeremy, 2013-03-20 10:28:20]

All fixed, ready for another look!

* renamed controller->extension
* encode json data in the renderer.

Nat, in response to:
> How about just asking for the stats for each tab, one after another. Then do
the
> min of creation time python side to get the stats for the specific tab?

There's no guarantee that the start times will be ordered the same as they are
in telemetry.  While I can't think of a counter-example, the coupling between
the telemetry and chrome sides seems loose at best. The way we're collecting the
tab timings currently means we don't really know which tab a specific time came
from.

If we need to separate the numbers by tab then I'm happy to circle back and
implement that but at present I think this will add complexity that currently
isn't needed.

https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/18001/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:197:
base::JSONWriter::Write(return_value.get(), tab_timing_json);
On 2013/03/13 16:52:25, nduca wrote:
> I'm a little uneasy that we turn this into json at the browser side. It seems
> like the message should be returning some struct, and then the jsonization
> should really be a an absolute last second sort ofthing.

Done.

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
File content/renderer/stats_collection_controller.cc (right):

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:43: if (!histogram) {
not if there's an else statement?

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:52: CppVariant* result) {
On 2013/03/13 16:52:25, nduca wrote:
> funky indentation

Done.

https://codereview.chromium.org/12389073/diff/18001/content/renderer/stats_co...
content/renderer/stats_collection_controller.cc:71: const CppArgumentList& args,
On 2013/03/13 16:52:25, nduca wrote:
> funky indentation

Done.

[jeremy, 2013-03-20 13:19:12]

nat: On second thoughts I'd like to send a vector of |TabTiming| structs via IPC
rather than a DictionaryValue, will change that in the next patch.  Would be
grateful for a review of the code save for that point. thanks!

[tonyg, 2013-03-21 01:49:50]

Looks reasonable to me. Nat should take a pass too.

https://codereview.chromium.org/12389073/diff/31026/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

https://codereview.chromium.org/12389073/diff/31026/chrome/test/ui/ui_test.cc...
chrome/test/ui/ui_test.cc:195:
launch_arguments_.AppendSwitch(switches::kStatsCollectionController);
Does this compile? I thought it was renamed.

[nduca, 2013-03-21 02:02:13]

https://codereview.chromium.org/12389073/diff/31026/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/31026/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:104: source.map_key(),
so whats this key? Presumably this ties back to a render widget host, no? Those
have routing IDs. The render_widget knows its routing id. The extension can get
to that, see the render_view. Then ask for timing ids for a given renderer id.

https://codereview.chromium.org/12389073/diff/31026/content/renderer/stats_co...
File content/renderer/stats_collection_extension.cc (right):

https://codereview.chromium.org/12389073/diff/31026/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:71: const CppArgumentList& args,
your indentation is funky here and above two funcs

[nduca, 2013-03-21 02:02:19]

I guess I'm waiting for the "next patch" where dictionaryization is done in the
extension rather than in the browser.

[jeremy, 2013-03-21 16:13:11]

All fixed, ready for another look.

https://codereview.chromium.org/12389073/diff/31026/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

https://codereview.chromium.org/12389073/diff/31026/chrome/test/ui/ui_test.cc...
chrome/test/ui/ui_test.cc:195:
launch_arguments_.AppendSwitch(switches::kStatsCollectionController);
On 2013/03/21 01:49:50, tonyg wrote:
> Does this compile? I thought it was renamed.

Done.

https://codereview.chromium.org/12389073/diff/31026/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/31026/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:104: source.map_key(),
This is a NotificationSource::map_key() which refers back to a
Source<NavigationController> from which I can get back to the WebContents
referring to the source tab.

https://codereview.chromium.org/12389073/diff/31026/content/renderer/stats_co...
File content/renderer/stats_collection_extension.cc (right):

https://codereview.chromium.org/12389073/diff/31026/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:71: const CppArgumentList& args,
On 2013/03/21 02:02:13, nduca wrote:
> your indentation is funky here and above two funcs

Done.

[jeremy, 2013-04-02 13:24:48]

nduca: Ready for another look, thanks for the pointers.

This patch changes the code to return timing data for the current tab rather
than all tabs.  The RenderView's routing id is used to identify a given tab. CL
description duly updated.

[tonyg, 2013-04-02 22:56:36]

lgtm

[jeremy, 2013-04-04 09:14:31]

Avi: Owner review for content/ plz.
Jay: Owner review for chrome/test plz.

[nduca, 2013-04-04 13:24:18]

lgtm on my part, thank you

[Avi (use Gerrit), 2013-04-04 15:41:55]

I'm good mod some nits, but jam has been wanting to review all content api
changes for a while. Tossing to him.

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
File content/browser/stats_collection_message_filter.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.h:15: class DictionaryValue;
No indent inside namespaces.

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
File content/renderer/stats_collection_extension.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.h:15: // It's use must be enabled
specifically via the
s/It's/Its/

[jam, 2013-04-04 17:28:39]

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:19: namespace {
ditto re putting this inside content namespace

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:28: content::WebContents*
contents = tab->GetWebContents();  // Can never be NULL.
nit: comment is redundant with code

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:70: size_t
crashed_tab_count_;
i don't see this used other than to collect?

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:83: registrar_.Add(this,
content::NOTIFICATION_LOAD_STOP,
so, it seems that this class is listening for LOAD_START/LOAD_STOP events in the
browser.

why not listen to them in the renderer, i.e. through
RenderViewObserver::DidStartLoading() and RenderViewObserver::DidStopLoading()?
then the change would be renderer-side only

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
File content/common/tab_load_stats.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
content/common/tab_load_stats.h:16: struct TabLoadTime {
tab is a chrome concept, content doesn't know about them.

this should be related to web contents

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
content/common/tab_load_stats.h:43: void ConvertTabLoadTimeToJSON(
this method is only called in the renderer. why is it in content\common?

https://codereview.chromium.org/12389073/diff/84005/content/public/common/con...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/public/common/con...
content/public/common/content_switches.h:90: CONTENT_EXPORT extern const char
kStatsCollectionExtension[];
extension is a chrome-loaded name. i would avoid it in content

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
File content/renderer/stats_collection_extension.cc (right):

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:33: content::RenderViewImpl*
render_view_impl =
nit: put the anonymous namespace in content so that you don't need "content::"

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:112: content::TabLoadTime
tab_load_time;
nit: "content::" needed

[Jay Civelli, 2013-04-04 17:41:37]

LGTM for chrome/test

[jeremy, 2013-04-07 14:51:54]

jam: all fixed, open issues:
* New name for StatsCollectionExtension?
* Can move to renderer be re-examined after startup tests are ported or does it
need to happen in this cl?

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:19: namespace {
On 2013/04/04 17:28:39, jam wrote:
> ditto re putting this inside content namespace

Done.

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:28: content::WebContents*
contents = tab->GetWebContents();  // Can never be NULL.
On 2013/04/04 17:28:39, jam wrote:
> nit: comment is redundant with code

Done.

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:70: size_t
crashed_tab_count_;
Removed.

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:83: registrar_.Add(this,
content::NOTIFICATION_LOAD_STOP,
On 2013/04/04 17:28:39, jam wrote:
> why not listen to them in the renderer, i.e. through
> RenderViewObserver::DidStartLoading() and
RenderViewObserver::DidStopLoading()?
> then the change would be renderer-side only

This makes sense but I fear I'm going to need to add more stats that require a
global view of tab loading when porting the startup tests, so I'd prefer to
leave this in the browser in the meantime.

Given that and the fact this CL has been under review for a month at this stage,
I would like to ask to be able to land this as is, once I'm done porting the
startup tests I can revisit and move this to the renderer if we discover that
there is no need to file this in the browser.

If you're worried about this cleanup not happening, I can file a bug or add a
todo but honestly unless you feel very strongly about this I'd rather get this
in, port the startup tests and then revisit.

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
File content/browser/stats_collection_message_filter.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.h:15: class DictionaryValue;
On 2013/04/04 15:41:56, Avi wrote:
> No indent inside namespaces.

Done.

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
File content/common/tab_load_stats.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
content/common/tab_load_stats.h:16: struct TabLoadTime {
Renamed tab -> web_contents globally.

https://codereview.chromium.org/12389073/diff/84005/content/common/tab_load_s...
content/common/tab_load_stats.h:43: void ConvertTabLoadTimeToJSON(
On 2013/04/04 17:28:39, jam wrote:
> this method is only called in the renderer. why is it in content\common?

Done.

https://codereview.chromium.org/12389073/diff/84005/content/public/common/con...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/public/common/con...
content/public/common/content_switches.h:90: CONTENT_EXPORT extern const char
kStatsCollectionExtension[];
np, What do you want me to call this?

Note that I already changed the name in response to comments by other reviewers.

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
File content/renderer/stats_collection_extension.cc (right):

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:33: content::RenderViewImpl*
render_view_impl =
On 2013/04/04 17:28:39, jam wrote:
> nit: put the anonymous namespace in content so that you don't need "content::"

Done.

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:112: content::TabLoadTime
tab_load_time;
On 2013/04/04 17:28:39, jam wrote:
> nit: "content::" needed

Done.

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.cc:112: content::TabLoadTime
tab_load_time;
On 2013/04/04 17:28:39, jam wrote:
> nit: "content::" needed

Done.

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
File content/renderer/stats_collection_extension.h (right):

https://codereview.chromium.org/12389073/diff/84005/content/renderer/stats_co...
content/renderer/stats_collection_extension.h:15: // It's use must be enabled
specifically via the
On 2013/04/04 15:41:56, Avi wrote:
> s/It's/Its/

Done.

[jam, 2013-04-07 21:14:04]

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
File content/browser/stats_collection_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/84005/content/browser/stats_col...
content/browser/stats_collection_message_filter.cc:83: registrar_.Add(this,
content::NOTIFICATION_LOAD_STOP,
On 2013/04/07 14:51:54, jeremy wrote:
> On 2013/04/04 17:28:39, jam wrote:
> > why not listen to them in the renderer, i.e. through
> > RenderViewObserver::DidStartLoading() and
> RenderViewObserver::DidStopLoading()?
> > then the change would be renderer-side only
> 
> This makes sense but I fear I'm going to need to add more stats that require a
> global view of tab loading when porting the startup tests, so I'd prefer to
> leave this in the browser in the meantime.

Can you elaborate on what you think you'll need?

I think we need to know this before this change lands. While I'm sympathetic
that the change has been under review for a long time, I think it's very
important that we only land what we need. Part of fighting code complexity is
making every change the simplest it can be to accomplish what's required. I'm
curious what data would be needed that wouldn't be accessible in the renderer,
since it seems that it has all the information about loading state.

> 
> Given that and the fact this CL has been under review for a month at this
stage,
> I would like to ask to be able to land this as is, once I'm done porting the
> startup tests I can revisit and move this to the renderer if we discover that
> there is no need to file this in the browser.
> 
> If you're worried about this cleanup not happening, I can file a bug or add a
> todo but honestly unless you feel very strongly about this I'd rather get this
> in, port the startup tests and then revisit.

[jeremy, 2013-04-08 04:41:45]

John: What name do you want me to use for StatsCollectionExtension?

On Mon, Apr 8, 2013 at 12:14 AM, <jam@chromium.org> wrote:

> This makes sense but I fear I'm going to need to add more stats that
> require a
>
>> global view of tab loading when porting the startup tests, so I'd
>>
> prefer to
>
>> leave this in the browser in the meantime.
>>
>
> Can you elaborate on what you think you'll need?
>

The context for this CL is that it's test infrastructure needed to port our
existing startup tests to Telemetry in order to make them more stable and
move them off old infrastructure. This CL started life as a straight port
of InitialLoadObserver as used in those tests.

One possible scenario where I'll need this to be in the browser is if we
run into issues around terminating a test run e.g. we load N tabs and need
to block until all tabs are loaded.  The current tests tell the
InitialLoadObserver to block until a given number of tabs have loaded and
then report stats back, it includes crashed tabs in this count. In
Telemetry we want to block from the Python side.  If the new approach turns
out to be flaky we may need to revert to the old method.

Another scenario is if we need to collect stats as part of the test run
that only the browser knows about (e.g. latency in launching the renderer
process for a given tab).

There's a distinct possibility that after rewriting the tests in Telemetry
this won't be needed in which case moving this code to the renderer should
be simple enough.  Currently though I'd prefer to have this flexibility.

I understand and share your concerns about code complexity, but personally
I think this change is localized and uninvasive enough that getting a bit
of flexibility in the early stages doesn't hurt much.

If you feel strongly about this, please say so and I'll move things to the
renderer.  I'd rather have the additional flexibility the current code
affords me [I'm also, quite frankly pretty sick of finessing this CL :) ]
but I trust your judgement here.

Best regards,
Jeremy

[jam, 2013-04-08 15:13:00]

On 2013/04/08 04:41:45, jeremy wrote:
> John: What name do you want me to use for StatsCollectionExtension?

The cl description says the JS object is statsCollectionController, but the code
looks like it's statsCollectionExtension. If it's statsCollectionController,
then the C++ code can just be called that.

> 
> On Mon, Apr 8, 2013 at 12:14 AM, <mailto:jam@chromium.org> wrote:
> 
> > This makes sense but I fear I'm going to need to add more stats that
> > require a
> >
> >> global view of tab loading when porting the startup tests, so I'd
> >>
> > prefer to
> >
> >> leave this in the browser in the meantime.
> >>
> >
> > Can you elaborate on what you think you'll need?
> >
> 
> The context for this CL is that it's test infrastructure needed to port our
> existing startup tests to Telemetry in order to make them more stable and
> move them off old infrastructure. This CL started life as a straight port
> of InitialLoadObserver as used in those tests.
> 
> One possible scenario where I'll need this to be in the browser is if we
> run into issues around terminating a test run e.g. we load N tabs and need
> to block until all tabs are loaded.  The current tests tell the
> InitialLoadObserver to block until a given number of tabs have loaded and
> then report stats back, it includes crashed tabs in this count. In
> Telemetry we want to block from the Python side.  If the new approach turns
> out to be flaky we may need to revert to the old method.

hmm, it seems that if there are flakiness, this could affect other tests no?
Seems like we would want to track down and fix any flakiness with telemetry.
> 
> Another scenario is if we need to collect stats as part of the test run
> that only the browser knows about (e.g. latency in launching the renderer
> process for a given tab).

Note there are ways of pushing this down, if you really end up needing it (i.e.
browser can send this stat to the renderer).

> 
> There's a distinct possibility that after rewriting the tests in Telemetry
> this won't be needed in which case moving this code to the renderer should
> be simple enough.  Currently though I'd prefer to have this flexibility.
> 
> I understand and share your concerns about code complexity, but personally
> I think this change is localized and uninvasive enough that getting a bit
> of flexibility in the early stages doesn't hurt much.
> 
> If you feel strongly about this, please say so and I'll move things to the
> renderer.  I'd rather have the additional flexibility the current code
> affords me [I'm also, quite frankly pretty sick of finessing this CL :) ]
> but I trust your judgement here.

Yes, please put it in the renderer. It's much better to add more code if it
turns out it's needed, than to remove code when it's not needed (the latter is
always too low-priority).

> 
> Best regards,
> Jeremy

[jeremy, 2013-05-27 09:57:02]

Browser code removed, ready for another look.

[jam, 2013-05-27 19:41:51]

lgtm

https://codereview.chromium.org/12389073/diff/130001/content/browser/histogra...
File content/browser/histogram_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/130001/content/browser/histogra...
content/browser/histogram_message_filter.cc:54:
switches::kStatsCollectionController);
since this switch is only used for tests, isn't
kReduceSecurityForStatsCollectionTests redundant?

https://codereview.chromium.org/12389073/diff/130001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/12389073/diff/130001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:856:
switches::kStatsCollectionController,
nit: please sort

https://codereview.chromium.org/12389073/diff/130001/content/public/common/bi...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/12389073/diff/130001/content/public/common/bi...
content/public/common/bindings_policy.h:23: // DOM automation bindings that
allows the JS content to retrieve a variety
nit: "automation bindings" seems inaccurate?

[commit-bot: I haz the power, 2013-05-28 15:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/12389073/146001

[commit-bot: I haz the power, 2013-05-28 19:11:43]

Change committed as 202620

[jam, 2013-05-28 20:58:02]

https://codereview.chromium.org/12389073/diff/130001/content/browser/histogra...
File content/browser/histogram_message_filter.cc (right):

https://codereview.chromium.org/12389073/diff/130001/content/browser/histogra...
content/browser/histogram_message_filter.cc:54:
switches::kStatsCollectionController);
On 2013/05/27 19:41:52, jam wrote:
> since this switch is only used for tests, isn't
> kReduceSecurityForStatsCollectionTests redundant?

what about this comment?

[tonyg, 2013-05-28 21:15:33]

This broke the reference perf tests. Example:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/bui...

Options are:
1. Revert this patch
2. Roll reference builds
3. Make telemetry files in this CL smart enough to pass the right flags and use
the right API on old builds or new builds.

[jeremy, 2013-05-28 21:15:46]

The reduced security flag is a carryover from existing code (there's
history behind it), I don't like it either but I'll send out a code review
tomorrow to remove it where we can discuss the pros and cons if any.


On Tue, May 28, 2013 at 11:58 PM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.**org/12389073/diff/130001/**
>
content/browser/histogram_**message_filter.cc<https://codereview.chromium.org/12389073/diff/130001/content/browser/histogram_message_filter.cc>
> File content/browser/histogram_**message_filter.cc (right):
>
> https://codereview.chromium.**org/12389073/diff/130001/**
>
content/browser/histogram_**message_filter.cc#newcode54<https://codereview.chromium.org/12389073/diff/130001/content/browser/histogram_message_filter.cc#newcode54>
> content/browser/histogram_**message_filter.cc:54:
> switches::**kStatsCollectionController);
> On 2013/05/27 19:41:52, jam wrote:
>
>> since this switch is only used for tests, isn't
>> kReduceSecurityForStatsCollect**ionTests redundant?
>>
>
> what about this comment?
>
>
https://codereview.chromium.**org/12389073/<https://codereview.chromium.org/1...
>

[jeremy, 2013-05-28 21:17:29]

Ouch, sorry :(

It's midnight here, but feel free to rollback and I'll try to resubmit
tomorrow with telemetry files that work across versions.


On Wed, May 29, 2013 at 12:15 AM, <tonyg@chromium.org> wrote:

> This broke the reference perf tests. Example:
> http://build.chromium.org/p/**chromium.perf/builders/Linux%**
>
20Perf%20%281%29/builds/24105/**steps/moz/logs/stdio<http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/builds/24105/steps/moz/logs/stdio>
>
> Options are:
> 1. Revert this patch
> 2. Roll reference builds
> 3. Make telemetry files in this CL smart enough to pass the right flags
> and use
> the right API on old builds or new builds.
>
>
https://codereview.chromium.**org/12389073/<https://codereview.chromium.org/1...
>

[ramant (doing other things), 2013-05-28 21:59:43]

Reverted this CL. thanks.

https://src.chromium.org/viewvc/chrome?revision=202662&view=revision

On 2013/05/28 21:17:29, jeremy wrote:
> Ouch, sorry :(
> 
> It's midnight here, but feel free to rollback and I'll try to resubmit
> tomorrow with telemetry files that work across versions.
> 
> 
> On Wed, May 29, 2013 at 12:15 AM, <mailto:tonyg@chromium.org> wrote:
> 
> > This broke the reference perf tests. Example:
> > http://build.chromium.org/p/**chromium.perf/builders/Linux%25**
> >
>
20Perf%20%281%29/builds/24105/**steps/moz/logs/stdio<http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/builds/24105/steps/moz/logs/stdio>
> >
> > Options are:
> > 1. Revert this patch
> > 2. Roll reference builds
> > 3. Make telemetry files in this CL smart enough to pass the right flags
> > and use
> > the right API on old builds or new builds.
> >
> >
>
https://codereview.chromium.**org/12389073/%3Chttps://codereview.chromium.org...>
> >

[jeremy, 2013-05-29 05:31:54]

tonyg: PTAL - should work with reference builds now.

[tonyg, 2013-05-29 15:38:59]

lgtm

https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/hi...
File tools/perf/perf_tools/histogram_metric.py (left):

https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/hi...
tools/perf/perf_tools/histogram_metric.py:40:
'window.domAutomationController.%s("%s") : ""' %
Looks like we lost the branch where neither domAutomationController nor
statsCollectionController is available. Is that important?

[jeremy, 2013-05-29 16:10:33]

I have no idea, as far as I can tell if we don't have them we should just
fail, wdyt?


On Wed, May 29, 2013 at 6:39 PM, <tonyg@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.**org/12389073/diff/171001/**
>
tools/perf/perf_tools/**histogram_metric.py<https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/histogram_metric.py>
> File tools/perf/perf_tools/**histogram_metric.py (left):
>
> https://codereview.chromium.**org/12389073/diff/171001/**
>
tools/perf/perf_tools/**histogram_metric.py#oldcode40<https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/histogram_metric.py#oldcode40>
> tools/perf/perf_tools/**histogram_metric.py:40:
> 'window.**domAutomationController.%s("%**s") : ""' %
> Looks like we lost the branch where neither domAutomationController nor
> statsCollectionController is available. Is that important?
>
>
https://codereview.chromium.**org/12389073/<https://codereview.chromium.org/1...
>

[tonyg, 2013-05-29 16:35:31]

On Wed, May 29, 2013 at 9:10 AM, Jeremy Moskovich <jeremy@chromium.org>wrote:

> I have no idea, as far as I can tell if we don't have them we should just
> fail, wdyt?
>
>
Let's try it.


>
> On Wed, May 29, 2013 at 6:39 PM, <tonyg@chromium.org> wrote:
>
>> lgtm
>>
>>
>>
>>
>> https://codereview.chromium.**org/12389073/diff/171001/**
>>
tools/perf/perf_tools/**histogram_metric.py<https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/histogram_metric.py>
>> File tools/perf/perf_tools/**histogram_metric.py (left):
>>
>> https://codereview.chromium.**org/12389073/diff/171001/**
>>
tools/perf/perf_tools/**histogram_metric.py#oldcode40<https://codereview.chromium.org/12389073/diff/171001/tools/perf/perf_tools/histogram_metric.py#oldcode40>
>> tools/perf/perf_tools/**histogram_metric.py:40:
>> 'window.**domAutomationController.%s("%**s") : ""' %
>> Looks like we lost the branch where neither domAutomationController nor
>> statsCollectionController is available. Is that important?
>>
>>
https://codereview.chromium.**org/12389073/<https://codereview.chromium.org/1...
>>
>
>

[commit-bot: I haz the power, 2013-05-29 16:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/12389073/171001

[commit-bot: I haz the power, 2013-05-29 20:44:35]

Change committed as 202955