Closed Bug 1441810 Opened 2 years ago Closed 2 years ago

GeckoView: Save and restore state

Categories

(GeckoView :: General, enhancement, P1)

All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sebastian, Assigned: droeh)

References

Details

(Whiteboard: [geckoview:klar])

Attachments

(3 files, 5 obsolete files)

Related Focus/Klar issue: https://github.com/mozilla-mobile/focus-android/issues/2174


Related WebView APIs:
* WebBackForwardList saveState(Bundle outState): https://developer.android.com/reference/android/webkit/WebView.html#saveState(android.os.Bundle)
* WebBackForwardList restoreState(Bundle inState): https://developer.android.com/reference/android/webkit/WebView.html#restoreState(android.os.Bundle)


In certain situations we use saveState/restoreState to save the current state of the WebView (including history) into a bundle. At a later point we restore the state (in a new WebView). This happens for example when a user switches tabs or when the activity gets destroyed in the background.

Is this something we can implement for GeckoView too? Or is there some other mechanism to save and restore a "session". Note that for Focus/Klar we keep the session Bundle in memory and do not write it to disk.
Whiteboard: [geckoview:klar]
In theory something could be done based on the session store, although in practice for immediate usage the session store is
- very much not e10s-ready (not sure in how far that would be relevant for Focus/Klar specifically, but for GeckoView in general I guess it's required)
- rather geared towards operating within Fennec.

One possible way forward that would be to separate out the actual tab data gathering and restoring into a separate (content) module.
As a rough sketch that would entail:
- Implement bug 1364609 to start using the TabStateCache instead of directly storing the collected session store data attached to a tab's <browser>, and also make the TabStateCache available for GeckoView as well
- Instead of directly adding a number of event listeners [1], respectively relying on event listeners provided by Fennec's browser.js [2], inject a content script [3] that
  - registers those listeners itself and takes the necessary actions
  - sends any collected tab data back to the TabStateCache from bug 1364609, plus notifies any interested parties that new data is available and possibly the reason why
  - provides an API for passing it stored session store data which the content script then restores, i.e. which more or less does what [4] does today

I think things could be separated relatively cleanly along those lines, although at a first glance these bits here [5] might possibly be a little more tricky to deal with, and maybe I've overlooked some fundamental problem.

That way, in Fennec the session store could operate more or less like it does today, except that some functionality will be outsources to this new module.

For GeckoView
- saveState could either get its data more or less directly (and synchronously) from the TabStateCache
  (+) the state data will be as fresh as possible
  (-) if Gecko is busy, this call could take a while
- or else any data arriving at the TabStateCache could immediately be forwarded to another cache on the Java side
  (+) saveState will always be able to return data very quickly
  (-) if Gecko is busy, the data available on the Java side might be out of date

[1] https://dxr.mozilla.org/mozilla-central/rev/5297541590781af40ff09e067646f3115960af75/mobile/android/components/SessionStore.js#605-621
[2] https://dxr.mozilla.org/mozilla-central/rev/5297541590781af40ff09e067646f3115960af75/mobile/android/components/SessionStore.js#282-283
[3] Although bug 1434768 might change things here and make session history more easily available from the parent process, though we'd probably still have to collect things like form data and scroll position/zoom level from within the content process
[4] https://dxr.mozilla.org/mozilla-central/rev/5297541590781af40ff09e067646f3115960af75/mobile/android/components/SessionStore.js#1296-1320
[5] https://dxr.mozilla.org/mozilla-central/rev/5297541590781af40ff09e067646f3115960af75/mobile/android/components/SessionStore.js#709-711 and similar occurrences in the other content event handling functions
Assignee: nobody → droeh
Priority: -- → P1
To avoid reinventing the wheel, it might also be an interesting idea to attempt using Desktop's content script (https://dxr.mozilla.org/mozilla-central/rev/bccdc684210431c233622650a91454c09f6af9eb/browser/components/sessionstore/content/content-sessionStore.js) for collecting the data here. To make this more manageable, I'd suggest attempting to get this going in GeckoView only at first (and add whatever minimal support infrastructure is necessary to connect the content script and its collected data with the desired API on the Android side) and leave migration of Fennec's session store to a later stage, as there are some definitive differences in behaviour and collected data that need sorting out, and I don't want to risk regressing things just because we might be in a hurry for Focus/Klar.
Attached patch Session store API first pass (obsolete) — Splinter Review
Per conversation with snorp, it would be good to get an API discussion going in parallel with the implementation work I'm doing at the moment.
Flags: needinfo?(snorp)
Comment on attachment 8962766 [details] [diff] [review]
Session store API first pass

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +567,5 @@
>      }
>  
> +    // Create a new session from a saved state
> +    // - Should GeckoSessionState include settings as well?
> +    public GeckoSession(final GeckoSessionState sessionState);

I don't think I like this, but could be convinced otherwise. I think the state should just go in open() as I describe below.

@@ +1022,5 @@
>          mEventDispatcher.dispatch("GeckoView:SetActive", msg);
>      }
>  
> +    // Opaque wrapper around a GeckoBundle that represents a stored session state
> +    public class GeckoSessionState implements Parcelable {

It would probably be good to have some string-based serialization available in case somebody wants to store this in something that doesn't support Parcelable.

@@ +1037,5 @@
> +        public void writeToParcel(Parcel out, int flags);
> +    }
> +
> +    // Sync save state
> +    public GeckoSessionState saveState();

It's not saving it anywhere, just returning it. Maybe just getState()?

@@ +1040,5 @@
> +    // Sync save state
> +    public GeckoSessionState saveState();
> +
> +    // Async save state?
> +    public void asyncSaveState(Response<GeckoSession> response);

We'd probably just call this one saveState() as well (or getState(), etc).

I'm not convinced we need/want an async path, though. Most of the time this is going to be called in Activity.onStop(), and you want to block.

It's a bit scary to make a synchronous call to Gecko, but we do it in some other places (like resuming/stopping the compositor). I think there may be some things we can do to EventDispatcher to make synchronous events more palatable (or at least slightly less scary). For instance: instead of sending a normal async message and spinning the event loop as we do in GeckoViewNavigation.jsm, we could tell EventDispatcher to do that for us. Since the dispatcher knows it's processing a synchronous message on the Gecko side, it would throw if the UI thread attempted to also dispatch a synchronous message.

@@ +1043,5 @@
> +    // Async save state?
> +    public void asyncSaveState(Response<GeckoSession> response);
> +
> +    // Suspend -- restores state and stops (eventually/lazily?) the GeckoSession
> +    public GeckoSessionState suspend();

This would expand to something like:

state = saveState();
close();
return state;

Right?

@@ +1046,5 @@
> +    // Suspend -- restores state and stops (eventually/lazily?) the GeckoSession
> +    public GeckoSessionState suspend();
> +
> +    // Sync restore/overwrite state
> +    public void restoreState();

I think the state should just be an argument to open(). This makes even more sense if you think about suspend() being what I wrote above.

@@ +1049,5 @@
> +    // Sync restore/overwrite state
> +    public void restoreState();
> +
> +    // Async restore/overwrite state (w/ callback for completion)?
> +    public void asyncRestoreState(Runnable r);

I don't think we need this -- open() is already async. You'll get appropriate page load events in order to determine progress, success, etc. Maybe we could have a onRestoreComplete() though, if that turns out to be needed for some reason.
NI to jchen, esawin, rbarker for API review
Flags: needinfo?(snorp)
Flags: needinfo?(rbarker)
Flags: needinfo?(nchen)
Flags: needinfo?(esawin)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Comment on attachment 8962766 [details] [diff] [review]
> Session store API first pass
> 
> Review of attachment 8962766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.
> java
> @@ +567,5 @@
> >      }
> >  
> > +    // Create a new session from a saved state
> > +    // - Should GeckoSessionState include settings as well?
> > +    public GeckoSession(final GeckoSessionState sessionState);
> 
> I don't think I like this, but could be convinced otherwise. I think the
> state should just go in open() as I describe below.

Fair enough. Thinking it over, we probably need it in open(), so having it here as well might be redundant.

> @@ +1022,5 @@
> >          mEventDispatcher.dispatch("GeckoView:SetActive", msg);
> >      }
> >  
> > +    // Opaque wrapper around a GeckoBundle that represents a stored session state
> > +    public class GeckoSessionState implements Parcelable {
> 
> It would probably be good to have some string-based serialization available
> in case somebody wants to store this in something that doesn't support
> Parcelable.

Yeah, that makes sense and is easy enough to do.

> @@ +1037,5 @@
> > +        public void writeToParcel(Parcel out, int flags);
> > +    }
> > +
> > +    // Sync save state
> > +    public GeckoSessionState saveState();
> 
> It's not saving it anywhere, just returning it. Maybe just getState()?

I don't like getState() for some reason, but I can't really articulate why. I could be probably be convinced to go with it.

> @@ +1040,5 @@
> > +    // Sync save state
> > +    public GeckoSessionState saveState();
> > +
> > +    // Async save state?
> > +    public void asyncSaveState(Response<GeckoSession> response);
> 
> We'd probably just call this one saveState() as well (or getState(), etc).
> 
> I'm not convinced we need/want an async path, though. Most of the time this
> is going to be called in Activity.onStop(), and you want to block.
> 
> It's a bit scary to make a synchronous call to Gecko, but we do it in some
> other places (like resuming/stopping the compositor). I think there may be
> some things we can do to EventDispatcher to make synchronous events more
> palatable (or at least slightly less scary). For instance: instead of
> sending a normal async message and spinning the event loop as we do in
> GeckoViewNavigation.jsm, we could tell EventDispatcher to do that for us.
> Since the dispatcher knows it's processing a synchronous message on the
> Gecko side, it would throw if the UI thread attempted to also dispatch a
> synchronous message.
> 
> @@ +1043,5 @@
> > +    // Async save state?
> > +    public void asyncSaveState(Response<GeckoSession> response);
> > +
> > +    // Suspend -- restores state and stops (eventually/lazily?) the GeckoSession
> > +    public GeckoSessionState suspend();
> 
> This would expand to something like:
> 
> state = saveState();
> close();
> return state;
> 
> Right?

Yeah.

> @@ +1046,5 @@
> > +    // Suspend -- restores state and stops (eventually/lazily?) the GeckoSession
> > +    public GeckoSessionState suspend();
> > +
> > +    // Sync restore/overwrite state
> > +    public void restoreState();
> 
> I think the state should just be an argument to open(). This makes even more
> sense if you think about suspend() being what I wrote above.
> 
> @@ +1049,5 @@
> > +    // Sync restore/overwrite state
> > +    public void restoreState();
> > +
> > +    // Async restore/overwrite state (w/ callback for completion)?
> > +    public void asyncRestoreState(Runnable r);
> 
> I don't think we need this -- open() is already async. You'll get
> appropriate page load events in order to determine progress, success, etc.
> Maybe we could have a onRestoreComplete() though, if that turns out to be
> needed for some reason.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> I'm not convinced we need/want an async path, though. Most of the time this
> is going to be called in Activity.onStop(), and you want to block.
> 
> It's a bit scary to make a synchronous call to Gecko, but we do it in some
> other places (like resuming/stopping the compositor). I think there may be
> some things we can do to EventDispatcher to make synchronous events more
> palatable (or at least slightly less scary). For instance: instead of
> sending a normal async message and spinning the event loop as we do in
> GeckoViewNavigation.jsm, we could tell EventDispatcher to do that for us.
> Since the dispatcher knows it's processing a synchronous message on the
> Gecko side, it would throw if the UI thread attempted to also dispatch a
> synchronous message.

I know that for absolute correctness and to get the absolutely latest page state, in theory there's no choice but to do a sync call all the way down to collecting the fresh state from Gecko, but...
Compared to today, how much other stuff is expected to happen in Gecko when going into the background? By opening Android's Recent Apps screen and then changing your mind, it's e.g. relative easy to trigger a quick onPause/onStop/onStart/onResume combo. Right now, this just means some janky scrolling for a few seconds as Gecko is still busy processing our pause handling (which might be specific to Fennec, or maybe still necessary even for GeckoView in general), but at least the Android app UI remains responsive.

Even if we minimise the amount of work triggered by us on the Gecko main thread in response to onStop etc., Gecko could of course still be busy anyway with loading a page, being stuck in some heavyweight JS script, ..., which would likewise mean that a quick background-foreground cycle would lead to a momentarily hung app UI while we're waiting for the saveState() call to fight its way through to the Gecko main thread and return back to Android's lifecycle event handling.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Comment on attachment 8962766 [details] [diff] [review]
> Session store API first pass
> 
> Review of attachment 8962766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.
> java
> @@ +1037,5 @@
> > +        public void writeToParcel(Parcel out, int flags);
> > +    }
> > +
> > +    // Sync save state
> > +    public GeckoSessionState saveState();
> 
> It's not saving it anywhere, just returning it. Maybe just getState()?
> 

I think the problem with getState is it seem to imply the state is live while what we are really doing is creating a state snapshot. so maybe createStateSnapshot() ?
Flags: needinfo?(rbarker)
My preference is for separate `saveState` and `restoreState` methods (perhaps with "Snapshot" in the name like Randall suggested), and that's it. I think it's important to use matching names like "save" and "restore"; "get" and "restore" don't quite match because "get" is usually matched with "set".

For `GeckoSession(state)` and `open(state)`, if they're just shortcuts for `open(); restoreState(state)`, then I don't think we should bother with it. Same for `suspend()`; if it's just a shortcut for `saveState(); close();` then we shouldn't bother.

As for sync/async, I think async should be the default like the rest of our APIs. Like Jan said, we should think hard before deciding to implement something synchronous. I suggest leaving the consumer to implement synchronous operation on top of async if the consumer absolutely needs it (or have GeckoView implement a sync option on top of GeckoSession's async option).
Flags: needinfo?(nchen)
This actually turned out to be a pretty small amount of code; the vast majority of the Fennec session storage code is irrelevant for our use case.
Attachment #8966253 - Flags: review?(snorp)
A first real pass at the API -- I'm flagging this for feedback rather than review because we need to settle the sync/async/both question before finalizing this (currently just async is supported). In order to support string based serialization (per snorp's earlier feedback) I'm currently doing something a bit ugly -- using JSON.stringify/parse on the JS side of things and just having the state be a string; that's because we don't have a general way of constructing a GeckoBundle from a string (ie, an inverse of GeckoBundle.toString()). I'm open to other ideas there.
Attachment #8962766 - Attachment is obsolete: true
Attachment #8966256 - Flags: feedback?(snorp)
Attachment #8966256 - Flags: feedback?(nchen)
Comment on attachment 8966253 [details] [diff] [review]
GeckoSession save/restore patch 1

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

::: mobile/android/modules/geckoview/GeckoViewContent.jsm
@@ +70,5 @@
>          break;
> +      case "GeckoView:SaveState":
> +        let history = SessionHistory.collect(this.browser.docShell);
> +        let formdata = FormData.collect(this.browser.contentWindow);
> +        let scrollpos = ScrollPosition.collect(this.browser.contentWindow);

Can you please also include the logic from sessionstore.js to save the current document resolution?
Comment on attachment 8966253 [details] [diff] [review]
GeckoSession save/restore patch 1

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

::: mobile/android/modules/geckoview/GeckoViewContent.jsm
@@ +80,5 @@
> +        };
> +
> +        aCallback.onSuccess(JSON.stringify(message));
> +        break;
> +      case "GeckoView:RestoreState":

I think when restoring the state we should do it as part of opening the window, so we wouldn't be using a separate message like this. I guess I'm missing the bit of the patch that sends that, but I think you would add the state to GeckoSession.Window.open(). That method has about a billion arguments, so maybe we need a better way to pass this kind of stuff in.

@@ +89,5 @@
> +
> +        if (savedHistory) {
> +          let restoredHistory = SessionHistory.restore(this.browser.docShell, savedHistory);
> +          restoredHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> +          this.browser.__SS_data = {

I don't think we need to set browser.__SS_data, but this would change anyway if we are restoring in the process of creating the browser.
Attachment #8966253 - Flags: review?(snorp) → review-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #13)
> @@ +89,5 @@
> > +
> > +        if (savedHistory) {
> > +          let restoredHistory = SessionHistory.restore(this.browser.docShell, savedHistory);
> > +          restoredHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> > +          this.browser.__SS_data = {
> 
> I don't think we need to set browser.__SS_data, but this would change anyway
> if we are restoring in the process of creating the browser.

We have to, because various bits of state can only be restored at some later stage after calling SessionHistory.restore() and reloadCurrentEntry():

- The document resolution and previous content viewer size need to be restored as early as possible, but on the other hand I don't think we can set them *before* the locationchange event following the reloadCurrentEntry()
- Form data can only be restored after loading has progressed far enough to fire the "load" event.
- Scroll position needs to wait until *after* the "load" event (so that the restored document resolution has been definitively applied), which in practice means on "pageshow"
(In reply to Jan Henning [:JanH] from comment #12)
> Can you please also include the logic from sessionstore.js to save the
> current document resolution?

Good catch, will do!

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #13)
> Comment on attachment 8966253 [details] [diff] [review]
> GeckoSession save/restore patch 1
> 
> Review of attachment 8966253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/geckoview/GeckoViewContent.jsm
> @@ +80,5 @@
> > +        };
> > +
> > +        aCallback.onSuccess(JSON.stringify(message));
> > +        break;
> > +      case "GeckoView:RestoreState":
> 
> I think when restoring the state we should do it as part of opening the
> window, so we wouldn't be using a separate message like this. I guess I'm
> missing the bit of the patch that sends that, but I think you would add the
> state to GeckoSession.Window.open(). That method has about a billion
> arguments, so maybe we need a better way to pass this kind of stuff in.

(The part that sends GeckoView:RestoreState is in the second patch)
I don't really agree that we should limit session restoration to happening in open(). To give a simple usage case, consider a simple tabbed browser implemented with one GeckoSession -- when you switch tabs you save the state of the current tab and restore the (saved) state of the tab you're switching to; there's no reason to force closing/opening to happen there as well. I tend to agree with Jim (in comment 9) that it would be best to just expose saveState/restoreState (possibly with a sync saveState as well) and let the consumer use them as they will -- unless there's some real advantage to combining open() and restoreState() that I'm missing, it feels like API bloat to me.

> @@ +89,5 @@
> > +
> > +        if (savedHistory) {
> > +          let restoredHistory = SessionHistory.restore(this.browser.docShell, savedHistory);
> > +          restoredHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> > +          this.browser.__SS_data = {
> 
> I don't think we need to set browser.__SS_data, but this would change anyway
> if we are restoring in the process of creating the browser.

We set this because we need to wait for "load" to restore scroll position and form data -- I'm pretty sure this will remain more or less the same no matter what we do for the API.
Comment on attachment 8966256 [details] [diff] [review]
GeckoSession save/restore patch 2

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

The consensus seems to be that async API is more useful than sync, which seems fine. Jan made good points. The naming of saveState() still doesn't seem quite right to me, but I don't have anything better to offer either.

r- mainly because of restoreState()

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1023,4 @@
>          mEventDispatcher.dispatch("GeckoView:SetActive", msg);
>      }
>  
> +    public static class GeckoSessionState implements Parcelable {

If we're going to have toString() and GeckoSessionState(String) I wonder if we shouldn't just use a String instead, since there isn't much benefit to having a separate class in front of it at that point. If we just rely on Parcelable instead I think we do get a bit more flexibility internally, but that does limit how folks can store this.

It's worth noting that I do think people will want to store this on the disk, so the state data format will need to be stable across GV versions.

@@ +1058,5 @@
> +            }
> +        };
> +    }
> +
> +    public void saveState(final Response<GeckoSessionState> response) {

@NotNull

@@ +1074,5 @@
> +        });
> +    }
> +
> +    public void restoreState(final GeckoSessionState state) {
> +        mEventDispatcher.dispatch("GeckoView:RestoreState", state.toString());

You should only be able to restore state on a closed session, which means this impl should basically be the same as open() but with the state.
Attachment #8966256 - Flags: feedback?(snorp) → feedback-
> You should only be able to restore state on a closed session

I think there's value in allowing an already open session. Assuming the state is stored on disk, I think in practice the session will likely already be open by the time data is available from disk. If we only allow closed sessions, the consumer would have to close the existing session every time they perform a restore, which doesn't seem very efficient or elegant.
Another question:
What is the intended behaviour when calling saveState() while the tab is still loading, or more importantly, still restoring? I think the session history can be collected again as soon as we've restored it, but form data/scroll position/zoom level won't be available until after load respectively pageshow.

If an app is naively programmed to unconditionally call saveState() when going into the background for example, this could easily happen, especially when restoring a large page, possibly with a slow network or running on a low-performance device on top. The end result is that form history or the current scroll position will be lost when that happens.
(In reply to Jim Chen [:jchen] [:darchons] from comment #17)
> > You should only be able to restore state on a closed session
> 
> I think there's value in allowing an already open session. Assuming the
> state is stored on disk, I think in practice the session will likely already
> be open by the time data is available from disk.

You could wait until you have the state from disk to open the session, but if you don't have to do that it would be a perf improvement, so this is a good point.

> If we only allow closed
> sessions, the consumer would have to close the existing session every time
> they perform a restore, which doesn't seem very efficient or elegant.

Sure, but are we ok with state leaking from the existing open session? I am not sure if I can identify anything in particular that would do this right now, but it seems like a possibility.
(In reply to Jan Henning [:JanH] from comment #18)
> Another question:
> What is the intended behaviour when calling saveState() while the tab is
> still loading, or more importantly, still restoring? I think the session
> history can be collected again as soon as we've restored it, but form
> data/scroll position/zoom level won't be available until after load
> respectively pageshow.
> 
> If an app is naively programmed to unconditionally call saveState() when
> going into the background for example, this could easily happen, especially
> when restoring a large page, possibly with a slow network or running on a
> low-performance device on top. The end result is that form history or the
> current scroll position will be lost when that happens.

Yeah, good question. Maybe saveState() should return whatever you passed in to restoreState() until we can deliver a better answer?
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #20)
> 
> Yeah, good question. Maybe saveState() should return whatever you passed in
> to restoreState() until we can deliver a better answer?

I guess another option is to delay sending the saveState() response until the session is in a good place. Since the API is async that should be relatively straightforward. A long delay in the response may be unexpected by callers, so we should document this behavior.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #20)
> Yeah, good question. Maybe saveState() should return whatever you passed in
> to restoreState() until we can deliver a better answer?

One idea: delete this.browser.__SS_data.formdata again after having called FormData.restoreTree and likewise for the scroll position. GeckoView:SaveState could then check that - if the corresponding _SS_data from the previous restoration is still present, it returns that that, otherwise we do a fresh collect.
(In reply to Dylan Roeh (:droeh) from comment #15)
> (In reply to Jan Henning [:JanH] from comment #12)
> > Can you please also include the logic from sessionstore.js to save the
> > current document resolution?
> 
> Good catch, will do!
> 
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #13)
> > Comment on attachment 8966253 [details] [diff] [review]
> > GeckoSession save/restore patch 1
> > 
> > Review of attachment 8966253 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/geckoview/GeckoViewContent.jsm
> > @@ +80,5 @@
> > > +        };
> > > +
> > > +        aCallback.onSuccess(JSON.stringify(message));
> > > +        break;
> > > +      case "GeckoView:RestoreState":
> > 
> > I think when restoring the state we should do it as part of opening the
> > window, so we wouldn't be using a separate message like this. I guess I'm
> > missing the bit of the patch that sends that, but I think you would add the
> > state to GeckoSession.Window.open(). That method has about a billion
> > arguments, so maybe we need a better way to pass this kind of stuff in.
> 
> (The part that sends GeckoView:RestoreState is in the second patch)
> I don't really agree that we should limit session restoration to happening
> in open(). To give a simple usage case, consider a simple tabbed browser
> implemented with one GeckoSession -- when you switch tabs you save the state
> of the current tab and restore the (saved) state of the tab you're switching
> to; there's no reason to force closing/opening to happen there as well.

Yeah, I'm now convinced we should allow restoreState() on an open session. The example you gave is a bad way to implement tabs, however. We should encourage consumers to use multiple GeckoSession instances instead.

> 
> > @@ +89,5 @@
> > > +
> > > +        if (savedHistory) {
> > > +          let restoredHistory = SessionHistory.restore(this.browser.docShell, savedHistory);
> > > +          restoredHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> > > +          this.browser.__SS_data = {
> > 
> > I don't think we need to set browser.__SS_data, but this would change anyway
> > if we are restoring in the process of creating the browser.
> 
> We set this because we need to wait for "load" to restore scroll position
> and form data -- I'm pretty sure this will remain more or less the same no
> matter what we do for the API.

Sure, you have to wait for "load", but you can just set this._pendingState or whatever instead of hanging it off of this.browser.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #20)
> (In reply to Jan Henning [:JanH] from comment #18)
> > Another question:
> > What is the intended behaviour when calling saveState() while the tab is
> > still loading, or more importantly, still restoring? I think the session
> > history can be collected again as soon as we've restored it, but form
> > data/scroll position/zoom level won't be available until after load
> > respectively pageshow.
> > 
> > If an app is naively programmed to unconditionally call saveState() when
> > going into the background for example, this could easily happen, especially
> > when restoring a large page, possibly with a slow network or running on a
> > low-performance device on top. The end result is that form history or the
> > current scroll position will be lost when that happens.
> 
> Yeah, good question. Maybe saveState() should return whatever you passed in
> to restoreState() until we can deliver a better answer?

It seems to me like this would be the easiest option, and a pretty good way of handling it.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #23)
> (In reply to Dylan Roeh (:droeh) from comment #15)
> > (In reply to Jan Henning [:JanH] from comment #12)
> > > Can you please also include the logic from sessionstore.js to save the
> > > current document resolution?
> > 
> > Good catch, will do!
> > 
> > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #13)
> > > Comment on attachment 8966253 [details] [diff] [review]
> > > GeckoSession save/restore patch 1
> > > 
> > > Review of attachment 8966253 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: mobile/android/modules/geckoview/GeckoViewContent.jsm
> > > @@ +80,5 @@
> > > > +        };
> > > > +
> > > > +        aCallback.onSuccess(JSON.stringify(message));
> > > > +        break;
> > > > +      case "GeckoView:RestoreState":
> > > 
> > > I think when restoring the state we should do it as part of opening the
> > > window, so we wouldn't be using a separate message like this. I guess I'm
> > > missing the bit of the patch that sends that, but I think you would add the
> > > state to GeckoSession.Window.open(). That method has about a billion
> > > arguments, so maybe we need a better way to pass this kind of stuff in.
> > 
> > (The part that sends GeckoView:RestoreState is in the second patch)
> > I don't really agree that we should limit session restoration to happening
> > in open(). To give a simple usage case, consider a simple tabbed browser
> > implemented with one GeckoSession -- when you switch tabs you save the state
> > of the current tab and restore the (saved) state of the tab you're switching
> > to; there's no reason to force closing/opening to happen there as well.
> 
> Yeah, I'm now convinced we should allow restoreState() on an open session.
> The example you gave is a bad way to implement tabs, however. We should
> encourage consumers to use multiple GeckoSession instances instead.

Agreed, it was just the first example I could think of.

> > 
> > > @@ +89,5 @@
> > > > +
> > > > +        if (savedHistory) {
> > > > +          let restoredHistory = SessionHistory.restore(this.browser.docShell, savedHistory);
> > > > +          restoredHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> > > > +          this.browser.__SS_data = {
> > > 
> > > I don't think we need to set browser.__SS_data, but this would change anyway
> > > if we are restoring in the process of creating the browser.
> > 
> > We set this because we need to wait for "load" to restore scroll position
> > and form data -- I'm pretty sure this will remain more or less the same no
> > matter what we do for the API.
> 
> Sure, you have to wait for "load", but you can just set this._pendingState
> or whatever instead of hanging it off of this.browser.

Ah, yeah, that's fine.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #16)
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.
> java
> @@ +1023,4 @@
> >          mEventDispatcher.dispatch("GeckoView:SetActive", msg);
> >      }
> >  
> > +    public static class GeckoSessionState implements Parcelable {
> 
> If we're going to have toString() and GeckoSessionState(String) I wonder if
> we shouldn't just use a String instead, since there isn't much benefit to
> having a separate class in front of it at that point. If we just rely on
> Parcelable instead I think we do get a bit more flexibility internally, but
> that does limit how folks can store this.

Yeah, I was kind of wondering about this as I wrote it. I kind of prefer it this way because it feels a bit strange to have saveState just give you a string in my opinion. Also, we could potentially have GeckoSessionState(String) do some validation on the string to make sure we aren't just throwing garbage at the state restoration code.

> It's worth noting that I do think people will want to store this on the
> disk, so the state data format will need to be stable across GV versions.

It seems like it's something that should be stable (and also should handle expansions pretty well -- ie, if you add data to the format, older saved states will restore the same way they did previously), but maybe the format should include a saved state format version number so that we've got something to compare to if at some point in the future something forces us to make a compatibility-breaking change?
Comment on attachment 8966256 [details] [diff] [review]
GeckoSession save/restore patch 2

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1023,4 @@
>          mEventDispatcher.dispatch("GeckoView:SetActive", msg);
>      }
>  
> +    public static class GeckoSessionState implements Parcelable {

Make this top-level or call it "SavedState" or something.

@@ +1042,5 @@
> +
> +        @Override
> +        public void writeToParcel(final Parcel dest, final int flags) {
> +            dest.writeString(mState);
> +        }

Add readFromParcel. See https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#694

@@ +1074,5 @@
> +        });
> +    }
> +
> +    public void restoreState(final GeckoSessionState state) {
> +        mEventDispatcher.dispatch("GeckoView:RestoreState", state.toString());

This wouldn't compile because `EventDispatcher.dispatch` expects a `GeckoBundle`
Attachment #8966256 - Flags: feedback?(nchen) → feedback+
I think this should address all feedback, save stuff that was missed in the previous iteration, and support e10s. Second part of the patch (the Java side) will be up tomorrow after I finish documentation.
Attachment #8966253 - Attachment is obsolete: true
Attachment #8968676 - Flags: review?(snorp)
Attachment #8968676 - Flags: review?(nchen)
Comment on attachment 8968676 [details] [diff] [review]
GeckoSession save/restore patch 1 (v2)

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

Mostly nits, but I think we need to fix the progress listener registration.

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +52,5 @@
> +      .createInstance(Ci.nsIWebProgress);
> +    this.progressFilter.addProgressListener(this, flags);
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIWebProgress);
> +    webProgress.addProgressListener(this.progressFilter, flags);

We only want to do this if we are restoring state.

@@ +74,4 @@
>                                                this);
>      this.messageManager.removeMessageListener("GeckoView:ZoomToInput",
>                                                this);
> +    this.messageManager.removeMessageListener("GeckoView:SaveStateBegin",

I think SaveState is fine. There isn't a SaveStateEnd message, and we can assume that like all messages this is asynchronous and may have a reply.

@@ +79,5 @@
> +    this.messageManager.removeMessageListener("GeckoView:RestoreStateBegin",
> +                                              this);
> +  }
> +
> +  collectFormData() {

It seems like this and collectScrollData could be conflated in order to avoid two frame walks. They are very similar.

@@ +203,5 @@
>          }, 500);
> +        break;
> +
> +      case "GeckoView:SaveStateBegin":
> +        if (this.__SS_data) {

We don't need to care over the __SS_data naming from SessionStore.js. Just _state or something similar is fine.

@@ +204,5 @@
> +        break;
> +
> +      case "GeckoView:SaveStateBegin":
> +        if (this.__SS_data) {
> +          // Short circuit and return the cached state if we're in the process of restoring

Seems more like "pending" state rather than cached

@@ +301,5 @@
>            type: "GeckoView:DOMWindowClose"
>          });
>          break;
> +      case "load":
> +        let formdata = this.__SS_data.formdata;

const

@@ +302,5 @@
>          });
>          break;
> +      case "load":
> +        let formdata = this.__SS_data.formdata;
> +        let scrolldata = this.__SS_data.scrolldata;

const

@@ +320,5 @@
> +  onLocationChange(aWebProgress, aRequest, aLocationURI, aFlags) {
> +    debug("onLocationChange");
> +
> +    if (this.__SS_data) {
> +      let scrolldata = this.__SS_data.scrolldata;

const
Attachment #8968676 - Flags: review?(snorp) → review-
Comment on attachment 8968676 [details] [diff] [review]
GeckoSession save/restore patch 1 (v2)

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

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +83,5 @@
> +  collectFormData() {
> +    // Store the main content
> +    let formdata = FormData.collect(content) || {};
> +
> +    // Loop over direct child frames, and store the text data

Instead of just copying the current Fennec code, might it be worth pinching mapFrameTree [1] from Desktop's content-sessionStore.js and use it like in [2] or maybe adapt it to collect both form data and scroll positions in one go?
1. This means we would be no longer limited to just on level of child frames.
2. To recurse into the child frames, a helper function (ssu.forEachNonDynamicChildFrame) is used that I seem to remember was specifically added as a Quantum Flow performance optimisation. You'd have to first move nsISessionStoreUtils from browser to toolkit, though.

[1] https://dxr.mozilla.org/mozilla-central/rev/5ded36cb383d3ccafd9b6c231c5120dcdae196a2/browser/components/sessionstore/content/content-sessionStore.js#57-81
[2] https://dxr.mozilla.org/mozilla-central/rev/5ded36cb383d3ccafd9b6c231c5120dcdae196a2/browser/components/sessionstore/content/content-sessionStore.js#487

@@ +307,5 @@
> +        if (formdata) {
> +          FormData.restoreTree(content, formdata);
> +        }
> +        if (scrolldata) {
> +          ScrollPosition.restoreTree(content, scrolldata);

I'm happy to be convinced otherwise, but from when I last looked at the MobileViewportManager, I seem to remember that depending on when the first paint happens, the resolution we've restored below might not be applied until after the "load" event.
Since scrolling depends on the correct resolution having been set (scrolling might only make sense if the document has been zoomed in), we can only restore the scroll position afterwards.
Since there's no guarantee which "load" event listener (ours, or the MVM's) is called first, for restoring the scroll position we'd have to either use a different event that happens later (pageshow) or at least spin the event loop once (I seem to remember there's some new helper function that can be used instead of setTimeout(..., 0), but can't remember its name right now).
Attachment #8968676 - Flags: review-
I think this addresses everything snorp brought up, as well as the potential issue with restoring scroll position on "load" that Jan caught.
Attachment #8968676 - Attachment is obsolete: true
Attachment #8968676 - Flags: review?(nchen)
Attachment #8969083 - Flags: review?(snorp)
Attachment #8969083 - Flags: review?(nchen)
Comment on attachment 8969083 [details] [diff] [review]
GeckoSession save/restore patch 1 (v3)

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

I think we should avoid GeckoView:SaveStateFinish if that's possible, otherwise just nits

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +79,5 @@
> +    let history = SessionHistory.collect(docShell);
> +    let scrollpos = ScrollPosition.collect(content) || {};
> +    let formdata = FormData.collect(content) || {}
> +
> +    // Loop over direct child frames, and store the form data and scroll positions.

Maybe file a bug regarding the deeper frame walk that Jan pointed out.

@@ +123,5 @@
> +    displaySize.height = height.value;
> +
> +    scrollpos.zoom.displaySize = displaySize;
> +
> +    return {history: history, formdata: formdata, scrolldata: scrollpos};

you can use { history, formdata, scrolldata: scrollpos }

@@ +194,5 @@
> +          // Short circuit and return the pending state if we're in the process of restoring
> +          sendAsyncMessage("GeckoView:SaveStateFinish", {state: this._savedState});
> +        } else {
> +          let state = this.collectSessionState();
> +          sendAsyncMessage("GeckoView:SaveStateFinish", {state: state});

Can we just reply to GeckoView:SaveState?

@@ +208,5 @@
> +          addEventListener("load", this, true);
> +          addEventListener("pageshow", this, true);
> +
> +          let flags = Ci.nsIWebProgress.NOTIFY_LOCATION;
> +          this.progressFilter.addProgressListener(this, flags);

I think you can set up the progress filter in init() once and just do webProgress.addProgressListener() here.

@@ +323,5 @@
> +
> +      this.progressFilter.removeProgressListener(this);
> +      let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIWebProgress);
> +      webProgress.removeProgressListener(this.progressFilter);

I think this line can go outside of the condition
Attachment #8969083 - Flags: review?(snorp) → review-
Comment on attachment 8969083 [details] [diff] [review]
GeckoSession save/restore patch 1 (v3)

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

Discussed with Dylan and realized you can't reply to the bundle message because it was forwarded through the MessageManager. Unfortunate, and we should fix that, but don't need to do it in this bug.
Attachment #8969083 - Flags: review- → review+
Addressed snorp's feedback and added a full frame tree traversal per Jan's comment.
Attachment #8969083 - Attachment is obsolete: true
Attachment #8969083 - Flags: review?(nchen)
Attachment #8969293 - Flags: review?(snorp)
Attachment #8969293 - Flags: review?(nchen)
Attachment #8969293 - Flags: review?(snorp) → review+
And here's the Java side of the patch.
Attachment #8966256 - Attachment is obsolete: true
Attachment #8969719 - Flags: review?(snorp)
Attachment #8969719 - Flags: review?(nchen)
Comment on attachment 8969719 [details] [diff] [review]
GeckoSession save/restore patch 2 (v2)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1067,5 @@
>  
> +    /**
> +     * Class representing a saved session state.
> +     */
> +    public static class SavedState implements Parcelable {

I like SessionState instead

@@ +1072,5 @@
> +        private String mState;
> +
> +        /**
> +         * Construct a SavedState from a String.
> +         * @param state A String representing a SavedState; should originate as output 

whitespace

@@ +1094,5 @@
> +        public void writeToParcel(final Parcel dest, final int flags) {
> +            dest.writeString(mState);
> +        }
> +
> +        // AIDL code may call readFromParcel even though it's not part of Parcelable.

We've cargo culted this around in a bunch of places, but I don't know why it's actually needed. I think this was mostly a convention to instantiate with a no-arg constructor and then call readFromParcel() in createFromParcel(). Regardless, we're not using this currently, so I'd remove it unless someone knows why it's necessary.

@@ +1118,5 @@
> +     * Save the current browsing session state of this GeckoSession. This session state
> +     * includes the history, scroll position, zoom, and any form data that has been entered,
> +     * but does not include information pertaining to the GeckoSession itself (for example,
> +     * this does not include settings on the GeckoSession).
> +     * @param response This is a response which will be called with the state once it has been

Include the caveat that the state will be null in the case of an error.

@@ +1126,5 @@
> +        mEventDispatcher.dispatch("GeckoView:SaveState", null, new EventCallback() {
> +            @Override
> +            public void sendSuccess(final Object result) {
> +                final SavedState state = new SavedState((String) result);
> +                response.respond(state);

response.respond(new SavedState((String)result)) seems more concise

@@ +1140,5 @@
> +    /**
> +     * Restore a saved state to this GeckoSession; only data that is saved (history, scroll
> +     * position, zoom, and form data) will be restored. These will overwrite the corresponding
> +     * state of this GeckoSession.
> +     * @param state A saved session state; this should originate from GeckoSession.saveState().

...or a SavedState that you created yourself, no?
Attachment #8969719 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #34)
> Comment on attachment 8969719 [details] [diff] [review]
> GeckoSession save/restore patch 2 (v2)
> 
> Review of attachment 8969719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1094,5 @@
> > +        public void writeToParcel(final Parcel dest, final int flags) {
> > +            dest.writeString(mState);
> > +        }
> > +
> > +        // AIDL code may call readFromParcel even though it's not part of Parcelable.
> 
> We've cargo culted this around in a bunch of places, but I don't know why
> it's actually needed. I think this was mostly a convention to instantiate
> with a no-arg constructor and then call readFromParcel() in
> createFromParcel(). Regardless, we're not using this currently, so I'd
> remove it unless someone knows why it's necessary.

I'll defer to Jim on this one (see comment 25).
 
> @@ +1140,5 @@
> > +    /**
> > +     * Restore a saved state to this GeckoSession; only data that is saved (history, scroll
> > +     * position, zoom, and form data) will be restored. These will overwrite the corresponding
> > +     * state of this GeckoSession.
> > +     * @param state A saved session state; this should originate from GeckoSession.saveState().
> 
> ...or a SavedState that you created yourself, no?

Maybe this is badly worded (or just unnecessary?) -- essentially I just want to be clear that the consumer can't/shouldn't use the string constructor to create a genuinely new SavedState, only to recreate a SavedState that was originally produced by GeckoSession.saveState().
> > > +        // AIDL code may call readFromParcel even though it's not part of Parcelable.
> > 
> > We've cargo culted this around in a bunch of places, but I don't know why
> > it's actually needed. I think this was mostly a convention to instantiate
> > with a no-arg constructor and then call readFromParcel() in
> > createFromParcel(). Regardless, we're not using this currently, so I'd
> > remove it unless someone knows why it's necessary.
> 
> I'll defer to Jim on this one (see comment 25).

AIDL-generated code can call `readFromParcel` if it's an out parameter. e.g.

> boolean loadState(out GeckoSession.SavedState state);

will generate a `readFromParcel` call.
Comment on attachment 8969293 [details] [diff] [review]
GeckoSession save/restore patch 1 (v4)

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

Looks good overall!

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +7,5 @@
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +ChromeUtils.defineModuleGetter(this, "SessionHistory", "resource://gre/modules/sessionstore/SessionHistory.jsm");
> +ChromeUtils.defineModuleGetter(this, "FormData", "resource://gre/modules/FormData.jsm");
> +ChromeUtils.defineModuleGetter(this, "ScrollPosition", "resource://gre/modules/ScrollPosition.jsm");

Put these into `defineLazyModuleGetters` call below

@@ +43,5 @@
>                                             this);
> +    this.messageManager.addMessageListener("GeckoView:SaveState",
> +                                           this);
> +    this.messageManager.addMessageListener("GeckoView:RestoreState",
> +                                           this);

You probably want to move these to onInit() because you want this to work even if there is no content delegate

@@ +49,5 @@
> +    this.progressFilter =
> +      Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
> +      .createInstance(Ci.nsIWebProgress);
> +    this.flags = Ci.nsIWebProgress.NOTIFY_LOCATION;
> +    this.progressFilter.addProgressListener(this, flags);

If this is only used for restoring, we should only add the listener when actually restoring.

@@ +121,5 @@
> +    // Save some data that'll help in adjusting the zoom level
> +    // when restoring in a different screen orientation.
> +    let displaySize = {};
> +    let width = {}, height = {};
> +    content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).getContentViewerSize(width, height);

Combine this `getInterface` call with the previous one.

@@ +152,4 @@
>          }
>          break;
>  
> +      case "GeckoView:ZoomToInput":

Keep the braces.

@@ +199,5 @@
> +          // Short circuit and return the pending state if we're in the process of restoring
> +          sendAsyncMessage("GeckoView:SaveStateFinish", {state: this._savedState});
> +        } else {
> +          let state = this.collectSessionState();
> +          sendAsyncMessage("GeckoView:SaveStateFinish", {state: state});

Probably want to call `JSON.stringify` on this side, just to be consistent with the restoring case.

@@ +210,5 @@
> +        if (this._savedState.history) {
> +          let restoredHistory = SessionHistory.restore(docShell, this._savedState.history);
> +
> +          addEventListener("load", this, true);
> +          addEventListener("pageshow", this, true);

Use options object: `{capture: true, mozSystemGroup: true, once: true}`

@@ +292,4 @@
>            type: "GeckoView:DOMWindowClose"
>          });
>          break;
> +      case "load":

Use braces.

@@ +296,5 @@
> +        const formdata = this._savedState.formdata;
> +        if (formdata) {
> +          FormData.restoreTree(content, formdata);
> +        }
> +        removeEventListener("load", this, true);

Don't need this with the "once" option.

@@ +298,5 @@
> +          FormData.restoreTree(content, formdata);
> +        }
> +        removeEventListener("load", this, true);
> +        break;
> +      case "pageshow":

Use braces.

@@ +303,5 @@
> +        const scrolldata = this._savedState.scrolldata;
> +        if (scrolldata) {
> +          ScrollPosition.restoreTree(content, scrolldata);
> +        }
> +        removeEventListener("pageshow", this, true);

Don't need this with the "once" option.

::: mobile/android/modules/geckoview/GeckoViewContent.jsm
@@ +66,5 @@
>          }
>          break;
> +      case "GeckoView:SaveState":
> +        this.messageManager.sendAsyncMessage("GeckoView:SaveState");
> +        this._saveStateCallback = aCallback;

This doesn't support multiple concurrent `saveState` calls.
Attachment #8969293 - Flags: review?(nchen) → review+
Comment on attachment 8969719 [details] [diff] [review]
GeckoSession save/restore patch 2 (v2)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1117,5 @@
> +    /**
> +     * Save the current browsing session state of this GeckoSession. This session state
> +     * includes the history, scroll position, zoom, and any form data that has been entered,
> +     * but does not include information pertaining to the GeckoSession itself (for example,
> +     * this does not include settings on the GeckoSession).

Newline before @param

@@ +1139,5 @@
> +
> +    /**
> +     * Restore a saved state to this GeckoSession; only data that is saved (history, scroll
> +     * position, zoom, and form data) will be restored. These will overwrite the corresponding
> +     * state of this GeckoSession.

Same here
Attachment #8969719 - Flags: review?(nchen) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8ba68c5138
Add session save/restore code to GeckoViewContent r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff4e915c722
Add GeckoSession.SavedState and session save/restore API to GeckoSession r=snorp,jchen
I missed a couple of things that need to be in onInit rather than onEnable; this patch is needed to work if we haven't set a ContentDelegate.
Attachment #8970325 - Flags: review?(nchen)
Comment on attachment 8970325 [details] [diff] [review]
GeckoSession save/restore patch 3

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

::: mobile/android/chrome/geckoview/GeckoViewContent.js
@@ +24,5 @@
> +
> +    this.progressFilter =
> +      Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
> +      .createInstance(Ci.nsIWebProgress);
> +    this.flags = Ci.nsIWebProgress.NOTIFY_LOCATION;

I would create this in the "GeckoView:RestoreState" listener, unless there's good reason not to.
Attachment #8970325 - Flags: review?(nchen) → review+
https://hg.mozilla.org/mozilla-central/rev/ef8ba68c5138
https://hg.mozilla.org/mozilla-central/rev/2ff4e915c722
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1456391
Flags: needinfo?(esawin)
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2197be608baf
Ensure session save/restore works even if no ContentDelegate is set. r=jchen
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.