Define a Java-side global application context.

Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Callers of ApplicationStatus.getApplicationContext will be updated
incrementally in followup CLs once it's been verified that they only
require a Context and not a BaseChromiumApplication.

BUG=552419
Committed: https://crrev.com/961a488f6a5d8c82b576444054f5d26b3f8a6877
Cr-Commit-Position: refs/heads/master@{#361306}

[Torne, 2015-11-06 16:38:24]

This isn't finished yet, but there are some issues that I need to discuss with
relevant code owners before I finalise it.

[Torne, 2015-11-06 16:43:19]

torne@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[Torne, 2015-11-06 16:43:19]

+ben/sky: I have some questions about mojo in the context of this change. No
need to do a proper review yet, it's not finished.

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
File mojo/runner/android/android_handler.cc (right):

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
mojo/runner/android/android_handler.cc:54: // TODO: what to do about this?
Mojo folks: is it reasonable to remove this mechanism and instead handle the
application context purely on the Java side? That's what would best match the
rest of my change.

Also, is it actually necessary to set the application context in the mojo shell
and also in the application being loaded? I'm not clear on what's going on here
:)

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java (right):

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68: // THIS
IS SKETCHY: nativeInit treats applicationContext as an Activity!
Mojo folks: The variable on this side is called "applicationContext" - is it
really an app context? The native code this calls into names it "activity".
Mixing these up is bad ;)

[Torne, 2015-11-06 16:51:33]

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

[Torne, 2015-11-06 16:51:34]

+sergeyu for remoting-related question:

https://codereview.chromium.org/1407233017/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/1407233017/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:148: *
THIS IS SKETCHY: the code doesn't update the app context, and the comment
doesn't make sense.
remoting folks: the comment here isn't consistent with the actual function
implementation, it doesn't update the context if you call it with a different
context (it can't, if you do that it would crash on the native side), and the
comment makes it seem like the context here is actually an Activity, which is
not something you are allowed to set as an application context - if "context"
here is an activity then you need to pass down "context.getApplicationContext()"
instead.

Is the comment just outdated and you are actually using the app context here, or
is the code wrong?

[Lambros, 2015-11-06 19:46:02]

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

[Lambros, 2015-11-06 19:46:03]

https://codereview.chromium.org/1407233017/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/1407233017/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:148: *
THIS IS SKETCHY: the code doesn't update the app context, and the comment
doesn't make sense.
On 2015/11/06 16:51:34, Torne wrote:
> remoting folks: the comment here isn't consistent with the actual function
> implementation, it doesn't update the context if you call it with a different
> context (it can't, if you do that it would crash on the native side), and the
> comment makes it seem like the context here is actually an Activity, which is
> not something you are allowed to set as an application context - if "context"
> here is an activity then you need to pass down
"context.getApplicationContext()"
> instead.
> 
> Is the comment just outdated and you are actually using the app context here,
or
> is the code wrong?

The comment seems to be outdated, probably all they way back to the initial
check-in. Sorry for the confusion!

This loadLibrary() function gets called from our main activity's OnCreate(),
passing in the Activity for the Context.

The code looks wrong - we should be passing the Application context to
Chromium's machinery, not the Activity. Looking at the native code, I don't
think we use |context| in any other way, so feel free to fix it to pass the
Application context instead.

The comment here should probably just say something simple like "Loads and
initializes the native code. Called on the UI thread."

I'm not sure whether we should fix the caller to pass in the Application
context, or whether we should do context.getApplicationContext() here? I don't
think it matters either way.

[Torne, 2015-11-09 15:16:20]

On 2015/11/06 19:46:03, Lambros wrote:
> The comment seems to be outdated, probably all they way back to the initial
> check-in. Sorry for the confusion!

No problem, finding this kind of mixup is one reason why I'm trying to move this
to Java where everything is more clear and it's easy to do things like calling
.getApplicationContext() :)

> This loadLibrary() function gets called from our main activity's OnCreate(),
> passing in the Activity for the Context.
> 
> The code looks wrong - we should be passing the Application context to
> Chromium's machinery, not the Activity. Looking at the native code, I don't
> think we use |context| in any other way, so feel free to fix it to pass the
> Application context instead.
> 
> The comment here should probably just say something simple like "Loads and
> initializes the native code. Called on the UI thread."

Updated comment.

> I'm not sure whether we should fix the caller to pass in the Application
> context, or whether we should do context.getApplicationContext() here? I don't
> think it matters either way.

I just used getApplicationContext here, since that's fine to call on something
that's already an app context (it just returns itself).

Does remoting/ look okay now?

[Lambros, 2015-11-09 18:36:03]

remoting/ lgtm, thanks!

[dgn, 2015-11-11 12:29:31]

Description was changed from

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is the sole
place that sets the native-side one.

Remove ApplicationStatus.getApplicationContext() and switch all users to
the new API, as ApplicationStatus's context is not set for all embedders
and ApplicationStatus is ultimately going to be moved out of base.

BUG=552419
==========

to

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is the sole
place that sets the native-side one.

Remove ApplicationStatus.getApplicationContext() and switch all users to
the new API, as ApplicationStatus's context is not set for all embedders
and ApplicationStatus is ultimately going to be moved out of base.

BUG=552419
==========

[Torne, 2015-11-13 14:08:25]

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

[Torne, 2015-11-13 14:08:26]

+qsr who wrote the comment
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/runner/androi...

qsr, can you help answer the questions I posed in this CL? It's *definitely* not
safe to initialise the native app context with an activity :)

[qsr, 2015-11-13 14:36:06]

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
File mojo/runner/android/android_handler.cc (right):

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
mojo/runner/android/android_handler.cc:54: // TODO: what to do about this?
On 2015/11/06 16:43:19, Torne wrote:
> Mojo folks: is it reasonable to remove this mechanism and instead handle the
> application context purely on the Java side? That's what would best match the
> rest of my change.

 Unfortunately, I don't think you can do this.

> Also, is it actually necessary to set the application context in the mojo
shell
> and also in the application being loaded? I'm not clear on what's going on
here
> :)

The code here (in interaction with org.chromium.mojo.shell.AndroidHandler) is
loading a dex file from the web, creating a new classloader that only depends on
the system class loader and not the app class loader. So, in particular, this
code doesn't have access to ApplicationStatus, or anything in base_java. This
mechanism is the way we inject the context to this new class loader.

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java (right):

https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68: // THIS
IS SKETCHY: nativeInit treats applicationContext as an Activity!
On 2015/11/06 16:43:19, Torne wrote:
> Mojo folks: The variable on this side is called "applicationContext" - is it
> really an app context? The native code this calls into names it "activity".
> Mixing these up is bad ;)

Yes, this code is wrong. This code has been forked from the mojo repo which has
evolved a lot since. In the mojo repo, this code is not passing an activity
anymore and this is not a problem anymore. I do not know what is the state of
this code in chromium. ben@ and sky@ will have more info than me, but I guess
that all of this code is just dead, as it is there to have a mojo content
handler for java, and I'm not sure Mandoline plan to use this.

[Torne, 2015-11-13 14:47:19]

On 2015/11/13 14:36:06, qsr wrote:
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> File mojo/runner/android/android_handler.cc (right):
> 
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> mojo/runner/android/android_handler.cc:54: // TODO: what to do about this?
> On 2015/11/06 16:43:19, Torne wrote:
> > Mojo folks: is it reasonable to remove this mechanism and instead handle the
> > application context purely on the Java side? That's what would best match
the
> > rest of my change.
> 
>  Unfortunately, I don't think you can do this.

I don't really understand why not. Can't the context be passed to the java entry
point?

> > Also, is it actually necessary to set the application context in the mojo
> shell
> > and also in the application being loaded? I'm not clear on what's going on
> here
> > :)
> 
> The code here (in interaction with org.chromium.mojo.shell.AndroidHandler) is
> loading a dex file from the web, creating a new classloader that only depends
on
> the system class loader and not the app class loader. So, in particular, this
> code doesn't have access to ApplicationStatus, or anything in base_java. This
> mechanism is the way we inject the context to this new class loader.

What does the code in the new classloader need the context for, though? If it's
using the native JNI mechanisms from base/ then it must have the corresponding
java code from base/ as well to function, no?

>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java
(right):
> 
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68: // THIS
> IS SKETCHY: nativeInit treats applicationContext as an Activity!
> On 2015/11/06 16:43:19, Torne wrote:
> > Mojo folks: The variable on this side is called "applicationContext" - is it
> > really an app context? The native code this calls into names it "activity".
> > Mixing these up is bad ;)
> 
> Yes, this code is wrong. This code has been forked from the mojo repo which
has
> evolved a lot since. In the mojo repo, this code is not passing an activity
> anymore and this is not a problem anymore. I do not know what is the state of
> this code in chromium. ben@ and sky@ will have more info than me, but I guess
> that all of this code is just dead, as it is there to have a mojo content
> handler for java, and I'm not sure Mandoline plan to use this.

[qsr, 2015-11-13 14:56:39]

On 2015/11/13 14:47:19, Torne wrote:
> On 2015/11/13 14:36:06, qsr wrote:
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > File mojo/runner/android/android_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > mojo/runner/android/android_handler.cc:54: // TODO: what to do about this?
> > On 2015/11/06 16:43:19, Torne wrote:
> > > Mojo folks: is it reasonable to remove this mechanism and instead handle
the
> > > application context purely on the Java side? That's what would best match
> the
> > > rest of my change.
> > 
> >  Unfortunately, I don't think you can do this.
> 
> I don't really understand why not. Can't the context be passed to the java
entry
> point?

 There is not java entry point. The entry point is in C++.

> 
> > > Also, is it actually necessary to set the application context in the mojo
> > shell
> > > and also in the application being loaded? I'm not clear on what's going on
> > here
> > > :)
> > 
> > The code here (in interaction with org.chromium.mojo.shell.AndroidHandler)
is
> > loading a dex file from the web, creating a new classloader that only
depends
> on
> > the system class loader and not the app class loader. So, in particular,
this
> > code doesn't have access to ApplicationStatus, or anything in base_java.
This
> > mechanism is the way we inject the context to this new class loader.
> 
> What does the code in the new classloader need the context for, though? If
it's
> using the native JNI mechanisms from base/ then it must have the corresponding
> java code from base/ as well to function, no?

 It needs the context for whatever it wants. The code is not known. Also, it
doesn't use the JNI mechanism from base/, it used standard JNI mechanism.

> 
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java
> (right):
> > 
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68: //
THIS
> > IS SKETCHY: nativeInit treats applicationContext as an Activity!
> > On 2015/11/06 16:43:19, Torne wrote:
> > > Mojo folks: The variable on this side is called "applicationContext" - is
it
> > > really an app context? The native code this calls into names it
"activity".
> > > Mixing these up is bad ;)
> > 
> > Yes, this code is wrong. This code has been forked from the mojo repo which
> has
> > evolved a lot since. In the mojo repo, this code is not passing an activity
> > anymore and this is not a problem anymore. I do not know what is the state
of
> > this code in chromium. ben@ and sky@ will have more info than me, but I
guess
> > that all of this code is just dead, as it is there to have a mojo content
> > handler for java, and I'm not sure Mandoline plan to use this.

[Torne, 2015-11-13 15:05:36]

On 2015/11/13 14:56:39, qsr wrote:
> On 2015/11/13 14:47:19, Torne wrote:
> > On 2015/11/13 14:36:06, qsr wrote:
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > > File mojo/runner/android/android_handler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > > mojo/runner/android/android_handler.cc:54: // TODO: what to do about this?
> > > On 2015/11/06 16:43:19, Torne wrote:
> > > > Mojo folks: is it reasonable to remove this mechanism and instead handle
> the
> > > > application context purely on the Java side? That's what would best
match
> > the
> > > > rest of my change.
> > > 
> > >  Unfortunately, I don't think you can do this.
> > 
> > I don't really understand why not. Can't the context be passed to the java
> entry
> > point?
> 
>  There is not java entry point. The entry point is in C++.

Well, there is a java entry point: Bootstrap. You can't have a C++ entry point
for a JNI program by definition; something has to call System.loadLibrary on the
java side.

> > 
> > > > Also, is it actually necessary to set the application context in the
mojo
> > > shell
> > > > and also in the application being loaded? I'm not clear on what's going
on
> > > here
> > > > :)
> > > 
> > > The code here (in interaction with org.chromium.mojo.shell.AndroidHandler)
> is
> > > loading a dex file from the web, creating a new classloader that only
> depends
> > on
> > > the system class loader and not the app class loader. So, in particular,
> this
> > > code doesn't have access to ApplicationStatus, or anything in base_java.
> This
> > > mechanism is the way we inject the context to this new class loader.
> > 
> > What does the code in the new classloader need the context for, though? If
> it's
> > using the native JNI mechanisms from base/ then it must have the
corresponding
> > java code from base/ as well to function, no?
> 
>  It needs the context for whatever it wants. The code is not known. Also, it
> doesn't use the JNI mechanism from base/, it used standard JNI mechanism.

Well, some of the mojo apps do use the JNI mechanism from base (which is why
they call base::android::InitApplicationContext), and my CL changes the way to
initialise that to be a Java-side call, so it seems like it's the problem of
those apps to call that on the java side one way or another.

> > 
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > > File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > > mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68: //
> THIS
> > > IS SKETCHY: nativeInit treats applicationContext as an Activity!
> > > On 2015/11/06 16:43:19, Torne wrote:
> > > > Mojo folks: The variable on this side is called "applicationContext" -
is
> it
> > > > really an app context? The native code this calls into names it
> "activity".
> > > > Mixing these up is bad ;)
> > > 
> > > Yes, this code is wrong. This code has been forked from the mojo repo
which
> > has
> > > evolved a lot since. In the mojo repo, this code is not passing an
activity
> > > anymore and this is not a problem anymore. I do not know what is the state
> of
> > > this code in chromium. ben@ and sky@ will have more info than me, but I
> guess
> > > that all of this code is just dead, as it is there to have a mojo content
> > > handler for java, and I'm not sure Mandoline plan to use this.

[qsr, 2015-11-13 15:09:01]

On 2015/11/13 15:05:36, Torne wrote:
> On 2015/11/13 14:56:39, qsr wrote:
> > On 2015/11/13 14:47:19, Torne wrote:
> > > On 2015/11/13 14:36:06, qsr wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > > > File mojo/runner/android/android_handler.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/android...
> > > > mojo/runner/android/android_handler.cc:54: // TODO: what to do about
this?
> > > > On 2015/11/06 16:43:19, Torne wrote:
> > > > > Mojo folks: is it reasonable to remove this mechanism and instead
handle
> > the
> > > > > application context purely on the Java side? That's what would best
> match
> > > the
> > > > > rest of my change.
> > > > 
> > > >  Unfortunately, I don't think you can do this.
> > > 
> > > I don't really understand why not. Can't the context be passed to the java
> > entry
> > > point?
> > 
> >  There is not java entry point. The entry point is in C++.
> 
> Well, there is a java entry point: Bootstrap. You can't have a C++ entry point
> for a JNI program by definition; something has to call System.loadLibrary on
the
> java side.
> 
> > > 
> > > > > Also, is it actually necessary to set the application context in the
> mojo
> > > > shell
> > > > > and also in the application being loaded? I'm not clear on what's
going
> on
> > > > here
> > > > > :)
> > > > 
> > > > The code here (in interaction with
org.chromium.mojo.shell.AndroidHandler)
> > is
> > > > loading a dex file from the web, creating a new classloader that only
> > depends
> > > on
> > > > the system class loader and not the app class loader. So, in particular,
> > this
> > > > code doesn't have access to ApplicationStatus, or anything in base_java.
> > This
> > > > mechanism is the way we inject the context to this new class loader.
> > > 
> > > What does the code in the new classloader need the context for, though? If
> > it's
> > > using the native JNI mechanisms from base/ then it must have the
> corresponding
> > > java code from base/ as well to function, no?
> > 
> >  It needs the context for whatever it wants. The code is not known. Also, it
> > doesn't use the JNI mechanism from base/, it used standard JNI mechanism.
> 
> Well, some of the mojo apps do use the JNI mechanism from base (which is why
> they call base::android::InitApplicationContext), and my CL changes the way to
> initialise that to be a Java-side call, so it seems like it's the problem of
> those apps to call that on the java side one way or another.

 Yes. They should call your new mechanism through JNI in their
InitApplicationContext() method.

> 
> > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > > > File mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1407233017/diff/1/mojo/runner/android/apk/src...
> > > > mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java:68:
//
> > THIS
> > > > IS SKETCHY: nativeInit treats applicationContext as an Activity!
> > > > On 2015/11/06 16:43:19, Torne wrote:
> > > > > Mojo folks: The variable on this side is called "applicationContext" -
> is
> > it
> > > > > really an app context? The native code this calls into names it
> > "activity".
> > > > > Mixing these up is bad ;)
> > > > 
> > > > Yes, this code is wrong. This code has been forked from the mojo repo
> which
> > > has
> > > > evolved a lot since. In the mojo repo, this code is not passing an
> activity
> > > > anymore and this is not a problem anymore. I do not know what is the
state
> > of
> > > > this code in chromium. ben@ and sky@ will have more info than me, but I
> > guess
> > > > that all of this code is just dead, as it is there to have a mojo
content
> > > > handler for java, and I'm not sure Mandoline plan to use this.

[Torne, 2015-11-13 15:23:23]

On 2015/11/13 15:09:01, qsr wrote:
>  Yes. They should call your new mechanism through JNI in their
> InitApplicationContext() method.

Well, that answers the question of what to do here, but wearing my hat as an
owner of base/android this then makes the base/android API inconvenient to use
if we actually intend to support it being consumed by programs whose
initialisation logic is all in C++, because they'd all have to implement a Java
call to ContextUtils.

The point of removing the native InitApplicationContext in the first place was
to ensure that the context was initialised early enough that Java code can
always assume it's set and correct, on the assumption that Java is what comes
first and C++ comes later. I suppose it doesn't really matter which side this
comes from really, as long as they are never different (i.e. JNI goes both ways)
and it doesn't change in the lifetime of a program (already asserted), because
if someone tries to use it too early they'll just crash. That would actually
make the native test runner change in this CL simpler, as it could keep setting
it from native and not need its JNI init sequence to be changed.

[Torne, 2015-11-20 13:43:55]

Description was changed from

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is the sole
place that sets the native-side one.

Remove ApplicationStatus.getApplicationContext() and switch all users to
the new API, as ApplicationStatus's context is not set for all embedders
and ApplicationStatus is ultimately going to be moved out of base.

BUG=552419
==========

to

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Remove ApplicationStatus.getApplicationContext() and switch all users to
the new API, as ApplicationStatus's context is not set for all embedders
and ApplicationStatus is ultimately going to be moved out of base.

BUG=552419
==========

[Torne, 2015-11-20 13:45:19]

Description was changed from

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Remove ApplicationStatus.getApplicationContext() and switch all users to
the new API, as ApplicationStatus's context is not set for all embedders
and ApplicationStatus is ultimately going to be moved out of base.

BUG=552419
==========

to

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Make ApplicationStatus.getApplicationContext use the new API for now,
until all callers are fixed to use ContextUtils instead, as part of the
plan to remove ApplicationStatus from base.

BUG=552419
==========

[Torne, 2015-11-20 13:55:36]

torne@chromium.org changed reviewers:
+ mef@chromium.org, nyquist@chromium.org, thakis@chromium.org,
yfriedman@chromium.org
- ben@chromium.org, sergeyu@chromium.org

[Torne, 2015-11-20 13:55:36]

Okay, have changed the CL to keep the native-side initialisation option so that
mojo applications and native tests can continue using it. Both java and native
init functions set the context for both sides, so only one needs to be called,
and I've switched to the java version where this is possible as it simplifies
most cases.

Ready to submit now, and description updated.

OWNERS roundup:

thakis: base/
nyquist: blimp/
mef: components/cronet/
yfriedman: content/
sky: mojo/
lambroslambrou: remoting/

[mef, 2015-11-20 14:45:54]

On 2015/11/20 13:55:36, Torne wrote:
> Okay, have changed the CL to keep the native-side initialisation option so
that
> mojo applications and native tests can continue using it. Both java and native
> init functions set the context for both sides, so only one needs to be called,
> and I've switched to the java version where this is possible as it simplifies
> most cases.
> 
> Ready to submit now, and description updated.
> 
> OWNERS roundup:
> 
> thakis: base/
> nyquist: blimp/
> mef: components/cronet/
> yfriedman: content/
> sky: mojo/
> lambroslambrou: remoting/

components/cronet/ lgtm

[nyquist, 2015-11-20 17:54:58]

blimp lgtm

[Nico, 2015-11-20 21:23:43]

base/ lgtm

[Torne, 2015-11-23 15:22:31]

Hm, some of the Java initialisation in Chrome relies on being able to cast
ApplicationStatus.getApplicationContext() to ChromeApplication, which is what's
causing the browser to crash on the bot.

I'll look at fixing that up in a separate CL and land that first, since it's not
directly related to this and it looks like it's easy to fix to just not depend
it.

[Yaron, 2015-11-23 15:28:43]

On 2015/11/23 15:22:31, Torne wrote:
> Hm, some of the Java initialisation in Chrome relies on being able to cast
> ApplicationStatus.getApplicationContext() to ChromeApplication, which is
what's
> causing the browser to crash on the bot.
> 
> I'll look at fixing that up in a separate CL and land that first, since it's
not
> directly related to this and it looks like it's easy to fix to just not depend
> it.

Are you talking about chrome/ layer or below? We have a lot of code that depends
on being able to convert app context to ChromeApplication. I suppose you could
introduce ChromeApplication.getForContext() - is that what you had in mind?

[Torne, 2015-11-23 15:31:37]

On 2015/11/23 15:28:43, Yaron wrote:
> On 2015/11/23 15:22:31, Torne wrote:
> > Hm, some of the Java initialisation in Chrome relies on being able to cast
> > ApplicationStatus.getApplicationContext() to ChromeApplication, which is
> what's
> > causing the browser to crash on the bot.
> > 
> > I'll look at fixing that up in a separate CL and land that first, since it's
> not
> > directly related to this and it looks like it's easy to fix to just not
depend
> > it.
> 
> Are you talking about chrome/ layer or below? We have a lot of code that
depends
> on being able to convert app context to ChromeApplication. I suppose you could
> introduce ChromeApplication.getForContext() - is that what you had in mind?

chrome/ layer. There don't seem to be that many places, and they all look pretty
bad (incorrect singleton creation, etc). It looks like they should be fixable by
just passing the actual activity down instead of relying on it being a global. I
don't want to introduce even more global getter functions.

[Torne, 2015-11-23 15:36:32]

On 2015/11/23 15:31:37, Torne wrote:
> On 2015/11/23 15:28:43, Yaron wrote:
> > On 2015/11/23 15:22:31, Torne wrote:
> > > Hm, some of the Java initialisation in Chrome relies on being able to cast
> > > ApplicationStatus.getApplicationContext() to ChromeApplication, which is
> > what's
> > > causing the browser to crash on the bot.
> > > 
> > > I'll look at fixing that up in a separate CL and land that first, since
it's
> > not
> > > directly related to this and it looks like it's easy to fix to just not
> depend
> > > it.
> > 
> > Are you talking about chrome/ layer or below? We have a lot of code that
> depends
> > on being able to convert app context to ChromeApplication. I suppose you
could
> > introduce ChromeApplication.getForContext() - is that what you had in mind?
> 
> chrome/ layer. There don't seem to be that many places, and they all look
pretty
> bad (incorrect singleton creation, etc). It looks like they should be fixable
by
> just passing the actual activity down instead of relying on it being a global.
I
> don't want to introduce even more global getter functions.

Hm, no, there are more than I found at first :/

This is a really bad pattern that makes the code very unpleasant. We really
shouldn't attach things to the Application at all.

[Torne, 2015-11-23 15:42:42]

Hm, probably the easiest option is to leave ApplicationStatus alone in this CL
and then just carry on and change all non-chrome-layer stuff to not depend on it
any more, and then move it to chrome, where if people really want to keep
casting the context to ChromeApplication they can..

[Torne, 2015-11-23 15:50:09]

Description was changed from

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Make ApplicationStatus.getApplicationContext use the new API for now,
until all callers are fixed to use ContextUtils instead, as part of the
plan to remove ApplicationStatus from base.

BUG=552419
==========

to

==========
Define a Java-side global application context.

Instead of each user of base setting the native-side global app context
separately, introduce a Java-side global app context, which is always
in sync with the native-side one. Switch most callers to setting
it on the Java side, except where this is problematic.

Callers of ApplicationStatus.getApplicationContext will be updated
incrementally in followup CLs once it's been verified that they only
require a Context and not a BaseChromiumApplication.

BUG=552419
==========

[Torne, 2015-11-23 15:51:30]

Updated CL and description to not change
ApplicationStatus.getApplicationContext, which should unbreak chrome_public
startup. Does the change in general (and content/ specifically) look okay now
Yaron?

Also, sky/ben: ping, still need a review for mojo/

[Torne, 2015-11-23 15:51:31]

Updated CL and description to not change
ApplicationStatus.getApplicationContext, which should unbreak chrome_public
startup. Does the change in general (and content/ specifically) look okay now
Yaron?

Also, sky/ben: ping, still need a review for mojo/

[sky, 2015-11-23 15:59:20]

mojo LGTM

[Yaron, 2015-11-23 20:35:16]

lgtm

https://codereview.chromium.org/1407233017/diff/120001/blimp/client/android/b...
File blimp/client/android/blimp_library_loader.cc (right):

https://codereview.chromium.org/1407233017/diff/120001/blimp/client/android/b...
blimp/client/android/blimp_library_loader.cc:64: // TODO(dtrainor): Start the
runner?
this is a pretty awesome function now :)

https://codereview.chromium.org/1407233017/diff/120001/content/public/android...
File content/public/android/java/src/org/chromium/content/app/ContentMain.java
(right):

https://codereview.chromium.org/1407233017/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ContentMain.java:27:
public static void initApplicationContext(Context context) {
I'd probably just inline this into callers (fine here or separate CL).

[Lambros, 2015-11-23 21:03:18]

remoting/ lgtm

[Torne, 2015-11-24 10:22:34]

https://codereview.chromium.org/1407233017/diff/120001/blimp/client/android/b...
File blimp/client/android/blimp_library_loader.cc (right):

https://codereview.chromium.org/1407233017/diff/120001/blimp/client/android/b...
blimp/client/android/blimp_library_loader.cc:64: // TODO(dtrainor): Start the
runner?
On 2015/11/23 20:35:16, Yaron wrote:
> this is a pretty awesome function now :)

Well if it's not needed dtrainor can remove it ;)

https://codereview.chromium.org/1407233017/diff/120001/content/public/android...
File content/public/android/java/src/org/chromium/content/app/ContentMain.java
(right):

https://codereview.chromium.org/1407233017/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ContentMain.java:27:
public static void initApplicationContext(Context context) {
On 2015/11/23 20:35:16, Yaron wrote:
> I'd probably just inline this into callers (fine here or separate CL).

Oh, yeah, I was going to do this but didn't get around to it. I'll do it in a
later CL since this one has all the OWNERS.

[Torne, 2015-11-24 10:22:56]

The CQ bit was checked by torne@chromium.org

[Torne, 2015-11-24 10:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
nyquist@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1407233017/#ps120001
(title: "Undo changes to ApplicationStatus")

[commit-bot: I haz the power, 2015-11-24 10:24:09]

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

[commit-bot: I haz the power, 2015-11-24 10:31:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-24 10:32:02]

Patchset 7 (id:??) landed as
https://crrev.com/961a488f6a5d8c82b576444054f5d26b3f8a6877
Cr-Commit-Position: refs/heads/master@{#361306}