Closed Bug 1445420 Opened 6 years ago Closed 6 years ago

Add GeckoRuntime for Gecko setup and initialization

Categories

(GeckoView :: General, enhancement, P1)

51 Branch
All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(3 files, 22 obsolete files)

16.92 KB, patch
esawin
: review+
Details | Diff | Splinter Review
14.45 KB, patch
esawin
: review+
Details | Diff | Splinter Review
27.68 KB, patch
esawin
: review+
Details | Diff | Splinter Review
We should have a coherent API for Gecko setup and initialization.
WIP: this is the base idea for the GeckoRuntime. It allows us to initialize Gecko with provided GeckoSettings and to track Gecko state (onReady and onShutdown).

GeckoSettings may be controlled through getSettings() at runtime.

Things we need to discuss include:
1. How do we want to handle Gecko state/config suspension and restoring?
2. Do we want to make explicit initialization mandatory (currently it's called in GeckoSession.open())?
3. Do we want to mimic the generic GeckoSessionSettings for GeckoSettings?
4. What else should we move into GeckoRuntime?
Attachment #8958596 - Flags: feedback?(snorp)
Comment on attachment 8958596 [details] [diff] [review]
0001-Bug-1445420-1.0-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8958596 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good. Needs tests :)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +531,5 @@
>          final boolean result = sNativeQueue.checkAndSetState(expectedState, newState);
>          if (result) {
>              Log.d(LOGTAG, "State changed to " + newState);
> +            if (newState == State.RUNNING) {
> +                EventDispatcher.getInstance().dispatch("Gecko:Running", null);

Maybe send Gecko:ThreadState with the state on every change?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +27,5 @@
> +
> +    private static GeckoSettings sSettings;
> +
> +    public interface StateCallback {
> +        void onReady();

I think we should just have onStateChange() which follows those in GeckoThread. Maybe just NOT_RUNNING, STARTING, RUNNING, EXITING? We definitely do not want to mirror the GeckoThread states.

@@ +37,5 @@
> +            sCallbacks.add(callback);
> +        }
> +    }
> +
> +    public static synchronized void init(final @NonNull Context context,

If you call init() when we're already initialized the settings won't be applied, right? Should we throw in that case?

@@ +54,5 @@
> +        }
> +
> +        EventDispatcher.getInstance().registerUiThreadListener(sListener,
> +            "Gecko:Exited",
> +            "Gecko:Running",

Yeah, this should just be a single Gecko:ThreadState message

@@ +82,5 @@
> +    private static final ArrayList<StateCallback> sCallbacks =
> +        new ArrayList<StateCallback>();
> +
> +    private static EventListener sListener = new EventListener();
> +    

whitespace

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +659,5 @@
>          }
>  
>          if (appContext != null) {
> +            GeckoSettings settings = new GeckoSettings();
> +            settings.setMultiprocessHint(

We should probably just make this the default in GeckoSettings(), no?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSettings.java
@@ +24,5 @@
> +    public Bundle getExtras() {
> +        return mExtras;
> +    }
> +
> +    public void setMultiprocessHint(boolean multiprocess) {

Now is maybe a good time to get away from the "multiprocess" naming. Something more specific...maybe setUseContentProcess()?
Attachment #8958596 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8958596 [details] [diff] [review]
0001-Bug-1445420-1.0-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8958596 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +659,5 @@
>          }
>  
>          if (appContext != null) {
> +            GeckoSettings settings = new GeckoSettings();
> +            settings.setMultiprocessHint(

Actually this is fine, I'm an idiot...
As discussed, this is how an instance-based runtime could work.
With this, we remove the assumption that there is only one runtime and allow more control over session creation.

For backward-compatibility, we provide a default runtime for Fennec and some hopefully rare special cases like a GeckoView unattached to a session.
Attachment #8958596 - Attachment is obsolete: true
Attachment #8959280 - Flags: feedback?(snorp)
Separated default runtime handling.
Attachment #8959280 - Attachment is obsolete: true
Attachment #8959280 - Flags: feedback?(snorp)
Attachment #8959310 - Flags: feedback?(snorp)
Attachment #8959281 - Attachment is obsolete: true
Attachment #8959281 - Flags: feedback?(snorp)
Attachment #8959311 - Flags: feedback?(snorp)
Fixed parceling error.
Attachment #8959310 - Attachment is obsolete: true
Attachment #8959310 - Flags: feedback?(snorp)
Attachment #8959346 - Flags: feedback?(snorp)
Switched order of init and attachTo.
Attachment #8959311 - Attachment is obsolete: true
Attachment #8959311 - Flags: feedback?(snorp)
Attachment #8959347 - Flags: feedback?(snorp)
Blocks: 1437549
Blocks: 1437551
Comment on attachment 8959346 [details] [diff] [review]
0001-Bug-1445420-1.1-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8959346 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is looking pretty good

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +536,5 @@
> +                final GeckoBundle state = new GeckoBundle(1);
> +                state.putInt("state", (newState == State.RUNNING
> +                                       ? GeckoRuntime.StateListener.STATE_READY
> +                                       : GeckoRuntime.StateListener.STATE_SHUTDOWN));
> +                EventDispatcher.getInstance().dispatch("Gecko:ThreadState", state);

Since there is a 1:1 relationship between a GeckoRuntime and a GeckoThread, and the GeckoRuntime *owns* the GeckoThread, can this just be a direct call on the GeckoRuntime?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +42,5 @@
> +    public void addStateListener(final @NonNull StateListener listener) {
> +        mStateListeners.add(listener);
> +    }
> +
> +    /* package */ void attachTo(final @NonNull Context context) {

Can this just be part of the constructor?

@@ +45,5 @@
> +
> +    /* package */ void attachTo(final @NonNull Context context) {
> +        Log.d(LOGTAG, "attachTo " + context.getApplicationContext());
> +        final Context appContext = context.getApplicationContext();
> +        if (!appContext.equals(GeckoAppShell.getApplicationContext())) {

If appContext is non-null, should this throw? Currently I can't think of a valid reason for this to occur, though maybe it's possible for the app context to change during the lifetime of an app?

@@ +75,5 @@
> +                                        settings.getArgs(),
> +                                        settings.getExtras(),
> +                                        flags)) {
> +            GeckoThread.launch();
> +            mSettings = new GeckoSettings(settings);

why not just assign the settings directly?
Attachment #8959346 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8959347 [details] [diff] [review]
0002-Bug-1445420-2.0-Add-default-runtime-fallback-for-Fen.patch

Review of attachment 8959347 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +41,5 @@
>  
> +    private static GeckoRuntime sDefaultRuntime;
> +
> +    public static synchronized @NonNull GeckoRuntime getDefault(
> +            final @NonNull Context context) {

If we already have sDefaultRuntime we should probably throw if it has a context different from the one passed in (though we should compare context.getApplicationContext())
Attachment #8959347 - Flags: feedback?(snorp) → feedback+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> Comment on attachment 8959346 [details] [diff] [review]
> 0001-Bug-1445420-1.1-Add-GeckoRuntime-and-GeckoSettings-f.patch
> 
> Review of attachment 8959346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is looking pretty good
> 
> ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
> @@ +536,5 @@
> > +                final GeckoBundle state = new GeckoBundle(1);
> > +                state.putInt("state", (newState == State.RUNNING
> > +                                       ? GeckoRuntime.StateListener.STATE_READY
> > +                                       : GeckoRuntime.StateListener.STATE_SHUTDOWN));
> > +                EventDispatcher.getInstance().dispatch("Gecko:ThreadState", state);
> 
> Since there is a 1:1 relationship between a GeckoRuntime and a GeckoThread,
> and the GeckoRuntime *owns* the GeckoThread, can this just be a direct call
> on the GeckoRuntime?

GeckoThread lives in o.m.gecko, we would need to expose internal methods in GeckoRuntime for that.

> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> java
> @@ +42,5 @@
> > +    public void addStateListener(final @NonNull StateListener listener) {
> > +        mStateListeners.add(listener);
> > +    }
> > +
> > +    /* package */ void attachTo(final @NonNull Context context) {
> 
> Can this just be part of the constructor?

For Parcelable support we need to separate attaching and construction, as the former will happen when the window is opened.

> @@ +45,5 @@
> > +
> > +    /* package */ void attachTo(final @NonNull Context context) {
> > +        Log.d(LOGTAG, "attachTo " + context.getApplicationContext());
> > +        final Context appContext = context.getApplicationContext();
> > +        if (!appContext.equals(GeckoAppShell.getApplicationContext())) {
> 
> If appContext is non-null, should this throw? Currently I can't think of a
> valid reason for this to occur, though maybe it's possible for the app
> context to change during the lifetime of an app?

I'm not sure about changing contexts, but I would prefer to keep behavior changes at a minimum with the introduction of the runtime.

> @@ +75,5 @@
> > +                                        settings.getArgs(),
> > +                                        settings.getExtras(),
> > +                                        flags)) {
> > +            GeckoThread.launch();
> > +            mSettings = new GeckoSettings(settings);
> 
> why not just assign the settings directly?

We want a copy of the settings, which is consistent with how we handle passing GeckoSessionSettings.
One thing we will also need to do is make sure GeckoRuntime is initialized in child processes. Something will need to be done here: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java#63
Attachment #8959346 - Attachment is obsolete: true
Attachment #8961492 - Flags: review?(snorp)
Attachment #8959347 - Attachment is obsolete: true
Attachment #8961493 - Flags: review?(snorp)
Comment on attachment 8961492 [details] [diff] [review]
0001-Bug-1445420-1.2-Add-GeckoRuntime-and-GeckoSettings-f.patch

I would like some input on the GeckoRuntime API (look for the GeckoViewActivity.java for an example).

From the app's perspective, how feasible/convenient would it be to keep a runtime instance around for each location you need to create a new session?

The new API for session creation looks like this:
  GeckoRuntime runtime = GeckoRuntime.create([Gecko settings]);
  GeckoSession session = runtime.createSession([GeckoSesssion settings]);
Attachment #8961492 - Flags: feedback?(rbarker)
Attachment #8961492 - Flags: feedback?(ekager)
Comment on attachment 8961492 [details] [diff] [review]
0001-Bug-1445420-1.2-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8961492 [details] [diff] [review]:
-----------------------------------------------------------------

You need to fix the tests, otherwise looking pretty good.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +541,5 @@
> +                final GeckoBundle state = new GeckoBundle(1);
> +                state.putInt("state", (newState == State.RUNNING
> +                                       ? GeckoRuntime.StateListener.STATE_READY
> +                                       : GeckoRuntime.StateListener.STATE_SHUTDOWN));
> +                EventDispatcher.getInstance().dispatch("Gecko:ThreadState", state);

We could let GeckoRuntime register a callback on the GeckoThread. Maybe it just passes this into GeckoThread.launch? As you can tell, I'm not a big fan of this decoupled messaging when we don't need it.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSettings.java
@@ +57,5 @@
> +     * Note: Only for internal use.
> +     *
> +     * @return The Gecko intent extras.
> +     */
> +    public Bundle getExtras() {

Do we use this for stuff other than the environment variables? Can we rename this to get/setEnvironmentVariables?

@@ +76,5 @@
> +     * Note: Only for internal use.
> +     *
> +     * @param args The Gecko process arguments.
> +     */
> +    public void setArgs(final String[] args) {

setArguments is maybe a little nicer
Attachment #8961492 - Flags: review?(snorp) → review-
Attachment #8961493 - Flags: review?(snorp) → review+
Comment on attachment 8961495 [details] [diff] [review]
0003-Bug-1445420-3.0-Add-GeckoChildRuntime-and-GeckoChild.patch

Review of attachment 8961495 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we need GeckoChildRuntime or GeckoChildSettings. We can just use GeckoRuntime/GeckoSettings. We would just ship the GeckoSettings from the parent (currently always those of the default GeckoRuntime) across to GeckoServiceChildProcess.start(), and it will init the default runtime there with the passed settings.
Attachment #8961495 - Flags: review?(snorp) → review-
Comment on attachment 8961493 [details] [diff] [review]
0002-Bug-1445420-2.1-Add-default-runtime-fallback-for-Fen.patch

Review of attachment 8961493 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +38,5 @@
> +     *       For regular apps, use create() and createSession() instead.
> +     *
> +     * @return The (static) default runtime for the context.
> +     */
> +    public static synchronized @NonNull GeckoRuntime getDefault(

I think maybe we also want createDefault(), which is like create() except it also sets the resulting GeckoRuntime as the default if none exists. It would throw if one already exists.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #19)
> I don't think we need GeckoChildRuntime or GeckoChildSettings. We can just
> use GeckoRuntime/GeckoSettings. We would just ship the GeckoSettings from
> the parent (currently always those of the default GeckoRuntime) across to
> GeckoServiceChildProcess.start(), and it will init the default runtime there
> with the passed settings.

There are some advantages for having a dedicated child runtime:
1. There is no overlap (except for args and extras) of settings between child and parent.
2. We don't expose the child setup in the public API by putting GeckoChildRuntime into o.m.gecko.

Also, I can further reduce GeckoChildRuntime since we don't need to expose state and some of the overloaded methods for internal use.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #20)
> Comment on attachment 8961493 [details] [diff] [review]
> 0002-Bug-1445420-2.1-Add-default-runtime-fallback-for-Fen.patch
> 
> Review of attachment 8961493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> java
> @@ +38,5 @@
> > +     *       For regular apps, use create() and createSession() instead.
> > +     *
> > +     * @return The (static) default runtime for the context.
> > +     */
> > +    public static synchronized @NonNull GeckoRuntime getDefault(
> 
> I think maybe we also want createDefault(), which is like create() except it
> also sets the resulting GeckoRuntime as the default if none exists. It would
> throw if one already exists.

To allow passing custom settings? That's not actually a use case we have currently. The idea is to never use the default runtime, unless you are in a situation where you don't have a choice (Fennec or no GeckoSession).
(In reply to Eugen Sawin [:esawin] from comment #22)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #20)
> > Comment on attachment 8961493 [details] [diff] [review]
> > 0002-Bug-1445420-2.1-Add-default-runtime-fallback-for-Fen.patch
> > 
> > Review of attachment 8961493 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > :::
> > mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> > java
> > @@ +38,5 @@
> > > +     *       For regular apps, use create() and createSession() instead.
> > > +     *
> > > +     * @return The (static) default runtime for the context.
> > > +     */
> > > +    public static synchronized @NonNull GeckoRuntime getDefault(
> > 
> > I think maybe we also want createDefault(), which is like create() except it
> > also sets the resulting GeckoRuntime as the default if none exists. It would
> > throw if one already exists.
> 
> To allow passing custom settings? That's not actually a use case we have
> currently. The idea is to never use the default runtime, unless you are in a
> situation where you don't have a choice (Fennec or no GeckoSession).

But Fennec needs to use a non-default runtime in order to pass arguments and environment variables for tests. It's probably not going to use a custom runtime to create the GeckoSession it [ab]uses, though...
Fixed issues found in tests. Haven't addressed snorp's review comments yet.
Attachment #8961492 - Attachment is obsolete: true
Attachment #8961492 - Flags: feedback?(rbarker)
Attachment #8961492 - Flags: feedback?(ekager)
Comment on attachment 8961495 [details] [diff] [review]
0003-Bug-1445420-3.0-Add-GeckoChildRuntime-and-GeckoChild.patch

Review of attachment 8961495 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoChildRuntime.java
@@ +112,5 @@
> +        @Override
> +        public void handleMessage(final String event, final GeckoBundle message,
> +                                  final EventCallback listener) {
> +            if ("Gecko:ThreadState".equals(event)) {
> +                for (final StateListener l: GeckoChildRuntime.this.mStateListeners) {

I would prefer we never use single letter variable names and even then, avoid lower case 'l' since it looks like a 1 (one).
Since the rebase, I'm getting test timeouts in some load{Data, String} tests and a failure in multipleLoads, although they don't seem related to the runtime.
Attachment #8962472 - Flags: review?(snorp)
Comment on attachment 8962472 [details] [diff] [review]
0004-Bug-1445420-4.0-Adjust-tests-to-run-with-GeckoRuntim.patch

Review of attachment 8962472 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java
@@ +101,5 @@
>          final Intent intent = getIntent();
> +
> +        final GeckoRuntimeSettings geckoSettings = new GeckoRuntimeSettings();
> +        geckoSettings.setArgs(new String[] { "-purgecaches" });
> +        geckoSettings.setExtras(intent.getExtras());

I still think we should have setEnvironmentVariables() or something instead of "here's some random junk in a bundle".

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
@@ +722,5 @@
> +            sRuntime = GeckoRuntime.create(
> +                InstrumentationRegistry.getTargetContext(),
> +                geckoSettings);
> +        }
> +        if (sRuntime == null) {

Can GeckoRuntime.create() return null? That shouldn't be allowed, IMHO. It should throw instead and you can remove this check.
Attachment #8962472 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #27)
> Comment on attachment 8962472 [details] [diff] [review]
> 0004-Bug-1445420-4.0-Adjust-tests-to-run-with-GeckoRuntim.patch
> 
> Review of attachment 8962472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/
> TestRunnerActivity.java
> @@ +101,5 @@
> >          final Intent intent = getIntent();
> > +
> > +        final GeckoRuntimeSettings geckoSettings = new GeckoRuntimeSettings();
> > +        geckoSettings.setArgs(new String[] { "-purgecaches" });
> > +        geckoSettings.setExtras(intent.getExtras());
> 
> I still think we should have setEnvironmentVariables() or something instead
> of "here's some random junk in a bundle".

This is ugly but renaming it won't help with that. Let's handle args and extras in a follow-up, there is a lot we could do to make it nicer, ideally convert useful args and env to actual settings.
Attachment #8961495 - Attachment is obsolete: true
Addressed discussed issues, including
- Removed runtime state listener
- Moved runtime under GeckoView.Window
- GeckoView.open take GeckoRuntime (and should be called before attaching to view)
- Test adjustments
Attachment #8962469 - Attachment is obsolete: true
Attachment #8962469 - Flags: review?(nchen)
Attachment #8963377 - Flags: review?(nchen)
Attachment #8961493 - Attachment is obsolete: true
Attachment #8961493 - Flags: review?(nchen)
Attachment #8963378 - Flags: review?(nchen)
Attachment #8962472 - Attachment is obsolete: true
Attachment #8962472 - Flags: review?(nchen)
Attachment #8963381 - Flags: review?(nchen)
Comment on attachment 8963377 [details] [diff] [review]
0001-Bug-1445420-1.4-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8963377 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +11,5 @@
>  import org.mozilla.gecko.process.GeckoProcessManager;
>  import org.mozilla.gecko.util.GeckoBundle;
>  import org.mozilla.gecko.util.FileUtils;
>  import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.geckoview.GeckoRuntime;

Not needed

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +27,5 @@
> +
> +public final class GeckoRuntime implements Parcelable {
> +    private static final String LOGTAG = "GeckoRuntime";
> +
> +    private GeckoRuntimeSettings mSettings = new GeckoRuntimeSettings();

Feel like we're creating `GeckoRuntimeSettings` copies a little unnecessarily? We should only need to copy the consumer-given settings once or maybe not at all.

@@ +29,5 @@
> +    private static final String LOGTAG = "GeckoRuntime";
> +
> +    private GeckoRuntimeSettings mSettings = new GeckoRuntimeSettings();
> +
> +    /* package */ void attachTo(final @NonNull Context context) {

We might want to make this a public API so consumers can attach an existing runtime to a new application Context.

@@ +30,5 @@
> +
> +    private GeckoRuntimeSettings mSettings = new GeckoRuntimeSettings();
> +
> +    /* package */ void attachTo(final @NonNull Context context) {
> +        Log.d(LOGTAG, "attachTo " + context.getApplicationContext());

if (DEBUG) { ... }

@@ +41,5 @@
> +    /* package */ boolean init() {
> +        return init(mSettings);
> +    }
> +
> +    /* package */ boolean init(final @Nullable GeckoRuntimeSettings settings) {

So GeckoThread can be launched _before_ being initialized (we do this for Fennec after APK update). I think we should support that.

@@ +42,5 @@
> +        return init(mSettings);
> +    }
> +
> +    /* package */ boolean init(final @Nullable GeckoRuntimeSettings settings) {
> +        Log.d(LOGTAG, "init");

if (DEBUG) { ... }

@@ +43,5 @@
> +    }
> +
> +    /* package */ boolean init(final @Nullable GeckoRuntimeSettings settings) {
> +        Log.d(LOGTAG, "init");
> +        if (GeckoThread.isLaunched()) {

Check the result of `GeckoThread.launch()` instead of this check here.

@@ +85,5 @@
> +     */
> +    public static @NonNull GeckoRuntime create(
> +          final @NonNull Context context,
> +          final @Nullable GeckoRuntimeSettings settings) {
> +        Log.d(LOGTAG, "create " + context);

if (DEBUG) { ... }

@@ +91,5 @@
> +        final GeckoRuntime runtime = new GeckoRuntime();
> +        runtime.attachTo(context);
> +
> +        if (!runtime.init(new GeckoRuntimeSettings(settings))) {
> +            throw new IllegalStateException("create failed due to init failure");

Maybe just let `init` throw. You'll get better exception messages.

@@ +101,5 @@
> +    /**
> +     * Shutdown the runtime. This will invalidate all attached sessions.
> +     */
> +    public void shutdown() {
> +        Log.d(LOGTAG, "shutdown");

if (DEBUG) { ... }

@@ +127,5 @@
> +        @Override
> +        public GeckoRuntime createFromParcel(final Parcel in) {
> +            final GeckoRuntime runtime = new GeckoRuntime();
> +            runtime.readFromParcel(in);
> +            runtime.init();

I don't think we should `init` here, because presumably the source runtime has already been initialized.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +14,5 @@
> +
> +public final class GeckoRuntimeSettings implements Parcelable {
> +    private boolean mUseContentProcess;
> +    private String[] mArgs = new String[0];
> +    private Bundle mExtras = new Bundle();

Only initialize these to empty objects for the GeckoRuntimeSettings() constructor, to avoid creating unnecessary objects.

@@ +30,5 @@
> +            mArgs = settings.mArgs.clone();
> +            if (settings.mExtras != null) {
> +                mExtras = new Bundle(settings.mExtras);
> +            }
> +            return;

Not needed

@@ +45,5 @@
> +    }
> +
> +    /**
> +     * Get the custom Gecko process arguments.
> +     * Note: Only for internal use.

What does "internal use" mean? I think a consumer could want to pass in arguments.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +190,5 @@
>                                  if (session.isOpen()) {
>                                      throw new IllegalArgumentException("Must use an unopened GeckoSession instance");
>                                  }
>  
> +                                session.open(GeckoSession.this.mWindow.mRuntime);

Check that mWindow is not null.

@@ +405,5 @@
>  
>      private final Listener mListener = new Listener();
>  
>      /* package */ static final class Window extends JNIObject implements IInterface {
> +        private GeckoRuntime mRuntime;

public final GeckoRuntime runtime;

@@ +661,5 @@
>       * is opened here. Opening a session is an asynchronous operation. You can check
>       * the current state via isOpen().
>       *
> +     * Call this when you are ready to use a GeckoSession instance before
> +     * attaching to a view.

This should go to the GeckoView documentation instead of here.
Attachment #8963377 - Flags: review?(nchen) → feedback+
Comment on attachment 8963378 [details] [diff] [review]
0002-Bug-1445420-2.2-Add-default-runtime-fallback-for-Fen.patch

Review of attachment 8963378 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1076,5 @@
>          final GeckoSession session = new GeckoSession();
> +        session.getSettings().setString(GeckoSessionSettings.CHROME_URI,
> +                                        "chrome://browser/content/browser.xul");
> +        session.setContentDelegate(this);
> +        session.open(GeckoRuntime.getDefault(this));

Hm I see a problem with requiring the consumer to call open here: this is non-optimal when we're restoring states, because the window created here will be replaced by the existing window during restore.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +27,5 @@
>  
>  public final class GeckoRuntime implements Parcelable {
>      private static final String LOGTAG = "GeckoRuntime";
>  
> +    private static GeckoRuntime sDefaultRuntime;

I thought we decided to let GeckoView keep the default runtime?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +253,5 @@
>  
>      public EventDispatcher getEventDispatcher() {
>          return mSession.getEventDispatcher();
>      }
>  

I think this shortcut is nice to have. Otherwise we should remove all these shortcuts (like `getEventDispatcher()` above).
Attachment #8963378 - Flags: review?(nchen) → feedback+
Comment on attachment 8963381 [details] [diff] [review]
0004-Bug-1445420-4.1-Adjust-tests-to-run-with-GeckoRuntim.patch

Review of attachment 8963381 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoSessionTestRuleTest.kt
@@ +675,5 @@
>          assertThat("Wait delegate should be cleared", waitCounter, equalTo(2))
>      }
>  
>      @Test fun wrapSession() {
> +        val session = sessionRule.wrapSession(sessionRule.createClosedSession(sessionRule.session.settings))

Use the raw constructor instead of `createClosedSession`, because `createClosedSession` already wraps the session so we wouldn't be directly testing `wrapSession`.

@@ +757,5 @@
>      }
>  
>      @Test(expected = AssertionError::class)
>      fun waitForPageStop_throwOnNotWrapped() {
> +        sessionRule.createClosedSession(sessionRule.session.settings).waitForPageStop()

Same as above.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +358,1 @@
>                  newSession.open()

This calls `GeckoSession.open` with a null argument, but with your other changes, this should be changed as well.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java
@@ +121,5 @@
>  
>      @Override
>      protected void onDestroy() {
>          mSession.close();
> +        mRuntime.shutdown();

Probably don't want this, because it will prevent tests form running one after another.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
@@ +515,5 @@
>      protected final GeckoSessionSettings mDefaultSettings;
>      protected final Set<GeckoSession> mSubSessions = new HashSet<>();
>  
>      protected ErrorCollector mErrorCollector;
> +    protected static GeckoRuntime sRuntime;;

Move this together with other static fields, and remove extra semicolon at the end.

@@ +819,5 @@
>          mTestScopeDelegates.clear();
>      }
>  
>      protected void cleanupSession(final GeckoSession session) {
> +        if (session != null && session.isOpen()) {

Why would `session` be null here?
Attachment #8963381 - Flags: review?(nchen) → feedback+
(In reply to Jim Chen [:jchen] [:darchons] from comment #32)
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> java
> @@ +27,5 @@
> > +
> > +public final class GeckoRuntime implements Parcelable {
> > +    private static final String LOGTAG = "GeckoRuntime";
> > +
> > +    private GeckoRuntimeSettings mSettings = new GeckoRuntimeSettings();
> 
> Feel like we're creating `GeckoRuntimeSettings` copies a little
> unnecessarily? We should only need to copy the consumer-given settings once
> or maybe not at all.

I've added the copy for consistency with GeckoSessionSettings. The initialization is required to avoid having special cases for Parcelable support.
 
> @@ +41,5 @@
> > +    /* package */ boolean init() {
> > +        return init(mSettings);
> > +    }
> > +
> > +    /* package */ boolean init(final @Nullable GeckoRuntimeSettings settings) {
> 
> So GeckoThread can be launched _before_ being initialized (we do this for
> Fennec after APK update). I think we should support that.

I'm not sure we should expose that. Preferably the app creates the runtime as early as possible in its startup. Our use cases are somewhat hacky and we are doing it for the app in case of content process preload.
 
> @@ +91,5 @@
> > +        final GeckoRuntime runtime = new GeckoRuntime();
> > +        runtime.attachTo(context);
> > +
> > +        if (!runtime.init(new GeckoRuntimeSettings(settings))) {
> > +            throw new IllegalStateException("create failed due to init failure");
> 
> Maybe just let `init` throw. You'll get better exception messages.

We log the more detailed message in init, I would like to avoid throwing to make default runtime handling easier.
 
> @@ +127,5 @@
> > +        @Override
> > +        public GeckoRuntime createFromParcel(final Parcel in) {
> > +            final GeckoRuntime runtime = new GeckoRuntime();
> > +            runtime.readFromParcel(in);
> > +            runtime.init();
> 
> I don't think we should `init` here, because presumably the source runtime
> has already been initialized.

What if this is used in a new process eventually? I wasn't sure about the (future) use cases of this and how we would attach to the new context in that case.

> @@ +45,5 @@
> > +    }
> > +
> > +    /**
> > +     * Get the custom Gecko process arguments.
> > +     * Note: Only for internal use.
> 
> What does "internal use" mean? I think a consumer could want to pass in
> arguments.

The args are not part of the GV API, I find it difficult to document/communicate what exactly we support and which args we might event just overwrite.

> @@ +661,5 @@
> >       * is opened here. Opening a session is an asynchronous operation. You can check
> >       * the current state via isOpen().
> >       *
> > +     * Call this when you are ready to use a GeckoSession instance before
> > +     * attaching to a view.
> 
> This should go to the GeckoView documentation instead of here.

I've duplicated it there, given that GeckoView is somewhat under-documented, I would expect users to look for GeckoSession's docs first.
Attachment #8963377 - Attachment is obsolete: true
Attachment #8963596 - Flags: review?(snorp)
Attachment #8963596 - Flags: review?(nchen)
(In reply to Eugen Sawin [:esawin] from comment #35)
> Created attachment 8963596 [details] [diff] [review]
> 0001-Bug-1445420-1.5-Add-GeckoRuntime-and-GeckoSettings-f.patch
> 
> > Feel like we're creating `GeckoRuntimeSettings` copies a little
> > unnecessarily? We should only need to copy the consumer-given settings once
> > or maybe not at all.
> 
> I've added the copy for consistency with GeckoSessionSettings. The
> initialization is required to avoid having special cases for Parcelable
> support.

Right now for the normal case of creating a runtime with custom settings, we create one GeckoRuntimeSettings instance on patch line 32, one instance on line 105, and then more one instance on line 69. I'd prefer if we only created one GeckoRuntimeSettings instance overall (in addition to the instance that the consumer creates).

Also just noticed settings parameter for init() is marked @Nullable but it'll crash if settings is null.

> > I don't think we should `init` here, because presumably the source runtime
> > has already been initialized.
> 
> What if this is used in a new process eventually? I wasn't sure about the
> (future) use cases of this and how we would attach to the new context in
> that case.

In a new process, the unparcelled runtime would refer to the original runtime, which has already been created in the original process. This is unsupported currently of course.

> > @@ +45,5 @@
> > What does "internal use" mean? I think a consumer could want to pass in
> > arguments.
> 
> The args are not part of the GV API, I find it difficult to
> document/communicate what exactly we support and which args we might event
> just overwrite.

Hm I have a feeling consumers will start using it if we provide it. It essentially becomes a public API even if we say it's not.

> > @@ +661,5 @@
> > >       * is opened here. Opening a session is an asynchronous operation. You can check
> > >       * the current state via isOpen().
> > >       *
> > > +     * Call this when you are ready to use a GeckoSession instance before
> > > +     * attaching to a view.
> > 
> > This should go to the GeckoView documentation instead of here.
> 
> I've duplicated it there, given that GeckoView is somewhat under-documented,
> I would expect users to look for GeckoSession's docs first.

Then we should say something like "When attaching to a GeckoView, open must be called before calling GeckoView.setSession.", to make it clear that `open` should be called whether using GeckoView or not, and if using GeckoView, `open` should be called before attaching.

Also, we should document GeckoView more if it feels under-documented.
(In reply to Jim Chen [:jchen] [:darchons] from comment #36)
> Right now for the normal case of creating a runtime with custom settings, we
> create one GeckoRuntimeSettings instance on patch line 32, one instance on
> line 105, and then more one instance on line 69. I'd prefer if we only
> created one GeckoRuntimeSettings instance overall (in addition to the
> instance that the consumer creates).

I think we should been the initial initialization in the class declaration to ensure it's never null (makes Parcelable and init() easier) and we should make the copy of the app's setting in create, but we can remove the copy in init(settings).

> Also just noticed settings parameter for init() is marked @Nullable but
> it'll crash if settings is null.

Good catch, thanks, it was only supposed to be @Nullable in create.

> Hm I have a feeling consumers will start using it if we provide it. It
> essentially becomes a public API even if we say it's not.

Hence my "here be dragons" comment. Do you think we could eventually provide explicit settings for each arg/env that we know we support? I think GV should "own" and document all the settings it supports without having to redirect users to Gecko documentation which might or might not apply for GV.

> Then we should say something like "When attaching to a GeckoView, open must
> be called before calling GeckoView.setSession.", to make it clear that
> `open` should be called whether using GeckoView or not, and if using
> GeckoView, `open` should be called before attaching.

That's clearer, thanks.
s/been/keep
(In reply to Eugen Sawin [:esawin] from comment #37)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #36)
> > Right now for the normal case of creating a runtime with custom settings, we
> > create one GeckoRuntimeSettings instance on patch line 32, one instance on
> > line 105, and then more one instance on line 69. I'd prefer if we only
> > created one GeckoRuntimeSettings instance overall (in addition to the
> > instance that the consumer creates).
> 
> I think we should been the initial initialization in the class declaration
> to ensure it's never null (makes Parcelable and init() easier)

You can remove `init()` (and the initial initialization) once you remove `runtime.init()` call from `createFromParcel`.

> > Hm I have a feeling consumers will start using it if we provide it. It
> > essentially becomes a public API even if we say it's not.
> 
> Hence my "here be dragons" comment. Do you think we could eventually provide
> explicit settings for each arg/env that we know we support? I think GV
> should "own" and document all the settings it supports without having to
> redirect users to Gecko documentation which might or might not apply for GV.

IMO env var should probably be generic (e.g. setEnvVar). Arguments should probably be itemized (e.g. setProfileName, setProfilePath, setPurgeCaches, etc.).
Attachment #8963381 - Attachment is obsolete: true
Attachment #8963713 - Flags: review?(nchen)
Comment on attachment 8963596 [details] [diff] [review]
0001-Bug-1445420-1.5-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8963596 [details] [diff] [review]:
-----------------------------------------------------------------

We could probably pick at this set of patches until the end of time, but this is at least looking like a good start to me.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +102,5 @@
> +        final GeckoRuntime runtime = new GeckoRuntime();
> +        runtime.attachTo(context);
> +
> +        if (!runtime.init(new GeckoRuntimeSettings(settings))) {
> +            throw new IllegalStateException("create failed due to init failure");

"Failed to initialize GeckoRuntime" maybe?

@@ +109,5 @@
> +        return runtime;
> +    }
> +
> +    /**
> +     * Shutdown the runtime. This will invalidate all attached sessions.

What does "invalidate" mean? AFAIK this will basically result in undefined behavior. We should have stronger wording here.

It would be nice if GeckoRuntime.close() also took care of closing all sessions that were attached to it.
Attachment #8963596 - Flags: review?(snorp) → review+
Comment on attachment 8963596 [details] [diff] [review]
0001-Bug-1445420-1.5-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8963596 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +53,5 @@
> +    /* package */ boolean init(final @Nullable GeckoRuntimeSettings settings) {
> +        if (DEBUG) {
> +            Log.d(LOGTAG, "init");
> +        }
> +        if (!GeckoThread.launch()) {

Call `launch` after `initMainProcess` returns true. You don't need this `if` block.

@@ +93,5 @@
> +     * @return An initialized runtime.
> +     */
> +    public static @NonNull GeckoRuntime create(
> +          final @NonNull Context context,
> +          final @Nullable GeckoRuntimeSettings settings) {

Fix indent?

@@ +101,5 @@
> +
> +        final GeckoRuntime runtime = new GeckoRuntime();
> +        runtime.attachTo(context);
> +
> +        if (!runtime.init(new GeckoRuntimeSettings(settings))) {

Again, no need to create extra GeckoRuntimeSettings instances.

@@ +140,5 @@
> +        @Override
> +        public GeckoRuntime createFromParcel(final Parcel in) {
> +            final GeckoRuntime runtime = new GeckoRuntime();
> +            runtime.readFromParcel(in);
> +            runtime.init();

You should remove this line (as explained in comment 36), and afterwards remove `init()` and the `mSettings` initializer.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +27,5 @@
> +    /* package */ GeckoRuntimeSettings(final @Nullable GeckoRuntimeSettings settings) {
> +        if (settings != null) {
> +            mUseContentProcess = settings.mUseContentProcess;
> +            mArgs = settings.mArgs.clone();
> +            if (settings.mExtras != null) {

Should never be null?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +191,5 @@
>                                      throw new IllegalArgumentException("Must use an unopened GeckoSession instance");
>                                  }
>  
> +                                if (GeckoSession.this.mWindow != null) {
> +                                    session.open(GeckoSession.this.mWindow.runtime);

Should call sendError in this case instead of sendSuccess, or we'll end up waiting for a new session.

@@ +663,5 @@
>       * is opened here. Opening a session is an asynchronous operation. You can check
>       * the current state via isOpen().
>       *
> +     * Call this when you are ready to use a GeckoSession instance before
> +     * attaching to a view.

Fix comment based on comment 36?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +205,5 @@
>          return session;
>      }
>  
> +    /**
> +     * Attach the session to this view. The session should be opened before

"Attach a session to..."
Attachment #8963596 - Flags: review?(nchen) → feedback+
Comment on attachment 8963713 [details] [diff] [review]
0004-Bug-1445420-4.2-Adjust-tests-to-run-with-GeckoRuntim.patch

Review of attachment 8963713 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +354,5 @@
>          sessionRule.delegateDuringNextWait(object : Callbacks.NavigationDelegate {
>              @AssertCalled(count = 1)
>              override fun onNewSession(session: GeckoSession, uri: String, response: GeckoSession.Response<GeckoSession>) {
> +                var newSession = sessionRule.createClosedSession(session.settings)
> +                newSession.open(sessionRule.runtime)

Actually, sorry, you need to keep all the `session.open()` calls. That goes through `GeckoSessionTestRule.openSession`, which does some extra work besides calling `GeckoSession.open()`

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt
@@ +31,5 @@
>  
>      @Test fun open_repeated() {
>          for (i in 1..5) {
>              sessionRule.session.close()
> +            sessionRule.session.open(sessionRule.runtime)

So keep `sessionRule.session.open()` here and elsewhere.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java
@@ +102,5 @@
>  
> +        final GeckoRuntimeSettings geckoSettings = new GeckoRuntimeSettings();
> +        geckoSettings.setArguments(new String[] { "-purgecaches" });
> +        geckoSettings.setExtras(intent.getExtras());
> +        mRuntime = GeckoRuntime.create(this, geckoSettings);

This will probably fail with multiple tests because they would all try to create additional runtimes, you need to use a default runtime.
Attachment #8963713 - Flags: review?(nchen) → feedback+
Comment on attachment 8963596 [details] [diff] [review]
0001-Bug-1445420-1.5-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8963596 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +206,5 @@
>      }
>  
> +    /**
> +     * Attach the session to this view. The session should be opened before
> +     * attaching.

Also add an assertion for `session.isOpen()`
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (PTO until April 9) from comment #41)
> It would be nice if GeckoRuntime.close() also took care of closing all
> sessions that were attached to it.

That could be interesting, do we want to support it? For that, we would need to keep track of sessions in the runtime, currently the relationship is one-way.
Shutting down Gecko automatically closes all Gecko windows, so technically the sessions are "closed", though the session objects in Java are not aware of that.
(In reply to Jim Chen [:jchen] [:darchons] from comment #33)
> Hm I see a problem with requiring the consumer to call open here: this is
> non-optimal when we're restoring states, because the window created here
> will be replaced by the existing window during restore.

Just non-optimal or is that an issue? Is it only because we're calling openWindow redundantly?

> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.
> java
> @@ +27,5 @@
> >  
> >  public final class GeckoRuntime implements Parcelable {
> >      private static final String LOGTAG = "GeckoRuntime";
> >  
> > +    private static GeckoRuntime sDefaultRuntime;
> 
> I thought we decided to let GeckoView keep the default runtime?

I've scrapped that idea for now due to the way we initialize Gecko in Fennec. Creating a runtime with GeckoThread running will currently throw and I think we want that behavior, at the same time I don't want to make the default constructor public.

> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
> @@ +253,5 @@
> >  
> >      public EventDispatcher getEventDispatcher() {
> >          return mSession.getEventDispatcher();
> >      }
> >  
> 
> I think this shortcut is nice to have. Otherwise we should remove all these
> shortcuts (like `getEventDispatcher()` above).

I don't think they are very useful since apps requiring any of those members will hold a GeckoSession reference, but I found GeckoView.getSettings() to be the only really misleading shortcut since it implies that we're returning GeckoView settings.
(In reply to Eugen Sawin [:esawin] from comment #47)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #33)
> > Hm I see a problem with requiring the consumer to call open here: this is
> > non-optimal when we're restoring states, because the window created here
> > will be replaced by the existing window during restore.
> 
> Just non-optimal or is that an issue? Is it only because we're calling
> openWindow redundantly?

I think it'll crash as of now because a session is already open when you perform the restore. You should check to make sure. You can of course call `close` on the existing session, but that's the non-optimal part.
Attachment #8963596 - Attachment is obsolete: true
Attachment #8964722 - Flags: review?(nchen)
Attachment #8963713 - Attachment is obsolete: true
Attachment #8964724 - Flags: review?(nchen)
Addressed comment 48.
Now we offer two ways to attach a session to GeckoView.
setSession(session) expects an open session and setSession(session, runtime) accepts an open session or a closed session and a non-null runtime, which is then used to open the session when required.
Attachment #8964722 - Attachment is obsolete: true
Attachment #8964722 - Flags: review?(nchen)
Attachment #8964944 - Flags: review?(nchen)
Attachment #8963378 - Attachment is obsolete: true
Attachment #8964945 - Flags: review?(nchen)
Comment on attachment 8964944 [details] [diff] [review]
0001-Bug-1445420-1.7-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8964944 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! LGTM

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +234,5 @@
>          if (mSession != null && mSession.isOpen()) {
>              throw new IllegalStateException("Current session is open");
>          }
>  
> +        if (runtime == null && !session.isOpen()) {

I think in addition, in the case of `session.isOpen()`, if `runtime == null`, we should set `mRuntime` to `session.getRuntime()`, or if `runtime != null`, we should assert `runtime == session.getRuntime()`.

@@ +359,5 @@
>          final SavedState ss = (SavedState) state;
>          super.onRestoreInstanceState(ss.getSuperState());
>  
>          if (mSession == null) {
> +            setSession(ss.session, ss.session.getRuntime());

`setSession(ss.session, null);`, because `ss.session` could be null, I think.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
@@ +61,5 @@
>  
> +        geckoSettings.setUseContentProcessHint(
> +            getIntent().getBooleanExtra(USE_MULTIPROCESS_EXTRA, true));
> +        geckoSettings.setExtras(getIntent().getExtras());
> +        mGeckoRuntime = GeckoRuntime.create(this, geckoSettings);

I think we have to use default runtime here because GeckoViewActivity can be restored with a runtime already running.

@@ +67,5 @@
> +        final GeckoSessionSettings sessionSettings = new GeckoSessionSettings();
> +        sessionSettings.setBoolean(GeckoSessionSettings.USE_MULTIPROCESS,
> +                                   geckoSettings.getUseContentProcessHint());
> +        mGeckoSession = new GeckoSession(sessionSettings);
> +        mGeckoSession.open(mGeckoRuntime);

Same here. I think we should just call `mGeckoView.setSession(mGeckoSession, mGeckoRuntime);` and let GeckoView manage the runtime.
Attachment #8964944 - Flags: review?(nchen) → review+
Comment on attachment 8964945 [details] [diff] [review]
0002-Bug-1445420-2.3-Add-default-runtime-fallback-for-Fen.patch

Review of attachment 8964945 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +98,5 @@
> +            GeckoSessionSettings.USE_REMOTE_DEBUGGER,
> +            GeckoSharedPrefs.forApp(this).getBoolean(
> +                GeckoPreferences.PREFS_DEVTOOLS_REMOTE_USB_ENABLED, false));
> +        mGeckoSession = new GeckoSession(settings);
> +        mGeckoSession.open(GeckoRuntime.getDefault(this));

We could be restoring WebAppActivity here, so use `mGeckoView.setSession(mGeckoSession, GeckoRuntime.getDefault(this));` and let GeckoView manage opening the session.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +47,5 @@
> +            sDefaultRuntime = new GeckoRuntime();
> +            sDefaultRuntime.attachTo(context);
> +            sDefaultRuntime.init(new GeckoRuntimeSettings());
> +        } else if (!context.getApplicationContext().equals(
> +                   GeckoAppShell.getApplicationContext())) {

This case actually happens, I think. The application context can change during Fennec's lifetime, so we should just attach to the new Context here.
Attachment #8964945 - Flags: review?(nchen) → review+
Comment on attachment 8964724 [details] [diff] [review]
0004-Bug-1445420-4.3-Adjust-tests-to-run-with-GeckoRuntim.patch

Review of attachment 8964724 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +353,5 @@
>  
>          sessionRule.delegateDuringNextWait(object : Callbacks.NavigationDelegate {
>              @AssertCalled(count = 1)
>              override fun onNewSession(session: GeckoSession, uri: String, response: GeckoSession.Response<GeckoSession>) {
> +                var newSession = sessionRule.createClosedSession(session.settings)

Technically should be `val newSession`
Attachment #8964724 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #53)
> @@ +359,5 @@
> >          final SavedState ss = (SavedState) state;
> >          super.onRestoreInstanceState(ss.getSuperState());
> >  
> >          if (mSession == null) {
> > +            setSession(ss.session, ss.session.getRuntime());
> 
> `setSession(ss.session, null);`, because `ss.session` could be null, I think.

setSession does not accept null anymore and with mSession = null it was essentially a NOP.

Can you please take another look, I've changed some behavior around setSession.
Attachment #8964944 - Attachment is obsolete: true
Attachment #8964990 - Flags: review?(nchen)
Attachment #8964724 - Attachment is obsolete: true
Attachment #8964945 - Attachment is obsolete: true
Attachment #8964992 - Flags: review+
Comment on attachment 8964990 [details] [diff] [review]
0001-Bug-1445420-1.8-Add-GeckoRuntime-and-GeckoSettings-f.patch

Review of attachment 8964990 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +243,5 @@
> +        if (runtime == null && !session.isOpen()) {
> +            throw new IllegalArgumentException("Session must be open before attaching");
> +        }
> +
> +        if (session.isOpen()) {

I think the two blocks are better as

> if (session.isOpen()) {
>     ...
> } else if (runtime == null) {
>     throw ...
> }
Attachment #8964990 - Flags: review?(nchen) → review+
I would like to hear some feedback from the Focus devs on the new runtime API before landing (mainly patch 1).

We're adding GeckoRuntime and GeckoRuntimeSettings.
The runtime is now required to open sessions. You can provide the runtime either by calling GeckoSession.open(runtime) or when attaching the session to the view via GeckoView.setSession(session, runtime). In the latter case, the view is responsible for opening the session, as before.

There is one caveat: due to implementation-specific restrictions, you can only have one runtime active in your process for now. To avoid issues, you need to bind the runtime to your apps's process lifecycle instead of the activity's.

See GeckoViewActivity for a basic example in patch 1.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(ekager)
We talked a little bit on Slack. This change looks good to me. :)
Flags: needinfo?(s.kaspari)
Thanks for the context in Slack and sorry for the delay! This also looks good to me! :)
Flags: needinfo?(ekager)
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43c585beaad3
[1.9] Add GeckoRuntime and GeckoSettings for Gecko setup and initialization. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d40aa987f0
[2.5] Add default runtime fallback for Fennec and unattached views. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/6db5a47c33a7
[4.4] Adjust tests to run with GeckoRuntime. r=snorp,jchen
Depends on: 1465877
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: