Move components/startup_metric_utils/* to components/startup_metric_utils/browser/*.

Move components/startup_metric_utils/* to components/startup_metric_utils/browser/*.

This CL first landed as revision 357702. It was reverted because of an
XP-only perf regression (crbug.com/551690). After discussion with
jschuh@, it was decided that this regression can be ignored.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794
Committed: https://crrev.com/45c02c1698aecadaa7ef2fa50894af672045adcf
Cr-Commit-Position: refs/heads/master@{#358636}

[fdoray, 2015-10-27 20:19:10]

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

[fdoray, 2015-10-27 20:19:10]

@gab: Could you do a first review for this CL? Thanks.

[gab, 2015-10-29 11:57:21]

lg and happy to see some unused dependencies cleanup :-), but one question about
one of the dependencies

Thanks,
Gab

https://codereview.chromium.org/1413153010/diff/1/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1413153010/diff/1/chrome/chrome_dll.gypi#newc...
chrome/chrome_dll.gypi:342:
'../components/components.gyp:startup_metric_utils_browser',
Hmm chrome_child_dll shouldn't depend on browser things (and a dependency wasn't
required here before, unless this is the propagation of the dependency removed
in chrome_renderer.gypi? if so we'll need to add common code for whatever piece
is common).

[fdoray, 2015-10-29 15:04:47]

gab@: Could you take another look? Thanks.

https://codereview.chromium.org/1413153010/diff/1/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/1413153010/diff/1/chrome/chrome_dll.gypi#newc...
chrome/chrome_dll.gypi:342:
'../components/components.gyp:startup_metric_utils_browser',
On 2015/10/29 11:57:21, gab wrote:
> Hmm chrome_child_dll shouldn't depend on browser things (and a dependency
wasn't
> required here before, unless this is the propagation of the dependency removed
> in chrome_renderer.gypi? if so we'll need to add common code for whatever
piece
> is common).

Yes, it was the propagation of the dependency removed from chrome_renderer.gypi.

The dependency was due to chrome_main_delegate.cc calling
RecordStartupProcessCreationTime() and RecordMainEntryPointTime(). There is no
reason to call these methods from a renderer process (they just set global
variables, which is useless unless we call other startup metric utils functions
later). Therefore, I added some "#if !defined(CHROME_MULTIPLE_DLL_CHILD)" to
eliminate the dependency between the renderer code and startup_metric_utils.

[gab, 2015-10-29 16:03:27]

Thanks, reply below.

Note: not a big deal this time, but best practice is to upload a separate patch
set titled "merge" or "rebase" or wtv after syncing and then the actual diff as
its own patch set (otherwise your inter-patch-set diff is polluted with changes
on trunk).

Cheers,
Gab

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:433: RecordMainStartupMetrics();
I think this should only be recorded in the browser, i.e.
CHROME_MULTIPLE_DLL_BROWSER instead of !CHROME_MULTIPLE_DLL_CHILD

[fdoray, 2015-10-29 17:00:01]

gab@: Could you take another look? Thanks.

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:433: RecordMainStartupMetrics();
On 2015/10/29 16:03:27, gab wrote:
> I think this should only be recorded in the browser, i.e.
> CHROME_MULTIPLE_DLL_BROWSER instead of !CHROME_MULTIPLE_DLL_CHILD

Unfortunately, CHROME_MULTIPLE_DLL_BROWSER/_CHILD are not always defined (they
are not for component builds). I think that's why elsewhere in
chrome_main_delegate.cc, !defined(BROWSER/CHILD) is used instead of
defined(CHILD/BROWSER).

When I use defined(CHROME_MULTIPLE_DLL_BROWSER) with a component build,
RecordMainStartupMetrics() is not executed, but it is if I use
!defined(CHROME_MULTIPLE_DLL_CHILD). 

See
https://code.google.com/p/chromium/codesearch#search/&q=CHROME_MULTIPLE_DLL_%...

[gab, 2015-10-29 17:18:28]

lgtm, thanks

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1413153010/diff/20001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:433: RecordMainStartupMetrics();
On 2015/10/29 17:00:01, fdoray wrote:
> On 2015/10/29 16:03:27, gab wrote:
> > I think this should only be recorded in the browser, i.e.
> > CHROME_MULTIPLE_DLL_BROWSER instead of !CHROME_MULTIPLE_DLL_CHILD
> 
> Unfortunately, CHROME_MULTIPLE_DLL_BROWSER/_CHILD are not always defined (they
> are not for component builds). I think that's why elsewhere in
> chrome_main_delegate.cc, !defined(BROWSER/CHILD) is used instead of
> defined(CHILD/BROWSER).
> 
> When I use defined(CHROME_MULTIPLE_DLL_BROWSER) with a component build,
> RecordMainStartupMetrics() is not executed, but it is if I use
> !defined(CHROME_MULTIPLE_DLL_CHILD). 
> 
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=CHROME_MULTIPLE_DLL_%...

I see. Then I guess this works (I don't like negative conditionals as they don't
scale to eventual new DLL splits but I doubt this will be a problem in practice
-- and there are other use cases with this issue in this file already).

Also, this means it will still be called in renderers in component builds which
is wrong (unless renderers eventually care about their main entry time, but then
MainEntryTime() should be in common/). We could confirm process_type.empty() but
that's a runtime check and won't help with removing the compile-time dependency.

Anyways, this is strictly better than before so lgtm.

[fdoray, 2015-10-30 18:15:30]

fdoray@chromium.org changed reviewers:
+ jeremy@chromium.org, sky@chromium.org

[fdoray, 2015-10-30 18:15:31]

sky@: Could you review chrome/, components/bookmarks/ and
components/html_viewer?

caitkp@: Could you review components/BUILD.gn and
components/startup_metric_utils.gypi?

Thanks!

https://codereview.chromium.org/1413153010/diff/60001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1413153010/diff/60001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:434: // initialized), so the function below
will also be called in renderers.
process command line initialized here:
content::ContentMainRunnerImpl::Initialize() Line 562

[fdoray, 2015-10-30 18:16:01]

fdoray@chromium.org changed reviewers:
+ caitkp@chromium.org
- jeremy@chromium.org

[fdoray, 2015-10-30 18:16:02]

sky@: Could you review chrome/, components/bookmarks/ and
components/html_viewer?

caitkp@: Could you review components/BUILD.gn and
components/startup_metric_utils.gypi?

Thanks!

[Cait (Slow), 2015-11-02 23:12:00]

components/BUILD.gn and components/startup_metric_utils.gypi LGTM

[fdoray, 2015-11-02 23:14:19]

caitkp@: Thanks!
sky@: Could you review chrome/*?

[Cait (Slow), 2015-11-02 23:16:51]

On 2015/11/02 23:14:19, fdoray wrote:
> caitkp@: Thanks!
> sky@: Could you review chrome/*?

Note that I didn't looks at components/bookmarks/ and components/html_viewer, so
sky@ (or someone else familiar with the bookmark code) should probably take a
look at those parts too.

[fdoray, 2015-11-03 19:06:49]

sky@: Could you review chrome/, components/bookmarks/ and
components/html_viewer?

[sky, 2015-11-03 23:29:45]

LGTM

[fdoray, 2015-11-03 23:45:02]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-11-03 23:45:02]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/1413153010/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-03 23:45:39]

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

[commit-bot: I haz the power, 2015-11-04 01:16:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 01:18:15]

Patchset 5 (id:??) landed as
https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}

[fdoray, 2015-11-05 22:38:00]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1431053003/ by fdoray@chromium.org.

The reason for reverting is: We suspect that this CL is responsible for a
7.1%-7.7% regression in startup.warm.blank_page (crbug.com/551690).

[fdoray, 2015-11-09 16:02:43]

Description was changed from

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794

Committed: https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}
==========

to

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794

Committed: https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}
==========

[fdoray, 2015-11-09 18:43:19]

Description was changed from

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794

Committed: https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}
==========

to

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

This CL first landed as revision 357702. It was reverted because of an
XP-only perf regression (crbug.com/551690). After discussion with
jschuh@, it was decided that this regression can be ignored.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794

Committed: https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}
==========

[fdoray, 2015-11-09 18:43:41]

Description was changed from

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

This CL first landed as revision 357702. It was reverted because of an
XP-only perf regression (crbug.com/551690). After discussion with
jschuh@, it was decided that this regression can be ignored.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

BUG=547794

Committed: https://crrev.com/055d3862641a91e4a5ff2a49bd5d0f835f0436d1
Cr-Commit-Position: refs/heads/master@{#357702}
==========

to

==========
Move components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

This CL first landed as revision 357702. It was reverted because of an
XP-only perf regression (crbug.com/551690). After discussion with
jschuh@, it was decided that this regression can be ignored.

We are working on new startup metrics that require sending IPCs from
the renderer to the browser. The message definitions will live in
components/startup_metric_utils/common/*, while the code that receives
IPCs and logs UMA metrics will live in
components/startup_metric_utils/browser/*.

As a first step towards implementing these new metrics, this CL moves
existing code from components/startup_metric_utils/* to
components/startup_metric_utils/browser/*.

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

[fdoray, 2015-11-09 18:55:18]

Re-opening the CL.

The CL first landed as revision 357702. It was reverted because of an XP-only
perf regression (crbug.com/551690). After discussion with jschuh@, it was
decided that this regression can be ignored, so I'll try resubmitting the CL.

[fdoray, 2015-11-09 18:55:30]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-11-09 18:55:30]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
sky@chromium.org, caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/1413153010/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-09 18:57:38]

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

[commit-bot: I haz the power, 2015-11-09 20:14:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-09 20:15:27]

Patchset 6 (id:??) landed as
https://crrev.com/45c02c1698aecadaa7ef2fa50894af672045adcf
Cr-Commit-Position: refs/heads/master@{#358636}