[Android] Support startup histogram.

[Android] Support startup histogram.

This CL adds support for the
Startup.BrowserMessageLoopStartTimeFromMainEntry UMA histogram to
chromium_testshell. It does this by recording the start time as
early as possible on the Java side (well before the C++ library is even
loaded), and then reading this on the C++ side through a new JNI call.

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

[jeremy, 2013-08-27 19:18:51]

Nice!!

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
(right):

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
static long applicationStartWallclockMs_;
nit: WallClock (?)

https://codereview.chromium.org/23443009/diff/3001/chrome/app/chrome_main_del...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/app/chrome_main_del...
chrome/app/chrome_main_delegate.cc:348: #endif
I think this would be a bit clearer as:

#if !defined(OS_ANDROID)
  startup_metric_utils...
#else
  // comment about why android doesn't need this code.
#endif

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:692: // On Android the first run is
handled very differently, and the C++ doesn't
nit: C++ -> "C++ side of things"

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.cc:59: *MainEntryPointTimeInternal() =
base::Time::Now();
Sorry, I don't understand this change?

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.cc:62: void
RecordSavedMainEntryPointTime(const base::Time& entry_point_time) {
Again

#if defined(OS_ANDROID)

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.h:37: void
RecordSavedMainEntryPointTime(const base::Time& entry_point_time);
# if defined(OS_ANDROID)

[Yaron, 2013-08-27 20:42:29]

My main q is about having this in the common folder. Jeremy: do we use this for
child processes? If not, why is it in common? It being there is breaking a few
of our java conventions.

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
(right):

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
static long applicationStartWallclockMs_;
Make private and should be named sApplicationStartWallClockMs

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:97: { "StartupMetricUtils",
RegisterStartupMetricUtils },
Hmm. This is only used by the browser process but the code is in common. It
should be in a common jni registrar or you'd blow up calling it from the
renderer.

https://codereview.chromium.org/23443009/diff/3001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:3469:
'android/java/src/org/chromium/chrome/common/StartupMetricUtils.java',
this should be in a separate jni target since it's in common. Otherwise you
couldn't make the renderer depend on it.

https://codereview.chromium.org/23443009/diff/3001/chrome/common/android/star...
File chrome/common/android/startup_metric_utils_android.h (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/android/star...
chrome/common/android/startup_metric_utils_android.h:8: #include
"base/android/jni_android.h"
I think you can just use #include <jni.h>

[Yaron, 2013-08-27 20:43:10]

Err, I should add that its' breaking conventions but can be fixed by making the
changes I listed in comments. Just don't know if it's necessary

[jeremy, 2013-08-27 21:01:07]

No, only used for the browser process.

[Anthony Berent, 2013-08-27 21:20:45]

Jeremy,

Should I move the portable startup_metric_utils.* to browser, or are other
things in there used in the renderer?
No, only used for the browser process.

https://codereview.chromium.**org/23443009/<https://codereview.chromium.org/2...

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jeremy, 2013-08-27 21:39:19]

Nothing in there should be used by the renderer, however I don't want to
task you with cleaning up my mess :)


On Tue, Aug 27, 2013 at 2:20 PM, Anthony Berent <aberent@google.com> wrote:

> Jeremy,
>
> Should I move the portable startup_metric_utils.* to browser, or are other
> things in there used in the renderer?
> No, only used for the browser process.
>
>
https://codereview.chromium.**org/23443009/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aberent, 2013-08-28 12:46:30]

On 2013/08/27 21:39:19, jeremy wrote:
> Nothing in there should be used by the renderer, however I don't want to
> task you with cleaning up my mess :)

Actually happy to volunteer, unless you object; since if I land this without
clearing up "your mess" the mess gets worse, but since it involves small changes
to lots of files I will do it as a separate CL, which I will land first. 
> 
> 
> On Tue, Aug 27, 2013 at 2:20 PM, Anthony Berent <mailto:aberent@google.com>
wrote:
> 
> > Jeremy,
> >
> > Should I move the portable startup_metric_utils.* to browser, or are other
> > things in there used in the renderer?
> > No, only used for the browser process.
> >
> >
>
https://codereview.chromium.**org/23443009/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[aberent, 2013-08-28 13:30:24]

On 2013/08/28 12:46:30, aberent wrote:
> On 2013/08/27 21:39:19, jeremy wrote:
> > Nothing in there should be used by the renderer, however I don't want to
> > task you with cleaning up my mess :)
> 
> Actually happy to volunteer, unless you object; since if I land this without
> clearing up "your mess" the mess gets worse, but since it involves small
changes
> to lots of files I will do it as a separate CL, which I will land first. 
> > 
> > 
> > On Tue, Aug 27, 2013 at 2:20 PM, Anthony Berent <mailto:aberent@google.com>
> wrote:
> > 
> > > Jeremy,
> > >
> > > Should I move the portable startup_metric_utils.* to browser, or are other
> > > things in there used in the renderer?
> > > No, only used for the browser process.
> > >
> > >
> >
>
https://codereview.chromium.**org/23443009/%253Chttps://codereview.chromium.o...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

I have created http://crbug/280523 to manage moving these files.

[aberent, 2013-08-28 14:57:52]

On 2013/08/28 13:30:24, aberent wrote:
> On 2013/08/28 12:46:30, aberent wrote:
> > On 2013/08/27 21:39:19, jeremy wrote:
> > > Nothing in there should be used by the renderer, however I don't want to
> > > task you with cleaning up my mess :)
> > 
> > Actually happy to volunteer, unless you object; since if I land this without
> > clearing up "your mess" the mess gets worse, but since it involves small
> changes
> > to lots of files I will do it as a separate CL, which I will land first. 
> > > 
> > > 
> > > On Tue, Aug 27, 2013 at 2:20 PM, Anthony Berent
<mailto:aberent@google.com>
> > wrote:
> > > 
> > > > Jeremy,
> > > >
> > > > Should I move the portable startup_metric_utils.* to browser, or are
other
> > > > things in there used in the renderer?
> > > > No, only used for the browser process.
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.**org/23443009/%25253Chttps://codereview.chromium...>
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I have created http://crbug/280523 to manage moving these files.

Moving startup_metric_utils.* turns out to be more difficult than it looked (see
the bug), so I will continue with this CL without moving them.

[aberent, 2013-08-28 15:53:05]

New patch will follow once I have tested it.

Yaron: are you happy for this to proceed without us sorting out the location of
startup_metric_utils, and without initializing the jni from the renderer, or do
we need to sort this first?

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
(right):

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
static long applicationStartWallclockMs_;
On 2013/08/27 19:18:51, jeremy wrote:
> nit: WallClock (?)

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
static long applicationStartWallclockMs_;
On 2013/08/27 20:42:30, Yaron wrote:
> Make private and should be named sApplicationStartWallClockMs

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/app/chrome_main_del...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/app/chrome_main_del...
chrome/app/chrome_main_delegate.cc:348: #endif
On 2013/08/27 19:18:51, jeremy wrote:
> I think this would be a bit clearer as:
> 
> #if !defined(OS_ANDROID)
>   startup_metric_utils...
> #else
>   // comment about why android doesn't need this code.
> #endif

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/android/chr...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/android/chr...
chrome/browser/android/chrome_jni_registrar.cc:97: { "StartupMetricUtils",
RegisterStartupMetricUtils },
On 2013/08/27 20:42:30, Yaron wrote:
> Hmm. This is only used by the browser process but the code is in common. It
> should be in a common jni registrar or you'd blow up calling it from the
> renderer.

I don't want to create a common jni register only to delete it in a few days
time so I am leaving this until we decide whether we are moving
startup_metric_utils.*, and if so where to. Although it is architecturally wrong
none of these functions are actually ever called from the renderer, so it won't
actually blow up.

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:692: // On Android the first run is
handled very differently, and the C++ doesn't
On 2013/08/27 19:18:51, jeremy wrote:
> nit: C++ -> "C++ side of things"

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:3469:
'android/java/src/org/chromium/chrome/common/StartupMetricUtils.java',
On 2013/08/27 20:42:30, Yaron wrote:
> this should be in a separate jni target since it's in common. Otherwise you
> couldn't make the renderer depend on it.

As discussed elsewhere we don't want the renderer to depend on it. Common is the
wrong place for it, but we need to move the startup_metric_utils.* before we can
move it.

https://codereview.chromium.org/23443009/diff/3001/chrome/common/android/star...
File chrome/common/android/startup_metric_utils_android.h (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/android/star...
chrome/common/android/startup_metric_utils_android.h:8: #include
"base/android/jni_android.h"
On 2013/08/27 20:42:30, Yaron wrote:
> I think you can just use #include <jni.h>

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.cc:59: *MainEntryPointTimeInternal() =
base::Time::Now();
On 2013/08/27 19:18:51, jeremy wrote:
> Sorry, I don't understand this change?

Done. You are right, it isn't needed.

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.cc:62: void
RecordSavedMainEntryPointTime(const base::Time& entry_point_time) {
On 2013/08/27 19:18:51, jeremy wrote:
> Again
> 
> #if defined(OS_ANDROID)

Done.

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/23443009/diff/3001/chrome/common/startup_metr...
chrome/common/startup_metric_utils.h:37: void
RecordSavedMainEntryPointTime(const base::Time& entry_point_time);
On 2013/08/27 19:18:51, jeremy wrote:
> # if defined(OS_ANDROID)

Done.

[jeremy, 2013-08-28 18:15:56]

lgtm with nits

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:38: // of UI thread tasks a
second request to start it can come in while the
nit: of of

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:39: // first request is still
being processed. We must keep the same
nit: Some people have strong feelings about using the term 'we' in comments,
just FYI, I really don't mind either way :)

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:41: // Also only record the
start time first time round, since this is the start
nit: time -> time the

https://codereview.chromium.org/23443009/diff/23001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:346: // On Android the main entry point time
is when the Java code starts, not here.
nit: not here -> this happens before the C++ side of thing executes.

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
chrome/common/startup_metric_utils.cc:68: #endif
#endif  // OS_ANDROID

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
chrome/common/startup_metric_utils.h:39: #endif
#endif  // OS_ANDROID

[Yaron, 2013-08-29 00:43:54]

lgtm

I guess it's fine for now (and since I'm OOO for the next little bit) but I'd
like to see crbug.com/280523 resolved. I think the easiest path is to
temporarily allow bookmarks to depend on the moved header file since you're not
making it worse but you could arguably make it better by moving to a component.

https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
(right):

https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
private static long sApplicationStartWallClockMs_;
NIT: drop trailing "_"

[aberent, 2013-08-29 11:24:21]

On 2013/08/29 00:43:54, Yaron (ooo - Sep 9) wrote:
> lgtm
> 
> I guess it's fine for now (and since I'm OOO for the next little bit) but I'd
> like to see crbug.com/280523 resolved. I think the easiest path is to
> temporarily allow bookmarks to depend on the moved header file since you're
not
> making it worse but you could arguably make it better by moving to a
component.
> 
>
https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
> File
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
> (right):
> 
>
https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
> chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
> private static long sApplicationStartWallClockMs_;
> NIT: drop trailing "_"

I have realized that startup_metric_utils_android.* doesn't actually need to
reference startup_metrics_utils, so can be in chrome/browser/android. This means
that I am not doing anything new nasty in this CL. Since
startup_metric_utils_android is no longer so closely tied to
startup_metric_utils I am also changing its name to uma_utils.*.

A new patch follows shortly.

I will still create a separate CL to fix http://crbug.com/280523, but they are
now almost independent.

[aberent, 2013-08-29 11:45:12]

https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java
(right):

https://codereview.chromium.org/23443009/diff/23001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/common/StartupMetricUtils.java:10:
private static long sApplicationStartWallClockMs_;
On 2013/08/29 00:43:54, Yaron (ooo - Sep 9) wrote:
> NIT: drop trailing "_"

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:38: // of UI thread tasks a
second request to start it can come in while the
On 2013/08/28 18:15:56, jeremy wrote:
> nit: of of

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:39: // first request is still
being processed. We must keep the same
On 2013/08/28 18:15:56, jeremy wrote:
> nit: Some people have strong feelings about using the term 'we' in comments,
> just FYI, I really don't mind either way :)

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.cc:41: // Also only record the
start time first time round, since this is the start
On 2013/08/28 18:15:56, jeremy wrote:
> nit: time -> time the

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:346: // On Android the main entry point time
is when the Java code starts, not here.
On 2013/08/28 18:15:56, jeremy wrote:
> nit: not here -> this happens before the C++ side of thing executes.

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
chrome/common/startup_metric_utils.cc:68: #endif
On 2013/08/28 18:15:56, jeremy wrote:
> #endif  // OS_ANDROID

Done.

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/23443009/diff/23001/chrome/common/startup_met...
chrome/common/startup_metric_utils.h:39: #endif
On 2013/08/28 18:15:56, jeremy wrote:
> #endif  // OS_ANDROID

Done.

[aberent, 2013-08-29 11:51:03]

John,

Can you give me an owner lgtm on a few files in this patch that Yaron doesn't
own. He and jeremy (who wrote the desktop version of this) have already given me
lgtms.

The files I need lgtms on are:
    chrome/app/chrome_main_delegate.cc
    chrome/browser/chrome_browser_main.cc
    chrome/common/startup_metric_utils.cc
    chrome/common/startup_metric_utils.h

Thanks.

- Anthony

[jam, 2013-08-29 15:16:16]

lgtm

https://codereview.chromium.org/23443009/diff/44001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/44001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:343: #if !defined(OS_ANDROID)
nit: this would be slightly easier to read if the order of the conditions was
reversed

https://codereview.chromium.org/23443009/diff/44001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23443009/diff/44001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:689: #if !defined(OS_ANDROID)
ditto

[aberent, 2013-08-29 16:19:44]

https://codereview.chromium.org/23443009/diff/44001/chrome/app/chrome_main_de...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/23443009/diff/44001/chrome/app/chrome_main_de...
chrome/app/chrome_main_delegate.cc:343: #if !defined(OS_ANDROID)
On 2013/08/29 15:16:17, jam wrote:
> nit: this would be slightly easier to read if the order of the conditions was
> reversed

Done.

https://codereview.chromium.org/23443009/diff/44001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/23443009/diff/44001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:689: #if !defined(OS_ANDROID)
On 2013/08/29 15:16:17, jam wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-08-29 16:19:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23443009/8001

[commit-bot: I haz the power, 2013-08-29 19:14:22]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-08-30 08:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23443009/8001

[commit-bot: I haz the power, 2013-08-30 15:42:28]

Change committed as 220588