Extract browser shared class initialization from ChromeApplication.

Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466
,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Committed: https://crrev.com/2a3b9d162386fc0e7ba0c97f13def3631752dfdb
Cr-Original-Commit-Position: refs/heads/master@{#417814}
Cr-Commit-Position: refs/heads/master@{#418262}

[Ted C, 2016-08-31 18:28:13]

tedchoc@chromium.org changed reviewers:
+ agrieve@chromium.org, yfriedman@chromium.org

[Ted C, 2016-08-31 18:28:14]

Hit this again in the internal bug referenced, so I wanted to throw this out
there for some initial thoughts/disgust to get the conversation started.

Not fully fleshed out and there are probably places that need to be triggering
the class initialization that relied on the early initialization from the
application (ChromeLauncherActivity doesn't initialize native but might need
this stuff...sync perhaps?).

[agrieve, 2016-08-31 19:30:03]

agrieve@chromium.org changed reviewers:
+ wnwen@chromium.org

[agrieve, 2016-08-31 19:30:04]

+ Peter who's done some ChromeBrowserInitializer work before.

Certainly looks like an improvement to me.

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserSharedClassInitializer.java
(right):

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserSharedClassInitializer.java:12:
/**
nit: remove empty comment

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:118:
ChromeApplication application = (ChromeApplication)
context.getApplicationContext();
nit: can use ContextUtils.getApplicationContext() rather than passing in a
context.

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
private static BrowserSharedClassInitializer fetchSharedClassInitializer(Context
context) {
Manifest + Reflection will also require an extra -keep so proguard doesn't
rename the class. 

Instead, how about having a
ChromeBrowserIniitializer.setSharedClassInitializer() that is called from the
downstream Application.onCreate? If it's still null when we need it, then assume
ChromePublic and new up the default SharedClassInitializer.

[Ted C, 2016-08-31 23:07:55]

tedchoc@chromium.org changed reviewers:
+ torne@chromium.org

[Ted C, 2016-08-31 23:07:56]

+torne to affirm or reject my recollection about Applications

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
private static BrowserSharedClassInitializer fetchSharedClassInitializer(Context
context) {
On 2016/08/31 19:30:04, agrieve wrote:
> Manifest + Reflection will also require an extra -keep so proguard doesn't
> rename the class. 
> 
> Instead, how about having a
> ChromeBrowserIniitializer.setSharedClassInitializer() that is called from the
> downstream Application.onCreate? If it's still null when we need it, then
assume
> ChromePublic and new up the default SharedClassInitializer.

This CL operated under the assumption that we wanted to be rid of as much
Application code as possible (and hopefully removed entirely).

I know that torne@ wanted to remove them as that was the guidance from
Android.

I do think your way works, but it again would be called for all processes
and not just the browser and that is less than ideal (of course it is
just setting a single variable essentially, but it was my attempt to
remove its involvement entirely).

[Torne, 2016-09-01 13:13:13]

On 2016/08/31 23:07:56, Ted C wrote:
> +torne to affirm or reject my recollection about Applications
> 
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
> (right):
> 
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
> private static BrowserSharedClassInitializer
fetchSharedClassInitializer(Context
> context) {
> On 2016/08/31 19:30:04, agrieve wrote:
> > Manifest + Reflection will also require an extra -keep so proguard doesn't
> > rename the class. 
> > 
> > Instead, how about having a
> > ChromeBrowserIniitializer.setSharedClassInitializer() that is called from
the
> > downstream Application.onCreate? If it's still null when we need it, then
> assume
> > ChromePublic and new up the default SharedClassInitializer.
> 
> This CL operated under the assumption that we wanted to be rid of as much
> Application code as possible (and hopefully removed entirely).
> 
> I know that torne@ wanted to remove them as that was the guidance from
> Android.

We definitely want to eliminate the concept of an Application subclass from all
layers *below* chrome (components/content/base/etc), because WebView never has a
custom application context and so code that depends on it cannot be sanely
shared with WebView.

Whether chrome eliminates it entirely or not is a separate question. The general
advice from Android is that custom application subclasses should be avoided and
the feature even existing in the first place was a mistake in hindsight, and I
agree with the logic behind this so would generally support us moving in that
direction even for code that doesn't impact WebView. To be specific, the main
reasons to avoid a custom application subclasses that are relevant to us are:

1) Content providers can be initialised *before* Application.onCreate is called,
which can make the code hard to deal with - anything that gets called by content
providers has to carefully avoid touching the Application, while the rest of the
code is just assuming they're magically initialised globals. I think we've had
bugs caused by this before.

2) Application.onCreate often ends up doing a lot of work that isn't always
required, since it doesn't know *why* the application is being started, and may
do work that's unnecessary/too early in cases where the app is starting up to
run a specific service/activity/etc. (in our case, it all gets run in the
renderer process even though it's mostly not needed).

The normal advice is to just use individual lazy-initialised singletons for
globals, instead of storing them all in one big singleton Application, and just
let them be created when first used (so that they aren't created when not
needed).

However, our upstream/downstream split causes a problem here, because there's no
easy way to replace the implementation of a particular singleton with a
different downstream version. Using the Application subclass is the easy option,
because the system ActivityThread code looks up the right subclass name in the
manifest for us and means we can trivially have different onCreate
implementations. I'm not sure that replacing that with our own metadata looked
up from the manifest is really a good answer, though; that has a runtime cost
we'll have to pay, and means keeping additional things in proguard, and
generally seems weird and surprising.

Just keeping a "setInitialiser()" type function call in Application.onCreate is
an option, but that doesn't solve the content provider issue - in fact it might
make more subtle bugs possible where we accidentally trigger the upstream
version of the initialisation in a downstream APK, if the startup happened
because of a content provider. This option still seems less gross than putting
it in metadata, though: if we were able to reduce Chrome's Application subclass
to just one or more calls to "setInitialiser()" type functions that do no real
work but just set what classes will be used to do particular parts of
initialisation *later*, on demand, I think that'd be a good place to get to.

There's a different option that comes to mind; not sure whether I like it more
or not (or what others will think), but: we could simply have an
"ApplicationGlobalFactory" type class defined upstream that contains just a
bunch of static methods that create particular globals, put it in its own
target, and simply not compile that target into the downstream version, instead
substituting a different target which defines the *same class* but with
different implementation. i.e. do the configuration at build time, not run time.

[agrieve, 2016-09-01 13:50:51]

On 2016/09/01 13:13:13, Torne wrote:
> On 2016/08/31 23:07:56, Ted C wrote:
> > +torne to affirm or reject my recollection about Applications
> > 
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
> > private static BrowserSharedClassInitializer
> fetchSharedClassInitializer(Context
> > context) {
> > On 2016/08/31 19:30:04, agrieve wrote:
> > > Manifest + Reflection will also require an extra -keep so proguard doesn't
> > > rename the class. 
> > > 
> > > Instead, how about having a
> > > ChromeBrowserIniitializer.setSharedClassInitializer() that is called from
> the
> > > downstream Application.onCreate? If it's still null when we need it, then
> > assume
> > > ChromePublic and new up the default SharedClassInitializer.
> > 
> > This CL operated under the assumption that we wanted to be rid of as much
> > Application code as possible (and hopefully removed entirely).
> > 
> > I know that torne@ wanted to remove them as that was the guidance from
> > Android.
> 
> We definitely want to eliminate the concept of an Application subclass from
all
> layers *below* chrome (components/content/base/etc), because WebView never has
a
> custom application context and so code that depends on it cannot be sanely
> shared with WebView.
> 
> Whether chrome eliminates it entirely or not is a separate question. The
general
> advice from Android is that custom application subclasses should be avoided
and
> the feature even existing in the first place was a mistake in hindsight, and I
> agree with the logic behind this so would generally support us moving in that
> direction even for code that doesn't impact WebView. To be specific, the main
> reasons to avoid a custom application subclasses that are relevant to us are:
> 
> 1) Content providers can be initialised *before* Application.onCreate is
called,
> which can make the code hard to deal with - anything that gets called by
content
> providers has to carefully avoid touching the Application, while the rest of
the
> code is just assuming they're magically initialised globals. I think we've had
> bugs caused by this before.
> 
> 2) Application.onCreate often ends up doing a lot of work that isn't always
> required, since it doesn't know *why* the application is being started, and
may
> do work that's unnecessary/too early in cases where the app is starting up to
> run a specific service/activity/etc. (in our case, it all gets run in the
> renderer process even though it's mostly not needed).
> 
> The normal advice is to just use individual lazy-initialised singletons for
> globals, instead of storing them all in one big singleton Application, and
just
> let them be created when first used (so that they aren't created when not
> needed).
> 
> However, our upstream/downstream split causes a problem here, because there's
no
> easy way to replace the implementation of a particular singleton with a
> different downstream version. Using the Application subclass is the easy
option,
> because the system ActivityThread code looks up the right subclass name in the
> manifest for us and means we can trivially have different onCreate
> implementations. I'm not sure that replacing that with our own metadata looked
> up from the manifest is really a good answer, though; that has a runtime cost
> we'll have to pay, and means keeping additional things in proguard, and
> generally seems weird and surprising.
> 
> Just keeping a "setInitialiser()" type function call in Application.onCreate
is
> an option, but that doesn't solve the content provider issue - in fact it
might
> make more subtle bugs possible where we accidentally trigger the upstream
> version of the initialisation in a downstream APK, if the startup happened
> because of a content provider. This option still seems less gross than putting
> it in metadata, though: if we were able to reduce Chrome's Application
subclass
> to just one or more calls to "setInitialiser()" type functions that do no real
> work but just set what classes will be used to do particular parts of
> initialisation *later*, on demand, I think that'd be a good place to get to.

We can solve the content provider issue by overriding attachBaseContext() rather
than onCreate().

> 
> There's a different option that comes to mind; not sure whether I like it more
> or not (or what others will think), but: we could simply have an
> "ApplicationGlobalFactory" type class defined upstream that contains just a
> bunch of static methods that create particular globals, put it in its own
> target, and simply not compile that target into the downstream version,
instead
> substituting a different target which defines the *same class* but with
> different implementation. i.e. do the configuration at build time, not run
time.

But I do like the idea of a build-time toggle as well. It might lead to more
annoying 2-sided patches though (can't add a new singleton upstream without
breaking downstream).

[agrieve, 2016-09-01 13:51:35]

On 2016/09/01 13:50:51, agrieve wrote:
> On 2016/09/01 13:13:13, Torne wrote:
> > On 2016/08/31 23:07:56, Ted C wrote:
> > > +torne to affirm or reject my recollection about Applications
> > > 
> > >
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
> > > private static BrowserSharedClassInitializer
> > fetchSharedClassInitializer(Context
> > > context) {
> > > On 2016/08/31 19:30:04, agrieve wrote:
> > > > Manifest + Reflection will also require an extra -keep so proguard
doesn't
> > > > rename the class. 
> > > > 
> > > > Instead, how about having a
> > > > ChromeBrowserIniitializer.setSharedClassInitializer() that is called
from
> > the
> > > > downstream Application.onCreate? If it's still null when we need it,
then
> > > assume
> > > > ChromePublic and new up the default SharedClassInitializer.
> > > 
> > > This CL operated under the assumption that we wanted to be rid of as much
> > > Application code as possible (and hopefully removed entirely).
> > > 
> > > I know that torne@ wanted to remove them as that was the guidance from
> > > Android.
> > 
> > We definitely want to eliminate the concept of an Application subclass from
> all
> > layers *below* chrome (components/content/base/etc), because WebView never
has
> a
> > custom application context and so code that depends on it cannot be sanely
> > shared with WebView.
> > 
> > Whether chrome eliminates it entirely or not is a separate question. The
> general
> > advice from Android is that custom application subclasses should be avoided
> and
> > the feature even existing in the first place was a mistake in hindsight, and
I
> > agree with the logic behind this so would generally support us moving in
that
> > direction even for code that doesn't impact WebView. To be specific, the
main
> > reasons to avoid a custom application subclasses that are relevant to us
are:
> > 
> > 1) Content providers can be initialised *before* Application.onCreate is
> called,
> > which can make the code hard to deal with - anything that gets called by
> content
> > providers has to carefully avoid touching the Application, while the rest of
> the
> > code is just assuming they're magically initialised globals. I think we've
had
> > bugs caused by this before.
> > 
> > 2) Application.onCreate often ends up doing a lot of work that isn't always
> > required, since it doesn't know *why* the application is being started, and
> may
> > do work that's unnecessary/too early in cases where the app is starting up
to
> > run a specific service/activity/etc. (in our case, it all gets run in the
> > renderer process even though it's mostly not needed).
> > 
> > The normal advice is to just use individual lazy-initialised singletons for
> > globals, instead of storing them all in one big singleton Application, and
> just
> > let them be created when first used (so that they aren't created when not
> > needed).
> > 
> > However, our upstream/downstream split causes a problem here, because
there's
> no
> > easy way to replace the implementation of a particular singleton with a
> > different downstream version. Using the Application subclass is the easy
> option,
> > because the system ActivityThread code looks up the right subclass name in
the
> > manifest for us and means we can trivially have different onCreate
> > implementations. I'm not sure that replacing that with our own metadata
looked
> > up from the manifest is really a good answer, though; that has a runtime
cost
> > we'll have to pay, and means keeping additional things in proguard, and
> > generally seems weird and surprising.
> > 
> > Just keeping a "setInitialiser()" type function call in Application.onCreate
> is
> > an option, but that doesn't solve the content provider issue - in fact it
> might
> > make more subtle bugs possible where we accidentally trigger the upstream
> > version of the initialisation in a downstream APK, if the startup happened
> > because of a content provider. This option still seems less gross than
putting
> > it in metadata, though: if we were able to reduce Chrome's Application
> subclass
> > to just one or more calls to "setInitialiser()" type functions that do no
real
> > work but just set what classes will be used to do particular parts of
> > initialisation *later*, on demand, I think that'd be a good place to get to.
> 
> We can solve the content provider issue by overriding attachBaseContext()
rather
> than onCreate().
> 
> > 
> > There's a different option that comes to mind; not sure whether I like it
more
> > or not (or what others will think), but: we could simply have an
> > "ApplicationGlobalFactory" type class defined upstream that contains just a
> > bunch of static methods that create particular globals, put it in its own
> > target, and simply not compile that target into the downstream version,
> instead
> > substituting a different target which defines the *same class* but with
> > different implementation. i.e. do the configuration at build time, not run
> time.
> 
> But I do like the idea of a build-time toggle as well. It might lead to more
> annoying 2-sided patches though (can't add a new singleton upstream without
> breaking downstream).

Hmm, I guess actually you could just add the getter downstream first... so
nevermind.

[Peter Wen, 2016-09-01 13:52:16]

Sounds like the setInitializer option is what Yaron did with
SafeBrowsingApiBridge#setSafeBrowsingHandlerType which is only called downstream
in ChromeApplicationInternal.onCreate and nowhere else. So that's good as an
example if we want to move in that direction.

[Torne, 2016-09-01 13:52:39]

On 2016/09/01 13:51:35, agrieve wrote:
> On 2016/09/01 13:50:51, agrieve wrote:
> > On 2016/09/01 13:13:13, Torne wrote:
> > > On 2016/08/31 23:07:56, Ted C wrote:
> > > > +torne to affirm or reject my recollection about Applications
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> > > > File
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2297193003/diff/1/chrome/android/java/src/org...
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:162:
> > > > private static BrowserSharedClassInitializer
> > > fetchSharedClassInitializer(Context
> > > > context) {
> > > > On 2016/08/31 19:30:04, agrieve wrote:
> > > > > Manifest + Reflection will also require an extra -keep so proguard
> doesn't
> > > > > rename the class. 
> > > > > 
> > > > > Instead, how about having a
> > > > > ChromeBrowserIniitializer.setSharedClassInitializer() that is called
> from
> > > the
> > > > > downstream Application.onCreate? If it's still null when we need it,
> then
> > > > assume
> > > > > ChromePublic and new up the default SharedClassInitializer.
> > > > 
> > > > This CL operated under the assumption that we wanted to be rid of as
much
> > > > Application code as possible (and hopefully removed entirely).
> > > > 
> > > > I know that torne@ wanted to remove them as that was the guidance from
> > > > Android.
> > > 
> > > We definitely want to eliminate the concept of an Application subclass
from
> > all
> > > layers *below* chrome (components/content/base/etc), because WebView never
> has
> > a
> > > custom application context and so code that depends on it cannot be sanely
> > > shared with WebView.
> > > 
> > > Whether chrome eliminates it entirely or not is a separate question. The
> > general
> > > advice from Android is that custom application subclasses should be
avoided
> > and
> > > the feature even existing in the first place was a mistake in hindsight,
and
> I
> > > agree with the logic behind this so would generally support us moving in
> that
> > > direction even for code that doesn't impact WebView. To be specific, the
> main
> > > reasons to avoid a custom application subclasses that are relevant to us
> are:
> > > 
> > > 1) Content providers can be initialised *before* Application.onCreate is
> > called,
> > > which can make the code hard to deal with - anything that gets called by
> > content
> > > providers has to carefully avoid touching the Application, while the rest
of
> > the
> > > code is just assuming they're magically initialised globals. I think we've
> had
> > > bugs caused by this before.
> > > 
> > > 2) Application.onCreate often ends up doing a lot of work that isn't
always
> > > required, since it doesn't know *why* the application is being started,
and
> > may
> > > do work that's unnecessary/too early in cases where the app is starting up
> to
> > > run a specific service/activity/etc. (in our case, it all gets run in the
> > > renderer process even though it's mostly not needed).
> > > 
> > > The normal advice is to just use individual lazy-initialised singletons
for
> > > globals, instead of storing them all in one big singleton Application, and
> > just
> > > let them be created when first used (so that they aren't created when not
> > > needed).
> > > 
> > > However, our upstream/downstream split causes a problem here, because
> there's
> > no
> > > easy way to replace the implementation of a particular singleton with a
> > > different downstream version. Using the Application subclass is the easy
> > option,
> > > because the system ActivityThread code looks up the right subclass name in
> the
> > > manifest for us and means we can trivially have different onCreate
> > > implementations. I'm not sure that replacing that with our own metadata
> looked
> > > up from the manifest is really a good answer, though; that has a runtime
> cost
> > > we'll have to pay, and means keeping additional things in proguard, and
> > > generally seems weird and surprising.
> > > 
> > > Just keeping a "setInitialiser()" type function call in
Application.onCreate
> > is
> > > an option, but that doesn't solve the content provider issue - in fact it
> > might
> > > make more subtle bugs possible where we accidentally trigger the upstream
> > > version of the initialisation in a downstream APK, if the startup happened
> > > because of a content provider. This option still seems less gross than
> putting
> > > it in metadata, though: if we were able to reduce Chrome's Application
> > subclass
> > > to just one or more calls to "setInitialiser()" type functions that do no
> real
> > > work but just set what classes will be used to do particular parts of
> > > initialisation *later*, on demand, I think that'd be a good place to get
to.
> > 
> > We can solve the content provider issue by overriding attachBaseContext()
> rather
> > than onCreate().
> > 
> > > 
> > > There's a different option that comes to mind; not sure whether I like it
> more
> > > or not (or what others will think), but: we could simply have an
> > > "ApplicationGlobalFactory" type class defined upstream that contains just
a
> > > bunch of static methods that create particular globals, put it in its own
> > > target, and simply not compile that target into the downstream version,
> > instead
> > > substituting a different target which defines the *same class* but with
> > > different implementation. i.e. do the configuration at build time, not run
> > time.
> > 
> > But I do like the idea of a build-time toggle as well. It might lead to more
> > annoying 2-sided patches though (can't add a new singleton upstream without
> > breaking downstream).
> 
> Hmm, I guess actually you could just add the getter downstream first... so
> nevermind.

Yeah. You'd only need to use this mechanism for things that are *actually
different* downstream, so you could land the different downstream version first,
and *then* replace the upstream code that created it directly with one that
indirected via the factory.

[Torne, 2016-09-01 13:53:53]

On 2016/09/01 13:52:16, Peter Wen wrote:
> Sounds like the setInitializer option is what Yaron did with
> SafeBrowsingApiBridge#setSafeBrowsingHandlerType which is only called
downstream
> in ChromeApplicationInternal.onCreate and nowhere else. So that's good as an
> example if we want to move in that direction.

It's definitely preferable to doing the actual work in Application.onCreate, so
just moving to that would be fine if that's what people prefer (as long as you
only do it in the chrome layer, and not lower levels that WebView cares about).

[agrieve, 2016-09-01 14:02:50]

On 2016/09/01 13:53:53, Torne wrote:
> On 2016/09/01 13:52:16, Peter Wen wrote:
> > Sounds like the setInitializer option is what Yaron did with
> > SafeBrowsingApiBridge#setSafeBrowsingHandlerType which is only called
> downstream
> > in ChromeApplicationInternal.onCreate and nowhere else. So that's good as an
> > example if we want to move in that direction.
> 
> It's definitely preferable to doing the actual work in Application.onCreate,
so
> just moving to that would be fine if that's what people prefer (as long as you
> only do it in the chrome layer, and not lower levels that WebView cares
about).

That would likely allow us to have lazy instantiation as well. Are there any
such classes that webview does care about? E.g. Maybe we should have
BrowserModule, ContentModule, BaseModule, etc where each one is build-time
replaceable. Using the word "Module" since they may not necessarily hold only
singletons (and this is the term dependency injection uses for this concept)

[Torne, 2016-09-01 14:11:56]

On 2016/09/01 14:02:50, agrieve wrote:
> On 2016/09/01 13:53:53, Torne wrote:
> > On 2016/09/01 13:52:16, Peter Wen wrote:
> > > Sounds like the setInitializer option is what Yaron did with
> > > SafeBrowsingApiBridge#setSafeBrowsingHandlerType which is only called
> > downstream
> > > in ChromeApplicationInternal.onCreate and nowhere else. So that's good as
an
> > > example if we want to move in that direction.
> > 
> > It's definitely preferable to doing the actual work in Application.onCreate,
> so
> > just moving to that would be fine if that's what people prefer (as long as
you
> > only do it in the chrome layer, and not lower levels that WebView cares
> about).
> 
> That would likely allow us to have lazy instantiation as well. Are there any
> such classes that webview does care about? E.g. Maybe we should have
> BrowserModule, ContentModule, BaseModule, etc where each one is build-time
> replaceable. Using the word "Module" since they may not necessarily hold only
> singletons (and this is the term dependency injection uses for this concept)

WebView doesn't currently have any downstream variation at all, and even if we
do it's likely to be at a high level (in the webview code itself) and not in
base/content/etc. I would rather those things stay concrete unless someone has a
really good use case.

[Ted C, 2016-09-02 17:46:42]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.

BUG=b/31171101
==========

to

==========
Extract browser shared class initialization from ChromeApplication.
    
There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.
    
initializeProcess() should also go away, but it seems tied to
the foreground session tracking.
    
This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.
    
BUG=560466,b/31171101
==========

[Ted C, 2016-09-02 17:58:51]

attempt part deux

This gets rid of the meta data oddness and uses a class level override like
SafeBrowsingApiBridge.

Still need to audit the AndroidManifest, look at services, providers, etc...
that do not trigger the native library and might have relied on the application
startup.

I was "somewhat" tempted to write a class where we could track unexpected access
to our overridable singletons.

class TrackeryThing<T> {
    private StackTraceElement[] mPreviousAccess;
    private T mThing;

    void set(T override) {
        if (mPreviousAccess != null) EXPLODE!;
        mThing = override;
    }

    T get() {
        if (DEBUG) {
            mPreviousAccess = Thread.currentThread().getStackTrace();
        }
        return mThing;
    }
}

In general, trying to track down the hidden dependencies makes this change
disconcerting for me, but we are also quite far from branch so if we are
convinced in doing this then I could do my manual audit, find what I can, and
hope we catch anything/wait for bugs to catch it.

End rambling.

[Ted C, 2016-09-02 21:49:32]

tedchoc@chromium.org changed reviewers:
+ khushalsagar@chromium.org, nyquist@chromium.org

[Ted C, 2016-09-02 21:49:33]

+nyquist +khushalsagar for sync related init paths.

I "think" this would totally work in all cases and could never cause any weird
issues with services that aren't used that often but somehow needed something
obscure from Application.

[Torne, 2016-09-05 11:33:18]

So this looks safe, but just one "initializeSharedClasses" method looks weird,
compared to things being initialised on-demand. I'm gonna try to stay out of
this as much as I can though ;)

[agrieve, 2016-09-06 14:34:44]

I think having only lazy getters would be even better, but that could still be
done with follow-ups and on a case-by-case basis. Having global init that is
browser-only (rather than renderer) is clearly an improvement, so lgtm!

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:58:
public static void setBrowserDelegateType(Class<? extends BrowserDelegate>
classOverride) {
Why not just pass in the actual BrowserDelegate instance? It has a trivial
constructor, so should be more efficient than passing in the class.

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:91:
public final void initializeSharedClasses() {
nit: I think it would be a bit nicer if this method were in
ChromeBrowserInitializer. That way all app entry-points would just have to know
about ChromeBrowserInitializer rather than ChromeBrowserInitializer or
BrowserDelegate. LIkewise, you could put setBrowserDelegateType into
ChromeBrowserInitializer and delete getInstance() (since no one else should need
to know about it).

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:30:
BrowserDelegate.getInstance().initializeSharedClasses();
nit: You call this in onCreate() in ChromeInvalidationClientService. I don't
think it matters where you call it from (constructor vs onCreate) but probably
should be consistent.

[Khushal, 2016-09-06 14:38:32]

On 2016/09/02 21:49:33, Ted C wrote:
> +nyquist +khushalsagar for sync related init paths.
> 
> I "think" this would totally work in all cases and could never cause any weird
> issues with services that aren't used that often but somehow needed something
> obscure from Application.

The cache invalidation/tango parts lgtm. Those 2 should be the only entry
points. Another one that I double checked was the TiclService, but looks like
all intents from the application come to the InvalidationClientService which
then re-directs them to the TiclService. Tommy can confirm too.

[Khushal, 2016-09-06 14:38:42]

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:30:
BrowserDelegate.getInstance().initializeSharedClasses();
Do we usually do stuff like this in OnCreate?

[Ted C, 2016-09-06 20:50:54]

Also, I definitely agree that moving to a lazy construction pattern is the right
thing to do long term.  Right now, I'm trying to get as much browser specific
code out of the application initialization (and general slim down
ChromeApplication).

I make no claims that this is the end goal by any stretch.

Added some questions to the comments before I go ahead and make any changes.

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:58:
public static void setBrowserDelegateType(Class<? extends BrowserDelegate>
classOverride) {
On 2016/09/06 14:34:43, agrieve wrote:
> Why not just pass in the actual BrowserDelegate instance? It has a trivial
> constructor, so should be more efficient than passing in the class.

I actually thought this was what you and Peter preferred based on this comment:
https://codereview.chromium.org/2297193003/#msg10

I don't have a particularly strong preference.  The one marginally nice 
thing with this approach is that we're not constructing an object in the
renderer/other processes that is never used. Granted, the object itself
shouldn't be terribly large, so there isn't much of a loss here.

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:91:
public final void initializeSharedClasses() {
On 2016/09/06 14:34:44, agrieve wrote:
> nit: I think it would be a bit nicer if this method were in
> ChromeBrowserInitializer. That way all app entry-points would just have to
know
> about ChromeBrowserInitializer rather than ChromeBrowserInitializer or
> BrowserDelegate. LIkewise, you could put setBrowserDelegateType into
> ChromeBrowserInitializer and delete getInstance() (since no one else should
need
> to know about it).

Long/medium term, I want to remove all the browser specific code from
ChromeApplication and move it here.  In particular, the foreground and
background
session tracking.

That isn't initialization specific "stuff", so maybe putting this in the
init package isn't the right place.

I think isolating the need for interacting with this is good though.  Really,
it should only be the Activities, ChromeBrowserInitializier, and the
DeferredStartupHandler.

Right now, it is used by the initializer and the startup handler, so I think
this is already outside of the scope of the CBI.

But, I do this this should be called something better than
initializeSharedClasses.

Maybe, initializePreNativeLibraryDependencies or "something".

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/gcm/ChromeGcmListenerService.java:30:
BrowserDelegate.getInstance().initializeSharedClasses();
On 2016/09/06 14:38:42, Khushal wrote:
> Do we usually do stuff like this in OnCreate?

onCreate seems better...moved everywhere to that.

[agrieve, 2016-09-07 02:07:44]

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:58:
public static void setBrowserDelegateType(Class<? extends BrowserDelegate>
classOverride) {
On 2016/09/06 20:50:54, Ted C wrote:
> On 2016/09/06 14:34:43, agrieve wrote:
> > Why not just pass in the actual BrowserDelegate instance? It has a trivial
> > constructor, so should be more efficient than passing in the class.
> 
> I actually thought this was what you and Peter preferred based on this
comment:
> https://codereview.chromium.org/2297193003/#msg10
> 
> I don't have a particularly strong preference.  The one marginally nice 
> thing with this approach is that we're not constructing an object in the
> renderer/other processes that is never used. Granted, the object itself
> shouldn't be terribly large, so there isn't much of a loss here.

I don't have a strong feeling one way or the other. Passing in an instance would
be less code, but avoiding an instantiation is reasonable thinking as well.

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:91:
public final void initializeSharedClasses() {
On 2016/09/06 20:50:54, Ted C wrote:
> On 2016/09/06 14:34:44, agrieve wrote:
> > nit: I think it would be a bit nicer if this method were in
> > ChromeBrowserInitializer. That way all app entry-points would just have to
> know
> > about ChromeBrowserInitializer rather than ChromeBrowserInitializer or
> > BrowserDelegate. LIkewise, you could put setBrowserDelegateType into
> > ChromeBrowserInitializer and delete getInstance() (since no one else should
> need
> > to know about it).
> 
> Long/medium term, I want to remove all the browser specific code from
> ChromeApplication and move it here.  In particular, the foreground and
> background
> session tracking.
> 
> That isn't initialization specific "stuff", so maybe putting this in the
> init package isn't the right place.
> 
> I think isolating the need for interacting with this is good though.  Really,
> it should only be the Activities, ChromeBrowserInitializier, and the
> DeferredStartupHandler.
> 
> Right now, it is used by the initializer and the startup handler, so I think
> this is already outside of the scope of the CBI.
> 
> But, I do this this should be called something better than
> initializeSharedClasses.
> 
> Maybe, initializePreNativeLibraryDependencies or "something".

I think I follow. To be honest, I'm not an expert on all the things
ChromeBrowserInitializer is responsible for, and just figured it had the same
mandate: "Initialize global state".

If ChromeBrowserInitializer is just for Chrome-as-a-browser things, and
BrowserDelegate is just for application-wide globals, it might be a good idea to
remove the term "Browser" from the name. The fact that it's in the browser
process is determined by its package name already.

How about "ensureApplicationGlobalsInitialized()" (although "something" does
have a certain appeal...)

[Peter Wen, 2016-09-07 14:36:48]

lgtm

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:58:
public static void setBrowserDelegateType(Class<? extends BrowserDelegate>
classOverride) {
On 2016/09/07 02:07:43, agrieve wrote:
> I don't have a strong feeling one way or the other. Passing in an instance
would
> be less code, but avoiding an instantiation is reasonable thinking as well.

+1 for avoiding instantiation, as this will set a pattern for other classes in
the future, which may be heavier to instantiate.

[Ted C, 2016-09-07 18:46:30]

One last round of rambling thoughts.  I think this plan makes sense to me, but
it'd be good to get some general consensus before I do all the renaming.

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:91:
public final void initializeSharedClasses() {
On 2016/09/07 02:07:43, agrieve wrote:
> On 2016/09/06 20:50:54, Ted C wrote:
> > On 2016/09/06 14:34:44, agrieve wrote:
> > > nit: I think it would be a bit nicer if this method were in
> > > ChromeBrowserInitializer. That way all app entry-points would just have to
> > know
> > > about ChromeBrowserInitializer rather than ChromeBrowserInitializer or
> > > BrowserDelegate. LIkewise, you could put setBrowserDelegateType into
> > > ChromeBrowserInitializer and delete getInstance() (since no one else
should
> > need
> > > to know about it).
> > 
> > Long/medium term, I want to remove all the browser specific code from
> > ChromeApplication and move it here.  In particular, the foreground and
> > background
> > session tracking.
> > 
> > That isn't initialization specific "stuff", so maybe putting this in the
> > init package isn't the right place.
> > 
> > I think isolating the need for interacting with this is good though. 
Really,
> > it should only be the Activities, ChromeBrowserInitializier, and the
> > DeferredStartupHandler.
> > 
> > Right now, it is used by the initializer and the startup handler, so I think
> > this is already outside of the scope of the CBI.
> > 
> > But, I do this this should be called something better than
> > initializeSharedClasses.
> > 
> > Maybe, initializePreNativeLibraryDependencies or "something".
> 
> I think I follow. To be honest, I'm not an expert on all the things
> ChromeBrowserInitializer is responsible for, and just figured it had the same
> mandate: "Initialize global state".
> 
> If ChromeBrowserInitializer is just for Chrome-as-a-browser things, and
> BrowserDelegate is just for application-wide globals, it might be a good idea
to
> remove the term "Browser" from the name. The fact that it's in the browser
> process is determined by its package name already.
> 
> How about "ensureApplicationGlobalsInitialized()" (although "something" does
> have a certain appeal...)
> 

These are some great points.

CBI is really designed for spinning up an Activity that requires the native
library.

I was originally thinking that the BrowserDelegate would be a consuming all
of the ChromeApplication "stuff", but I'm no longer convinced.

Maybe this should be really specific about the browser process one time
initialization.  We could start pulling out stuff from CBI to this as well
as the stuff in ChromeApplication.

Then the stuff around activity foreground/background could go into a second
lifetime tracking class to keep these clearly distinct and not as muddled
as ChromeApplication has become.

Some maybe this class would be better named as ProcessInitializer, maybe
ProcessInitializerHelper or ProcessInitializerDelegate.  While I agree
the Browser name seems duplicate, it is helpful if you see if referenced
in stuff like the application where we are poking at browser specific stuff
that not necessary should be poked by those things.

Here's my updated direction.

This class would pull in the following from ChromeApplication:
onCreate stuff
initializeSharedClasses
initializeProcess

That gets moved to this ProcessInitializerDelegate-thingy:
initializePreNative
initializePostNativeDeferredStartupTasks
initializePostNativeStartup

I feel that is clearer about the intent.

Then we create a ChromeSessionTracker or something that pulls in
the foreground/background session tracking business from ChromeApplication.

I feel that is actually a pretty good breakup of work into more logic
units.  Thoughts before I do some renaming and general touch up?

The idea is that it wouldn't change this patch much except for whatever
name we think is best for this class and methods, but at least paves
a better direction IMO.

[agrieve, 2016-09-07 18:50:30]

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java
(right):

https://codereview.chromium.org/2297193003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/BrowserDelegate.java:91:
public final void initializeSharedClasses() {
On 2016/09/07 18:46:30, Ted C wrote:
> On 2016/09/07 02:07:43, agrieve wrote:
> > On 2016/09/06 20:50:54, Ted C wrote:
> > > On 2016/09/06 14:34:44, agrieve wrote:
> > > > nit: I think it would be a bit nicer if this method were in
> > > > ChromeBrowserInitializer. That way all app entry-points would just have
to
> > > know
> > > > about ChromeBrowserInitializer rather than ChromeBrowserInitializer or
> > > > BrowserDelegate. LIkewise, you could put setBrowserDelegateType into
> > > > ChromeBrowserInitializer and delete getInstance() (since no one else
> should
> > > need
> > > > to know about it).
> > > 
> > > Long/medium term, I want to remove all the browser specific code from
> > > ChromeApplication and move it here.  In particular, the foreground and
> > > background
> > > session tracking.
> > > 
> > > That isn't initialization specific "stuff", so maybe putting this in the
> > > init package isn't the right place.
> > > 
> > > I think isolating the need for interacting with this is good though. 
> Really,
> > > it should only be the Activities, ChromeBrowserInitializier, and the
> > > DeferredStartupHandler.
> > > 
> > > Right now, it is used by the initializer and the startup handler, so I
think
> > > this is already outside of the scope of the CBI.
> > > 
> > > But, I do this this should be called something better than
> > > initializeSharedClasses.
> > > 
> > > Maybe, initializePreNativeLibraryDependencies or "something".
> > 
> > I think I follow. To be honest, I'm not an expert on all the things
> > ChromeBrowserInitializer is responsible for, and just figured it had the
same
> > mandate: "Initialize global state".
> > 
> > If ChromeBrowserInitializer is just for Chrome-as-a-browser things, and
> > BrowserDelegate is just for application-wide globals, it might be a good
idea
> to
> > remove the term "Browser" from the name. The fact that it's in the browser
> > process is determined by its package name already.
> > 
> > How about "ensureApplicationGlobalsInitialized()" (although "something" does
> > have a certain appeal...)
> > 
> 
> These are some great points.
> 
> CBI is really designed for spinning up an Activity that requires the native
> library.
> 
> I was originally thinking that the BrowserDelegate would be a consuming all
> of the ChromeApplication "stuff", but I'm no longer convinced.
> 
> Maybe this should be really specific about the browser process one time
> initialization.  We could start pulling out stuff from CBI to this as well
> as the stuff in ChromeApplication.
> 
> Then the stuff around activity foreground/background could go into a second
> lifetime tracking class to keep these clearly distinct and not as muddled
> as ChromeApplication has become.
> 
> Some maybe this class would be better named as ProcessInitializer, maybe
> ProcessInitializerHelper or ProcessInitializerDelegate.  While I agree
> the Browser name seems duplicate, it is helpful if you see if referenced
> in stuff like the application where we are poking at browser specific stuff
> that not necessary should be poked by those things.
> 
> Here's my updated direction.
> 
> This class would pull in the following from ChromeApplication:
> onCreate stuff
> initializeSharedClasses
> initializeProcess
> 
> That gets moved to this ProcessInitializerDelegate-thingy:
> initializePreNative
> initializePostNativeDeferredStartupTasks
> initializePostNativeStartup
> 
> I feel that is clearer about the intent.
> 
> Then we create a ChromeSessionTracker or something that pulls in
> the foreground/background session tracking business from ChromeApplication.
> 
> I feel that is actually a pretty good breakup of work into more logic
> units.  Thoughts before I do some renaming and general touch up?
> 
> The idea is that it wouldn't change this patch much except for whatever
> name we think is best for this class and methods, but at least paves
> a better direction IMO.

sounds good to me!

[Ted C, 2016-09-08 21:30:35]

Uploaded with the new naming...PTAL for the hopefully final round of me changing
my mind.

[agrieve, 2016-09-09 13:53:29]

lgtm

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:104:
ProcessInitializationHandler.getInstance().initializePreNative();
nit: might be better to move this to the start* methods in order to avoid doing
work in a constructor.

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:95:
* Adding anything expensive to this must be avoided as it would delay the Chrome
startup path.
nit: can you add one more line here saying that all process entry-points that
don't use ChromeBrowserInitializer must call this method.

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeInvalidationClientService.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeInvalidationClientService.java:18:
ProcessInitializationHandler.getInstance().initializePreNative();
nit: should this go before the call to super?

[Ted C, 2016-09-09 16:41:31]

The CQ bit was checked by tedchoc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 16:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 16:48:25]

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

[commit-bot: I haz the power, 2016-09-09 16:48:26]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ted C, 2016-09-09 20:12:05]

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:104:
ProcessInitializationHandler.getInstance().initializePreNative();
On 2016/09/09 13:53:29, agrieve wrote:
> nit: might be better to move this to the start* methods in order to avoid
doing
> work in a constructor.

Moved to handlePreNativeStartup

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:95:
* Adding anything expensive to this must be avoided as it would delay the Chrome
startup path.
On 2016/09/09 13:53:29, agrieve wrote:
> nit: can you add one more line here saying that all process entry-points that
> don't use ChromeBrowserInitializer must call this method.

Done.

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeInvalidationClientService.java
(right):

https://codereview.chromium.org/2297193003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeInvalidationClientService.java:18:
ProcessInitializationHandler.getInstance().initializePreNative();
On 2016/09/09 13:53:29, agrieve wrote:
> nit: should this go before the call to super?

Done.

[Ted C, 2016-09-09 20:12:10]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-09-09 20:12:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org, wnwen@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2297193003/#ps140001
(title: "Final nits")

[commit-bot: I haz the power, 2016-09-09 20:13:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 21:41:53]

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

[commit-bot: I haz the power, 2016-09-09 21:41:54]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ted C, 2016-09-09 23:57:58]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-09-09 23:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
agrieve@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2297193003/#ps160001
(title: "Fix sync shell tests")

[commit-bot: I haz the power, 2016-09-09 23:58:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 01:51:41]

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

[commit-bot: I haz the power, 2016-09-10 01:51:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ted C, 2016-09-10 02:18:33]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-09-10 02:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
agrieve@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2297193003/#ps180001
(title: "Init tests on UI thread")

[commit-bot: I haz the power, 2016-09-10 02:18:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 03:32:39]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.
    
There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.
    
initializeProcess() should also go away, but it seems tied to
the foreground session tracking.
    
This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.
    
BUG=560466,b/31171101
==========

to

==========
Extract browser shared class initialization from ChromeApplication.
    
There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.
    
initializeProcess() should also go away, but it seems tied to
the foreground session tracking.
    
This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.
    
BUG=560466,b/31171101
==========

[commit-bot: I haz the power, 2016-09-10 03:32:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-10 03:34:30]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.
    
There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.
    
initializeProcess() should also go away, but it seems tied to
the foreground session tracking.
    
This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.
    
BUG=560466,b/31171101
==========

to

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

[commit-bot: I haz the power, 2016-09-10 03:34:31]

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

[Ted C, 2016-09-10 15:47:57]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2329813002/ by tedchoc@chromium.org.

The reason for reverting is: Missed tests in chrome_public_apk.  Need to do the
same thing I did in SyncTestBase.

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

.

[Ted C, 2016-09-13 00:32:49]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

to

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

[Ted C, 2016-09-13 00:35:10]

The CQ bit was checked by tedchoc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 00:35:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 01:51:30]

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

[commit-bot: I haz the power, 2016-09-13 01:51:31]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-09-13 16:01:57]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-09-13 16:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
agrieve@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2297193003/#ps200001
(title: "Moar test fixes")

[commit-bot: I haz the power, 2016-09-13 16:02:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 16:07:28]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

to

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

[commit-bot: I haz the power, 2016-09-13 16:07:30]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-13 16:10:49]

Description was changed from

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Cr-Commit-Position: refs/heads/master@{#417814}
==========

to

==========
Extract browser shared class initialization from ChromeApplication.

There are still many other things that I would like to remove
from ChromeApplication.  The foreground session tracking for
one could be moved to whatever delegate we settle on.

initializeProcess() should also go away, but it seems tied to
the foreground session tracking.

This focused on the two "more easily" moveable bits of the
deferred startup and class initialization.

BUG=560466,b/31171101

Committed: https://crrev.com/5dc9845c8cde999555c74391eb9c61832b072157
Committed: https://crrev.com/2a3b9d162386fc0e7ba0c97f13def3631752dfdb
Cr-Original-Commit-Position: refs/heads/master@{#417814}
Cr-Commit-Position: refs/heads/master@{#418262}
==========

[commit-bot: I haz the power, 2016-09-13 16:10:50]

Patchset 11 (id:??) landed as
https://crrev.com/2a3b9d162386fc0e7ba0c97f13def3631752dfdb
Cr-Commit-Position: refs/heads/master@{#418262}