Add UMA metric: Startup.BrowserMainToRendererMain.

Add UMA metric: Startup.BrowserMainToRendererMain.

This new metric keeps track of the time from BrowserMain() to
RendererMain().

Time is recorded by renderer processes when they enter RendererMain().
This time is sent to the browser through an IPC. The browser records the
first renderer main entry time that it receives and ignores the others.
When the first non-empty paint occurs, the browser computes
Startup.BrowserMainToRendererMain
(First Renderer Main Entry Time - Browser Main Entry Time) and logs it.

We need a renderer->browser IPC because the startup temperature is only
known in the browser (we cannot pass this information with a cmd-line
flag because it is not known yet when the first renderer is launched).

We need to wait until the first non-empty paint to log the metric,
because we don't have any guarantee that both the startup temperature
and the first renderer main entry time are known before that.

BUG=547794
Committed: https://crrev.com/443bd11bc8106bea98a89819b7bb1b35d6e5ddf8
Cr-Commit-Position: refs/heads/master@{#359904}

[fdoray, 2015-11-06 07:20:20]

fdoray@chromium.org changed reviewers:
+ asvitkine@chromium.org, ben@chromium.org, gab@chromium.org

[fdoray, 2015-11-06 07:20:21]

asvitkine@chromium.org: Please review changes in
tools/metrics/histograms/histograms.xml

ben@chromium.org: Please review changes in chrome/, components/BUILD.gn,
components/html_viewer, content/

gab@chromium.org: Please review changes in components/startup_metric_utils

[Alexei Svitkine (slow), 2015-11-06 16:21:26]

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44531: +    Time from the main()
function in chrome.dll to chrome_child.dll's main().
Is it logged for every renderer or for the first one only?

Is Windows specific, given you mention dlls, or not? Please expand description
to clarify these things.

[Alexei Svitkine (slow), 2015-11-06 16:23:08]

"The browser records the
first renderer main entry time that it receives and ignores the others."

It seems unfortunate to have to pay the price of an extra IPC per renderer start
if we're going to be throwing away most of them. Perhaps once browser knows it's
received one such IPC, when it could start renderers in a way that tell them not
to send this?

[gab, 2015-11-06 17:41:59]

On 2015/11/06 16:23:08, Alexei Svitkine (slow) wrote:
> "The browser records the
> first renderer main entry time that it receives and ignores the others."
> 
> It seems unfortunate to have to pay the price of an extra IPC per renderer
start
> if we're going to be throwing away most of them. Perhaps once browser knows
it's
> received one such IPC, when it could start renderers in a way that tell them
not
> to send this?

Feels like the overhead of a command-line flag (simplest way I can think of for
the browser to let the renderer know without IPC) is worse and more code
complexity then redundant IPCs and that one more per renderer is irrelevant (I
assume IPCs are very cheap given the number of IPCs constantly involved between
the renderers and the browser).

[gab, 2015-11-06 18:05:17]

lg

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils.gypi (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils.gypi:12: 'target_name':
'startup_metric_utils_browser_utils',
I prefer keeping "startup_metric_utils_browser", adding "_utils" sounds
overloaded (or is there precedence for this nomenclature?).

(I'm okay with the _message_filter target though since that's a specialization
of the former target)

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_message_filter.h
(right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_message_filter.h:17:
StartupMetricMessageFilter();
I think the style now allows to use 
StartupMetricMessageFilter() = default;
inline here (same for destructor).

(the rule being that inline constructor/destructor are allowed IFF your class
has no non-POD members)

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.cc:441: if
(!g_renderer_main_entry_point_time.Get().is_null())
Shouldn't this be a positive check for is_null()? (as-is doesn't this only
overwrite it when it's not null -- i.e., never if it's never set elsewhere?)

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.cc:478: //
temperature, so log Startup.BrowserDLLMainToRendererDLLMain.
Log Startup.BrowserDLLMainToRendererDLLMain now that the first renderer main
entry time and the startup temperature are known.

(i.e. avoid "we" and "so")

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_utils.h (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.h:74: // function
don't do anything.
s/don't do anything/are ignored/

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/common/startup_metric_messages.h (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/common/startup_metric_messages.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
You'll also need an OWNERS file to add the typical IPC owners to these
_messages.h files I think.

https://codereview.chromium.org/1413533008/diff/1/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/1413533008/diff/1/content/renderer/renderer_m...
content/renderer/renderer_main.cc:209:
renderer_main_entry_time.ToInternalValue()));
I would think the IPC layer already knows how to serialize base::Time, have you
tried passing the base::Time object as-is?

[gab, 2015-11-06 18:06:26]

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44531: +    Time from the main()
function in chrome.dll to chrome_child.dll's main().
On 2015/11/06 16:21:26, Alexei Svitkine (slow) wrote:
> Is it logged for every renderer or for the first one only?
> 
> Is Windows specific, given you mention dlls, or not? Please expand description
> to clarify these things.

Good point, @fdoray: isn't this just BrowserMainToRendererMain (i.e. on all
platforms)?

[fdoray, 2015-11-09 14:27:28]

Description was changed from

==========
Add UMA metric: Startup.BrowserDLLMainToRendererDLLMain.

This new metric keeps track of the time from the main() function in
chrome.dll to chrome_child.dll's main().

Time is recorded by renderer processes when they enter RendererMain().
This time is sent to the browser through an IPC. The browser records the
first renderer main entry time that it receives and ignores the others.
When the first non-empty paint occurs, the browser computes
Startup.BrowserDLLMainToRendererDLLMain
(First Renderer Main Entry Time - Browser Main Entry Time) and logs it.

We need a renderer->browser IPC because the startup temperature is only
known in the browser (we cannot pass this information with a cmd-line
flag because it is not known yet when the first renderer is launched).

We need to wait until the first non-empty paint to log the metric,
because we don't have any guarantee that both the startup temperature
and the first renderer main entry time are known before that.

BUG=547794
==========

to

==========
Add UMA metric: Startup.BrowserMainToRendererMain.

This new metric keeps track of the time from BrowserMain() to
RendererMain().

Time is recorded by renderer processes when they enter RendererMain().
This time is sent to the browser through an IPC. The browser records the
first renderer main entry time that it receives and ignores the others.
When the first non-empty paint occurs, the browser computes
Startup.BrowserMainToRendererMain
(First Renderer Main Entry Time - Browser Main Entry Time) and logs it.

We need a renderer->browser IPC because the startup temperature is only
known in the browser (we cannot pass this information with a cmd-line
flag because it is not known yet when the first renderer is launched).

We need to wait until the first non-empty paint to log the metric,
because we don't have any guarantee that both the startup temperature
and the first renderer main entry time are known before that.

BUG=547794
==========

[fdoray, 2015-11-09 15:53:56]

gab@ and asvitkine@: I addressed all your comments.

asvitkine@chromium.org: Could you take another look at
tools/metrics/histograms/histograms.xml

ben@chromium.org: Please review changes in chrome/, components/BUILD.gn,
components/html_viewer, content/

gab@chromium.org: Could you take another look at components/startup_metric_utils

agl@chromium.org: Please review changes in ipc/ipc_message_start.h and
components/startup_metric_utils/common/startup_metric_messages.h.

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils.gypi (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils.gypi:12: 'target_name':
'startup_metric_utils_browser_utils',
On 2015/11/06 18:05:17, gab wrote:
> I prefer keeping "startup_metric_utils_browser", adding "_utils" sounds
> overloaded (or is there precedence for this nomenclature?).
> 
> (I'm okay with the _message_filter target though since that's a specialization
> of the former target)

Done.

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_message_filter.h
(right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_message_filter.h:17:
StartupMetricMessageFilter();
On 2015/11/06 18:05:17, gab wrote:
> I think the style now allows to use 
> StartupMetricMessageFilter() = default;
> inline here (same for destructor).
> 
> (the rule being that inline constructor/destructor are allowed IFF your class
> has no non-POD members)

Done, for the destructor. The constructor cannot be default (has to pass
StartupMetricMsgStart to the base class constructor).

Note that the base class has non-POD members. Is it still OK to use =default in
that case?

The style guide
https://google.github.io/styleguide/cppguide.html#Inline_Functions says "Beware
of destructors, which are often longer than they appear because of implicit
member- and base-destructor calls!"

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.cc:441: if
(!g_renderer_main_entry_point_time.Get().is_null())
On 2015/11/06 18:05:17, gab wrote:
> Shouldn't this be a positive check for is_null()? (as-is doesn't this only
> overwrite it when it's not null -- i.e., never if it's never set elsewhere?)

Done.

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.cc:478: //
temperature, so log Startup.BrowserDLLMainToRendererDLLMain.
On 2015/11/06 18:05:17, gab wrote:
> Log Startup.BrowserDLLMainToRendererDLLMain now that the first renderer main
> entry time and the startup temperature are known.
> 
> (i.e. avoid "we" and "so")

Done.

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_utils.h (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.h:74: // function
don't do anything.
On 2015/11/06 18:05:17, gab wrote:
> s/don't do anything/are ignored/

Done.

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/common/startup_metric_messages.h (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/common/startup_metric_messages.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/11/06 18:05:17, gab wrote:
> You'll also need an OWNERS file to add the typical IPC owners to these
> _messages.h files I think.

Acknowledged.

https://codereview.chromium.org/1413533008/diff/1/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/1413533008/diff/1/content/renderer/renderer_m...
content/renderer/renderer_main.cc:209:
renderer_main_entry_time.ToInternalValue()));
On 2015/11/06 18:05:17, gab wrote:
> I would think the IPC layer already knows how to serialize base::Time, have
you
> tried passing the base::Time object as-is?

Done.

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1413533008/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44531: +    Time from the main()
function in chrome.dll to chrome_child.dll's main().
On 2015/11/06 18:06:26, gab wrote:
> On 2015/11/06 16:21:26, Alexei Svitkine (slow) wrote:
> > Is it logged for every renderer or for the first one only?
> > 
> > Is Windows specific, given you mention dlls, or not? Please expand
description
> > to clarify these things.
> 
> Good point, @fdoray: isn't this just BrowserMainToRendererMain (i.e. on all
> platforms)?

Done.

[fdoray, 2015-11-09 15:54:55]

fdoray@chromium.org changed reviewers:
+ agl@chromium.org

[fdoray, 2015-11-09 15:54:56]

gab@ and asvitkine@: I addressed all your comments.

asvitkine@chromium.org: Could you take another look at
tools/metrics/histograms/histograms.xml

ben@chromium.org: Please review changes in chrome/, components/BUILD.gn,
components/html_viewer, content/

gab@chromium.org: Could you take another look at components/startup_metric_utils

+agl@chromium.org: Please review changes in ipc/ipc_message_start.h and
components/startup_metric_utils/common/startup_metric_messages.h.

[gab, 2015-11-09 16:01:29]

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_message_filter.h
(right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_message_filter.h:17:
StartupMetricMessageFilter();
On 2015/11/09 15:53:56, fdoray wrote:
> On 2015/11/06 18:05:17, gab wrote:
> > I think the style now allows to use 
> > StartupMetricMessageFilter() = default;
> > inline here (same for destructor).
> > 
> > (the rule being that inline constructor/destructor are allowed IFF your
class
> > has no non-POD members)
> 
> Done, for the destructor. The constructor cannot be default (has to pass
> StartupMetricMsgStart to the base class constructor).
> 
> Note that the base class has non-POD members. Is it still OK to use =default
in
> that case?
> 
> The style guide
> https://google.github.io/styleguide/cppguide.html#Inline_Functions says
"Beware
> of destructors, which are often longer than they appear because of implicit
> member- and base-destructor calls!"

Yes, essentially inlining this class' destructor will end up inlining the base
class' destructor in the caller (which itself will be a jump to the impl of that
destructor if it turns out to not be inlinable).

[Alexei Svitkine (slow), 2015-11-09 16:06:04]

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:305:
UMA_HISTOGRAM_AND_TRACE_WITH_STARTUP_TEMPERATURE(
Given that you're using this macro which adds extra suffixes, you need to add an
affected-histogram entry to <histogram_suffixes name="StartupTemperature"
separator="."> in the xml.

[gab, 2015-11-09 16:21:36]

lgtm w/ comments

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
File components/startup_metric_utils/common/startup_metric_messages.h (right):

https://codereview.chromium.org/1413533008/diff/1/components/startup_metric_u...
components/startup_metric_utils/common/startup_metric_messages.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/11/09 15:53:56, fdoray wrote:
> On 2015/11/06 18:05:17, gab wrote:
> > You'll also need an OWNERS file to add the typical IPC owners to these
> > _messages.h files I think.
> 
> Acknowledged.

I meant that you'll need to add that OWNERS file yourself as part of this CL
(i.e. not just get an OWNERS review).

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:305:
UMA_HISTOGRAM_AND_TRACE_WITH_STARTUP_TEMPERATURE(
On 2015/11/09 16:06:03, Alexei Svitkine (slow) wrote:
> Given that you're using this macro which adds extra suffixes, you need to add
an
> affected-histogram entry to <histogram_suffixes name="StartupTemperature"
> separator="."> in the xml.

Good catch, @fdoray: can you also add a comment to that effect on the
*_STARTUP_TEMPERATURE macros.

https://codereview.chromium.org/1413533008/diff/40001/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/1413533008/diff/40001/content/renderer/render...
content/renderer/renderer_main.cc:87: base::Time renderer_main_entry_time =
base::Time::Now();
const

[fdoray, 2015-11-09 17:10:16]

fdoray@chromium.org changed reviewers:
+ tsepez@chromium.org
- agl@chromium.org

[fdoray, 2015-11-09 17:10:17]

gab@ and asvitkine@: all done

-agl@
+tsepez@: Could you review ipc/ipc_message_start.h and
components/startup_metric_utils/common/startup_metric_messages.h?

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/1413533008/diff/40001/components/startup_metr...
components/startup_metric_utils/browser/startup_metric_utils.cc:305:
UMA_HISTOGRAM_AND_TRACE_WITH_STARTUP_TEMPERATURE(
On 2015/11/09 16:21:36, gab wrote:
> On 2015/11/09 16:06:03, Alexei Svitkine (slow) wrote:
> > Given that you're using this macro which adds extra suffixes, you need to
add
> an
> > affected-histogram entry to <histogram_suffixes name="StartupTemperature"
> > separator="."> in the xml.
> 
> Good catch, @fdoray: can you also add a comment to that effect on the
> *_STARTUP_TEMPERATURE macros.

Done.

https://codereview.chromium.org/1413533008/diff/40001/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/1413533008/diff/40001/content/renderer/render...
content/renderer/renderer_main.cc:87: base::Time renderer_main_entry_time =
base::Time::Now();
On 2015/11/09 16:21:36, gab wrote:
> const

Done.

[Alexei Svitkine (slow), 2015-11-09 17:32:59]

histograms lgtm

[fdoray, 2015-11-09 17:51:05]

ben@chromium.org: Please review changes in chrome/, components/BUILD.gn,
components/html_viewer, content/

agl@chromium.org: Please review changes in ipc/ipc_message_start.h and
components/startup_metric_utils/common/startup_metric_messages.h.

[Tom Sepez, 2015-11-09 18:09:17]

Messages LGTM.

[agl, 2015-11-09 19:17:45]

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

[agl, 2015-11-09 19:17:45]

LGTM for ipc/.

[fdoray, 2015-11-10 21:27:15]

ben@chromium.org: Please review changes in chrome/, components/BUILD.gn,
components/html_viewer, content/

[Ben Goodger (Google), 2015-11-13 20:07:37]

https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
File components/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
components/html_viewer/BUILD.gn:164:
"//components/startup_metric_utils/browser:browser",
why is this necessary?

[fdoray, 2015-11-13 20:25:22]

ben@: Could you take another look?

https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
File components/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
components/html_viewer/BUILD.gn:164:
"//components/startup_metric_utils/browser:browser",
On 2015/11/13 20:07:37, Ben Goodger (Google) wrote:
> why is this necessary?

html_viewer builds correctly without ":browser". However, since there is now 2
targets defined in components/startup_metric_utils/browser/BUILD.gn, I felt that
it was less ambiguous that way.

Would you prefer that I replace all "/browser:browser" by just "/browser"?

Note: html_viewer cannot depend on
//components/startup_metric_utils/browser:message_filter because of this error:

entrypoints.obj : error LNK2005: MojoBeginReadData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoBeginWriteData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoClose already defined in system_thunks.obj
entrypoints.obj : error LNK2005: MojoCreateDataPipe already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoCreateMessagePipe already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoCreateSharedBuffer already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoDuplicateBufferHandle already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoEndReadData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoEndWriteData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoGetTimeTicksNow already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoMapBuffer already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoReadData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoReadMessage already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoUnmapBuffer already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoWait already defined in system_thunks.obj
entrypoints.obj : error LNK2005: MojoWaitMany already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoWriteData already defined in
system_thunks.obj
entrypoints.obj : error LNK2005: MojoWriteMessage already defined in
system_thunks.obj

[Ben Goodger (Google), 2015-11-13 22:56:33]

On 2015/11/13 20:25:22, fdoray wrote:
> ben@: Could you take another look?
> 
>
https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
> File components/html_viewer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
> components/html_viewer/BUILD.gn:164:
> "//components/startup_metric_utils/browser:browser",
> On 2015/11/13 20:07:37, Ben Goodger (Google) wrote:
> > why is this necessary?
> 
> html_viewer builds correctly without ":browser". However, since there is now 2
> targets defined in components/startup_metric_utils/browser/BUILD.gn, I felt
that
> it was less ambiguous that way.
> 
> Would you prefer that I replace all "/browser:browser" by just "/browser"?

Yes. gn resolves //foo to //foo:foo, so saying //foo:foo is always a waste of
characters :-)

[Ben Goodger (Google), 2015-11-13 22:57:57]

On 2015/11/13 22:56:33, Ben Goodger (Google) wrote:
> On 2015/11/13 20:25:22, fdoray wrote:
> > ben@: Could you take another look?
> > 
> >
>
https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
> > File components/html_viewer/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1413533008/diff/60001/components/html_viewer/...
> > components/html_viewer/BUILD.gn:164:
> > "//components/startup_metric_utils/browser:browser",
> > On 2015/11/13 20:07:37, Ben Goodger (Google) wrote:
> > > why is this necessary?
> > 
> > html_viewer builds correctly without ":browser". However, since there is now
2
> > targets defined in components/startup_metric_utils/browser/BUILD.gn, I felt
> that
> > it was less ambiguous that way.
> > 
> > Would you prefer that I replace all "/browser:browser" by just "/browser"?
> 
> Yes. gn resolves //foo to //foo:foo, so saying //foo:foo is always a waste of
> characters :-)

One pattern I like is to have a group in every BUILD.gn matching the name of the
dir that builds all the deps (incl tests) of that dir, and have a target called
"lib" that builds the meat.

e.g. for src/foo:

//foo:foo
  deps = [ ":lib", ":unittests" ]

//foo:lib
//foo:unittests
etc.

[fdoray, 2015-11-16 14:22:22]

ben@: I updated components/startup_metric_utils/browser/BUILD.gn as you
recommended. Can you take another look?

[Ben Goodger (Google), 2015-11-16 19:12:58]

chrome stuff lgtm

[fdoray, 2015-11-16 19:13:33]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-11-16 19:13:33]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
tsepez@chromium.org, asvitkine@chromium.org, agl@chromium.org
Link to the patchset: https://codereview.chromium.org/1413533008/#ps140001
(title: "fix whitespace error.")

[commit-bot: I haz the power, 2015-11-16 19:13:58]

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

[commit-bot: I haz the power, 2015-11-16 20:44:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-16 20:45:43]

Patchset 8 (id:??) landed as
https://crrev.com/443bd11bc8106bea98a89819b7bb1b35d6e5ddf8
Cr-Commit-Position: refs/heads/master@{#359904}