Add stats collection for telemetry startup.warm.blank_page test.

Add stats collection for telemetry startup.warm.blank_page test.

This attempts to mirror similar functionality in Chrome.

Add StartupPerformance[Times|DataCollector] to mojo:metrics.
Record times from the shell, mojo:browser, and mojo:html_viewer.
Add the --enable-stats-collection-bindings flag for mandoline.

Add StatsCollectionController to mojo:html_viewer.
This registers and implements JS callbacks for perf testing.

TODO: Record more appropriate times; sanity check values!
TODO: Consolidate more code in startup_metric_utils.
TODO: Serve histogram JSON, not times struct, from mojo:tracing?
TODO: Refine mojo:tracing; combine with or use components/tracing?
TODO: Use base::TimeTicks deltas (crbug.com/521164)
TODO: Android support.

BUG=513779, 517622, 521164
TEST="./tools/perf/run_benchmark --browser=mandoline-debug startup.warm.blank_page --also-run-disabled-tests" runs okay.
R=yzshen@chromium.org,sky@chromium.org
TBR=gab@chromium.org

Committed: https://crrev.com/57daf7e60527375fd5a15fb30948b5be97bebae7
Cr-Commit-Position: refs/heads/master@{#343983}

[msw, 2015-08-12 21:51:15]

Patchset #10 (id:180001) has been deleted

[msw, 2015-08-12 22:32:00]

msw@chromium.org changed reviewers:
+ sky@chromium.org, yzshen@chromium.org

[msw, 2015-08-12 22:32:39]

Hey Yuzhu and Scott, please take a high-level look; thanks!

[sky, 2015-08-12 23:54:36]

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.cc:164: : html_document_(html_document) {
nit: make sure you run git cl format.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:76: class Observer {
I'm hoping to nuke HTMLDocument soon. Can you move this to HTMLDOcumentOOPIF?
You may want to push this to HTMLFrame. And move to its own header.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:97: void
AddObserver(HTMLDocument::Observer* observer);
Can these two be private since Observer takes care of it for you?

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_controller.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: static
StatsCollectionController* Install(HTMLDocument* html_document,
Can you make this take WebFrame, which would work with both HTMLDocument types?

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_observer.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_observer.h:29: base::Time start_time_;
Shouldn't this use timeticks?

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing.mojom:33: int64 shell_process_creation_time;
Again, I think all this should be timeticks, not times.

[yzshen1, 2015-08-13 15:59:12]

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/DEPS (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/DEPS:29: "+mojo/runner",
(Please see my comments in the other DEPS file.)

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:134: virtual void didStartLoading(bool
toDifferentDocument);
naming style: to_different_document, so it is consistent with the other methods.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:213: StatsCollectionController*
stats_collection_controller_;
Please comment about ownership and when it is null/non-null, etc.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_controller.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:193: run_loop.Run();
Maybe consider using InterfacePtr<>::WaitForIncomingResponse()?

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_controller.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: static
StatsCollectionController* Install(HTMLDocument* html_document,
Please comment about the lifespan and ownership of the returned value.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:57: StatsCollectionObserver
observer_;
Maybe consider merging the observer into this class?

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_observer.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_observer.h:25: const base::Time&
load_start_time() { return start_time_; }
Please use the same name for getter and the corresponding member variable.

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/DEPS
File mandoline/ui/browser/DEPS (right):

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/D...
mandoline/ui/browser/DEPS:7: "+mojo/runner",
It probably shouldn't depend on runner to gain access to the switch definition.

I think we should define the switch in
mojo/services/tracing/public/cpp/switches.{h,cc} instead.

(I did similar things for devtools_service.)

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/D...
mandoline/ui/browser/DEPS:9: "+mojo/services/tracing",
Maybe we should put the interface in a public/ subfolder?

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_manager.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_manager.cc:69: const base::Time startup_time =
base::Time::Now();
Why do we need to call base::Time::Now() three times here? I don't think the
differences between the three are meaningful here.

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/context.cc
File mojo/runner/context.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/context.cc...
mojo/runner/context.cc:321:
application_manager_.ConnectToService(GURL("mojo:tracing"), &controller);
Please consider directly obtaining a service provider from line 313, and ask for
a StartupPerformanceControllerPtr from it. That way we avoid the cost of
connecting to the application a second time.

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/switches.cc
File mojo/runner/switches.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/switches.c...
mojo/runner/switches.cc:47: const char kStatsCollectionController[] =
"enable-stats-collection-bindings";
nit: Variable name should match the value.

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing.mojom:43: interface StartupPerformanceController {
'Controller' seems a little confusing. Because Trace'Controller' above is an
interface implemented by clients of the tracing app.
StartupPerformance'Controller', on the other hand, is implemented by the tracing
app.

This interface is more like the TraceDataCollector above. Maybe it should be
called StartupPerformanceDataCollector?

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing_app.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing_app.cc:124:
callback.Run(startup_performance_times_.Clone().Pass());
nit: Pass() could be omitted in this case.

[msw, 2015-08-13 22:12:06]

+CC Sadrul; this may eventually relate to http://crbug.com/504456

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/DEPS (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/DEPS:29: "+mojo/runner",
On 2015/08/13 15:59:11, yzshen1 wrote:
> (Please see my comments in the other DEPS file.)

Done.

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/DEPS
File mandoline/ui/browser/DEPS (right):

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/D...
mandoline/ui/browser/DEPS:7: "+mojo/runner",
On 2015/08/13 15:59:12, yzshen1 wrote:
> It probably shouldn't depend on runner to gain access to the switch
definition.
> 
> I think we should define the switch in
> mojo/services/tracing/public/cpp/switches.{h,cc} instead.
> 
> (I did similar things for devtools_service.)

Done.

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/D...
mandoline/ui/browser/DEPS:9: "+mojo/services/tracing",
On 2015/08/13 15:59:12, yzshen1 wrote:
> Maybe we should put the interface in a public/ subfolder?

Done.

[msw, 2015-08-13 22:12:34]

(not ready for another review yet, sorry...)

[msw, 2015-08-14 23:20:02]

Patchset #12 (id:240001) has been deleted

[msw, 2015-08-14 23:20:35]

Comments addressed, please take another look; thanks!
Please lmk if you have thoughts about the many TODOs.
(sorry for mixing a rebase in with the new patch set)

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.cc:164: : html_document_(html_document) {
On 2015/08/12 23:54:36, sky wrote:
> nit: make sure you run git cl format.

Done.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/html_document.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:76: class Observer {
On 2015/08/12 23:54:36, sky wrote:
> I'm hoping to nuke HTMLDocument soon. Can you move this to HTMLDOcumentOOPIF?
> You may want to push this to HTMLFrame. And move to its own header.

Nothing actually uses StatsCollectionController::GetTabLoadTiming (the only
reason this observation was needed), so it's removed.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:97: void
AddObserver(HTMLDocument::Observer* observer);
On 2015/08/12 23:54:36, sky wrote:
> Can these two be private since Observer takes care of it for you?

Removed.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:134: virtual void didStartLoading(bool
toDifferentDocument);
On 2015/08/13 15:59:11, yzshen1 wrote:
> naming style: to_different_document, so it is consistent with the other
methods.

Removed.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/html_document.h:213: StatsCollectionController*
stats_collection_controller_;
On 2015/08/13 15:59:11, yzshen1 wrote:
> Please comment about ownership and when it is null/non-null, etc.

Hmm, holding gin::Wrappable objects on the heap isn't supported:
https://code.google.com/p/chromium/codesearch#chromium/src/gin/handle.h&l=12
So I've replaced this with a StartupPerformanceDataCollectorPtr.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_controller.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:193: run_loop.Run();
On 2015/08/13 15:59:12, yzshen1 wrote:
> Maybe consider using InterfacePtr<>::WaitForIncomingResponse()?

Done.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_controller.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: static
StatsCollectionController* Install(HTMLDocument* html_document,
On 2015/08/13 15:59:12, yzshen1 wrote:
> Please comment about the lifespan and ownership of the returned value.

Done.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: static
StatsCollectionController* Install(HTMLDocument* html_document,
On 2015/08/12 23:54:36, sky wrote:
> Can you make this take WebFrame, which would work with both HTMLDocument
types?

Done.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:57: StatsCollectionObserver
observer_;
On 2015/08/13 15:59:12, yzshen1 wrote:
> Maybe consider merging the observer into this class?

Removed.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
File components/html_viewer/stats_collection_observer.h (right):

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_observer.h:25: const base::Time&
load_start_time() { return start_time_; }
On 2015/08/13 15:59:12, yzshen1 wrote:
> Please use the same name for getter and the corresponding member variable.

Removed.

https://codereview.chromium.org/1278673002/diff/220001/components/html_viewer...
components/html_viewer/stats_collection_observer.h:29: base::Time start_time_;
On 2015/08/12 23:54:36, sky wrote:
> Shouldn't this use timeticks?

Removed.

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_manager.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_manager.cc:69: const base::Time startup_time =
base::Time::Now();
On 2015/08/13 15:59:12, yzshen1 wrote:
> Why do we need to call base::Time::Now() three times here? I don't think the
> differences between the three are meaningful here.

This is just a placeholder with TODOs. I'm not entirely sure when/where these
various times should actually be recorded... I can try to figure that out in
this CL, or resolve these TODOs in a follow-up. The repeated Now() calls won't
outlive the TODOs, but I can re-use a single time and leave the TODOS... WDYT?

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/context.cc
File mojo/runner/context.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/context.cc...
mojo/runner/context.cc:321:
application_manager_.ConnectToService(GURL("mojo:tracing"), &controller);
On 2015/08/13 15:59:12, yzshen1 wrote:
> Please consider directly obtaining a service provider from line 313, and ask
for
> a StartupPerformanceControllerPtr from it. That way we avoid the cost of
> connecting to the application a second time.

Done.

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/switches.cc
File mojo/runner/switches.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/runner/switches.c...
mojo/runner/switches.cc:47: const char kStatsCollectionController[] =
"enable-stats-collection-bindings";
On 2015/08/13 15:59:12, yzshen1 wrote:
> nit: Variable name should match the value.

Done.

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing.mojom:33: int64 shell_process_creation_time;
On 2015/08/12 23:54:36, sky wrote:
> Again, I think all this should be timeticks, not times.

I filed http://crbug.com/521164 for this and added TODOs.

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing.mojom:43: interface StartupPerformanceController {
On 2015/08/13 15:59:12, yzshen1 wrote:
> 'Controller' seems a little confusing. Because Trace'Controller' above is an
> interface implemented by clients of the tracing app.
> StartupPerformance'Controller', on the other hand, is implemented by the
tracing
> app.
> 
> This interface is more like the TraceDataCollector above. Maybe it should be
> called StartupPerformanceDataCollector?
> 

I went with StartupPerformanceDataCollector, but it's a long name; other
suggestions are welcome.

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing_app.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing_app.cc:124:
callback.Run(startup_performance_times_.Clone().Pass());
On 2015/08/13 15:59:12, yzshen1 wrote:
> nit: Pass() could be omitted in this case.

Done.

[yzshen1, 2015-08-17 05:46:39]

Mostly looks good. Thanks!

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_manager.cc (right):

https://codereview.chromium.org/1278673002/diff/220001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_manager.cc:69: const base::Time startup_time =
base::Time::Now();
On 2015/08/14 23:20:35, msw wrote:
> On 2015/08/13 15:59:12, yzshen1 wrote:
> > Why do we need to call base::Time::Now() three times here? I don't think the
> > differences between the three are meaningful here.
> 
> This is just a placeholder with TODOs. I'm not entirely sure when/where these
> various times should actually be recorded... I can try to figure that out in
> this CL, or resolve these TODOs in a follow-up. The repeated Now() calls won't
> outlive the TODOs, but I can re-use a single time and leave the TODOS... WDYT?

SGTM. Thanks!

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
File mojo/services/tracing/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/220001/mojo/services/tracing/...
mojo/services/tracing/tracing.mojom:43: interface StartupPerformanceController {
On 2015/08/14 23:20:35, msw wrote:
> On 2015/08/13 15:59:12, yzshen1 wrote:
> > 'Controller' seems a little confusing. Because Trace'Controller' above is an
> > interface implemented by clients of the tracing app.
> > StartupPerformance'Controller', on the other hand, is implemented by the
> tracing
> > app.
> > 
> > This interface is more like the TraceDataCollector above. Maybe it should be
> > called StartupPerformanceDataCollector?
> > 
> 
> I went with StartupPerformanceDataCollector, but it's a long name; other
> suggestions are welcome.

(Optional) One idea is "Performance" -> "Perf". So it is
StartupPerfDataCollector.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document.cc:406: if
(startup_performance_data_collector_.get()) {
nit: ".get()" is not necessary for null-check. (I think it is preferred by our
style guide that way?)

Besides, although it is not a concern here, in some cases it affects performance
too: 
Using ".get()" forces the InterfacePtr to setup router and handler watching for
the message pipe handle, if it is not set up yet. On the other hand, using "if
(startup_performance_data_collector_)" only checks whether it holds a valid
message pipe handle.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document.cc:407:
startup_performance_data_collector_->SetFirstWebContentsMainFrameLoadTime(
Do you think we should comment on why we call this for every onload event? (And
similarly for line 431)

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_document_oopif.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document_oopif.cc:135: global_state_, view,
resource_waiter_.Pass(), this);
Is it included in this patch unintentionally?

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:759: if
(startup_performance_data_collector_.get()) {
nit: .get() is not necessary.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/stats_collection_controller.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:89: return
tracing::StartupPerformanceDataCollectorPtr();
nit, optional: it is still compilable to return nullptr, which seems easier to
read.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:111:
StatsCollectionController* stats_collection_controller =
(Out of curiosity) When is this instance destructed?

[sky, 2015-08-17 20:09:30]

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:200: startup_performance_data_collector_ =
Do we really need this for all frames? Should it only be on the top most frame,
and only if that frame is local? As an example of why it matters
didHandleOnloadEvents won't be called for remote frames.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/stats_collection_controller.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:133:
.SetMethod("getHistogram", &StatsCollectionController::GetHistogram)
What are the life of these bindings? The same as that of the HTMLFrame? Or the
life of the local root HTMLFrame?

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/stats_collection_controller.h (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: // This bails and
returns an invalid pointer without the command line flag.
'invalid pointer' makes it sounds like the pointer is bad. I think you mean null
or empty ptr.

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
File mojo/services/tracing/public/interfaces/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
mojo/services/tracing/public/interfaces/tracing.mojom:27: StopAndFlush();
How do you know when the final DataCollected() is called?

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
mojo/services/tracing/public/interfaces/tracing.mojom:44: interface
StartupPerformanceDataCollector {
Expand your documentation above to cover this.
It's also not clear why there are a bunch of setters. Could a single Done() with
a struct be used?

[msw, 2015-08-17 21:07:36]

Patchset #15 (id:320001) has been deleted

[msw, 2015-08-17 21:25:51]

Please take another look; I'll address TODOs in the meantime.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document.cc:406: if
(startup_performance_data_collector_.get()) {
On 2015/08/17 05:46:38, yzshen1 wrote:
> nit: ".get()" is not necessary for null-check. (I think it is preferred by our
> style guide that way?)
> 
> Besides, although it is not a concern here, in some cases it affects
performance
> too: 
> Using ".get()" forces the InterfacePtr to setup router and handler watching
for
> the message pipe handle, if it is not set up yet. On the other hand, using "if
> (startup_performance_data_collector_)" only checks whether it holds a valid
> message pipe handle.

Done; thanks for the extra info.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document.cc:407:
startup_performance_data_collector_->SetFirstWebContentsMainFrameLoadTime(
On 2015/08/17 05:46:38, yzshen1 wrote:
> Do you think we should comment on why we call this for every onload event?
(And
> similarly for line 431)

The mojom explains that only the first time is recorded, but in the interest of
performance I added static flags in these functions.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_document_oopif.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_document_oopif.cc:135: global_state_, view,
resource_waiter_.Pass(), this);
On 2015/08/17 05:46:38, yzshen1 wrote:
> Is it included in this patch unintentionally?

No, this param is unused, so I'm removing it. I was going to use it at first,
then I realized it was redundant. |this| is passed as the HTMLFrameDelegate arg,
and |html_document_app_| is returned in the  HTMLFrameDelegate::GetApp override.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:200: startup_performance_data_collector_ =
On 2015/08/17 20:09:30, sky wrote:
> Do we really need this for all frames? Should it only be on the top most
frame,
> and only if that frame is local? As an example of why it matters
> didHandleOnloadEvents won't be called for remote frames.

Done; thanks for the advice!

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:759: if
(startup_performance_data_collector_.get()) {
On 2015/08/17 05:46:38, yzshen1 wrote:
> nit: .get() is not necessary.

Done.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/stats_collection_controller.cc (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:89: return
tracing::StartupPerformanceDataCollectorPtr();
On 2015/08/17 05:46:38, yzshen1 wrote:
> nit, optional: it is still compilable to return nullptr, which seems easier to
> read.

Done; thanks for the tip!

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:111:
StatsCollectionController* stats_collection_controller =
On 2015/08/17 05:46:38, yzshen1 wrote:
> (Out of curiosity) When is this instance destructed?

gin's Handle/Wrappable code and documentation isn't clear about the destruction
time frame. The best hint they give is in wrappable.h: "If clients fail to
create a wrapper for a wrappable object, the object will leak because we use the
weak callback from the wrapper as the signal to delete the wrapped object.". The
object lifetime appears to be the script context lifetime, although some objects
can be destroyed earlier on memory pressure (see gin_browsertest.cc). The
instance ownership is definitely passed to the gin::Handle; almost all other
calls to gin::CreateHandle pass a "new Foo()", including the content-side
StatsCollectionController:
https://code.google.com/p/chromium/codesearch#search/&q=gin::CreateHandle

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.cc:133:
.SetMethod("getHistogram", &StatsCollectionController::GetHistogram)
On 2015/08/17 20:09:30, sky wrote:
> What are the life of these bindings? The same as that of the HTMLFrame? Or the
> life of the local root HTMLFrame?

See my comment above, I'd guess that the lifetime of the script context would
roughly match that of the HTMLFrame, but wrapped objects might be destroyed
earlier on memory pressure.

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
File components/html_viewer/stats_collection_controller.h (right):

https://codereview.chromium.org/1278673002/diff/280001/components/html_viewer...
components/html_viewer/stats_collection_controller.h:32: // This bails and
returns an invalid pointer without the command line flag.
On 2015/08/17 20:09:30, sky wrote:
> 'invalid pointer' makes it sounds like the pointer is bad. I think you mean
null
> or empty ptr.

Done.

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
File mojo/services/tracing/public/interfaces/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
mojo/services/tracing/public/interfaces/tracing.mojom:27: StopAndFlush();
On 2015/08/17 20:09:30, sky wrote:
> How do you know when the final DataCollected() is called?

I just added the struct and interface below and moved this file from
mojo/services/tracing/. I don't know much about the existing (unchanged)
interfaces.

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
mojo/services/tracing/public/interfaces/tracing.mojom:44: interface
StartupPerformanceDataCollector {
On 2015/08/17 20:09:30, sky wrote:
> Expand your documentation above to cover this.
> It's also not clear why there are a bunch of setters. Could a single Done()
with
> a struct be used?

I added minimal documentation, let me know if it's okay. A single setter
wouldn't work, as we need data from the shell (via context.cc), mojo:browser,
and mojo:html_viewer. I considered using a single setter function that took a
string and a value, eg. SetTracingTime("ShellProcessCreation", time), and then
the collector could just return a map of string->time... would you prefer that?

[sky, 2015-08-17 21:41:57]

LGTM

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
File mojo/services/tracing/public/interfaces/tracing.mojom (right):

https://codereview.chromium.org/1278673002/diff/280001/mojo/services/tracing/...
mojo/services/tracing/public/interfaces/tracing.mojom:44: interface
StartupPerformanceDataCollector {
On 2015/08/17 21:25:51, msw wrote:
> On 2015/08/17 20:09:30, sky wrote:
> > Expand your documentation above to cover this.
> > It's also not clear why there are a bunch of setters. Could a single Done()
> with
> > a struct be used?
> 
> I added minimal documentation, let me know if it's okay. A single setter
> wouldn't work, as we need data from the shell (via context.cc), mojo:browser,
> and mojo:html_viewer. I considered using a single setter function that took a
> string and a value, eg. SetTracingTime("ShellProcessCreation", time), and then
> the collector could just return a map of string->time... would you prefer
that?

No, that's too obscure. If you can't do the single setter than I prefer what you
have here.

[yzshen1, 2015-08-17 21:45:52]

LGTM

https://codereview.chromium.org/1278673002/diff/340001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1278673002/diff/340001/components/html_viewer...
components/html_viewer/html_frame.cc:167: startup_performance_data_collector_ =
Please consider commenting on why it is only installed in this case.

[msw, 2015-08-17 22:07:06]

msw@chromium.org changed reviewers:
+ gab@chromium.org

[msw, 2015-08-17 22:08:04]

+TBR gab@ for components/startup_metric_utils

https://codereview.chromium.org/1278673002/diff/340001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1278673002/diff/340001/components/html_viewer...
components/html_viewer/html_frame.cc:167: startup_performance_data_collector_ =
On 2015/08/17 21:45:52, yzshen1 wrote:
> Please consider commenting on why it is only installed in this case.

Done.

[msw, 2015-08-17 22:08:11]

The CQ bit was checked by msw@chromium.org

[msw, 2015-08-17 22:08:11]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1278673002/#ps360001
(title: "Address comment and minor TODO; remove some TODOs.")

[commit-bot: I haz the power, 2015-08-17 22:08:29]

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

[commit-bot: I haz the power, 2015-08-17 22:31:26]

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

[commit-bot: I haz the power, 2015-08-17 22:31:27]

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

[msw, 2015-08-17 22:54:03]

The CQ bit was checked by msw@chromium.org

[msw, 2015-08-17 22:54:04]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1278673002/#ps380001
(title: "Fix android build; TODO: Android startup perf testing.")

[commit-bot: I haz the power, 2015-08-17 22:54:36]

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

[commit-bot: I haz the power, 2015-08-18 01:25:45]

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

[commit-bot: I haz the power, 2015-08-18 01:25:46]

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

[gab, 2015-08-18 01:35:00]

components/startup_metric_utils lgtm % comment nit

https://codereview.chromium.org/1278673002/diff/380001/components/startup_met...
File components/startup_metric_utils/startup_metric_utils.h (right):

https://codereview.chromium.org/1278673002/diff/380001/components/startup_met...
components/startup_metric_utils/startup_metric_utils.h:38: // Call this with a
previously recorded timestamp if unable to call on startup.
"if unable to call" what "on startup" ?

"if unable to call RecordMainEntryPointTime() directly on startup" ?

[msw, 2015-08-18 17:59:48]

https://codereview.chromium.org/1278673002/diff/380001/components/startup_met...
File components/startup_metric_utils/startup_metric_utils.h (right):

https://codereview.chromium.org/1278673002/diff/380001/components/startup_met...
components/startup_metric_utils/startup_metric_utils.h:38: // Call this with a
previously recorded timestamp if unable to call on startup.
On 2015/08/18 01:35:00, gab wrote:
> "if unable to call" what "on startup" ?
> 
> "if unable to call RecordMainEntryPointTime() directly on startup" ?

Done.

[msw, 2015-08-18 17:59:55]

The CQ bit was checked by msw@chromium.org

[msw, 2015-08-18 17:59:55]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
yzshen@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1278673002/#ps400001
(title: "Update comment.")

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

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

[commit-bot: I haz the power, 2015-08-18 18:55:39]

Committed patchset #18 (id:400001)

[commit-bot: I haz the power, 2015-08-18 18:56:14]

Patchset 18 (id:??) landed as
https://crrev.com/57daf7e60527375fd5a15fb30948b5be97bebae7
Cr-Commit-Position: refs/heads/master@{#343983}