1509110 - GeckoView: Expose "history stack" of a session and allow navigating to index

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In Android Components and the Reference browser we want to show a bottom panel with the "history" stack of a tab and allow the user to navigate to a specific page in the tab's history.

As an example long press on the "back" button in Fennec.
Screenshot: https://user-images.githubusercontent.com/28052/47804929-6fcf4200-dd0c-11e8-99c1-b42f7c88fe43.jpeg

This bug is more about a discussion around how a consumer of GV can implement such a feature than a concrete API request.

For developing the component we most likely want:

* (1) A history stack with all URLs visited in a session
* (2) An index that points to the position in the stack the session is currently in 
* (3) A way to navigate to a specific index or multiple steps back/forward

For (1) there was some discussion on Slack whether GV should provide this list or whether the app is reposible for maintaining this list itself with the help of the HistoryDelegate. A risk here is that the app list and the list gecko has could get out of sync and then navigating the history would not be possible anymore. WebView offers copyBackForwardList().

For (2) I couldn't find anything in WebView. There's canGoBackOrForward(steps) but you'd have to do some kind of binary search on that to determine the index you are at.

For (3) WebView has goBackOrForward (int steps).

Comment 1

[Chris Peterson [:cpeterson]]

The Fenix team says this is a P2 for Fenix MVP.

Comment 2

[Jonathan Almeida [:jonalmeida]]

When FireTV moves to GeckoView, they would also need this for some custom handling of YoutubeTV links.

See [0] and [1].


[0]: https://github.com/mozilla-mobile/firefox-tv/issues/1542
[1]: https://github.com/mozilla-mobile/firefox-tv/issues/1543

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Do we need onVisited if we have onHistoryListUpdate? (I'm assuming lastVisitedURL would be uris[currentIndex + 1] on the new API)

Comment 5

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

In my opinion, GeckoSession.goto() is a pretty vague name; WebView uses goBackOrForward which is a bit verbose but a lot clearer. Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

Comment 6

[Chris Peterson [:cpeterson]]

Sebastian, does this history API need to be synchronous?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Dylan Roeh (:droeh) from comment #5)

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)

What do people think about the following API?

interface HistoryDelegate {
    ...
    void onHistoryListUpdate(String[] uris, int currentIndex);
}

class GeckoSession {
    ...
    void goto(int index);
} 

Instead of an array of String in onHistoryListUpdate we could have a HistoryItem which would look a lot like the arguments to HistoryDelegate.onVisited, which looks like:

@Nullable GeckoResult<Boolean> onVisited(@NonNull GeckoSession session,
                                         @NonNull String url,
                                         @Nullable String lastVisitedURL,
                                         @VisitFlags int flags)

In particular, I think the flags would be useful in onHistoryListUpdate. The WebView history item[0] has some stuff like title and favicon, but I don't think those really belong here.

[0] https://developer.android.com/reference/android/webkit/WebHistoryItem

In my opinion, GeckoSession.goto() is a pretty vague name; WebView uses goBackOrForward which is a bit verbose but a lot clearer.

Yeah. Maybe gotoHistoryIndex? I guess goBackOrForward works too.

Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Maybe instead of a separate array/index pair we just have a List subclass that includes the current index? That way the app can just store a reference to that whole thing. For relative navigation I was thinking we would just add an overload for goForward and goBack that take an offset. So goBack(2) will go back two spots in the list. goBack() would chain to goBack(1), etc.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

I think that's part of a larger philosophical discussion of "should we save state in GeckoSession". I still don't think we need that, but we should instead fire initial events for things like onCanGoBack.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Some further thoughts on this:

I think the "history stack" object is very similar to a saved state in practice, and possibly we should be using the same underlying representation for both (albeit perhaps with different exposed interfaces?).

Likewise, from an implementation standpoint, onHistoryListUpdate is essentially just a subset of the state saved callback (let's tentatively say onStateSaved); onHistoryListUpdate should be called whenever onStateSaved is called due to navigation (but not for scrolling or form data changes).

Those things considered, I think this bug should be viewed as pretty closely related to bug 1463878

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)

Also, assuming that onHistoryListUpdate and goto use the same indexing scheme, this requires the app to store the current index in order for goto to be useful in most cases; having goto use relative indexing (so that the current history entry is index 0) makes goto a bit more useful on its own.

Maybe instead of a separate array/index pair we just have a List subclass that includes the current index? That way the app can just store a reference to that whole thing. For relative navigation I was thinking we would just add an overload for goForward and goBack that take an offset. So goBack(2) will go back two spots in the list. goBack() would chain to goBack(1), etc.

I would suggest we expand onCanGoBack and onCanGoForward to supply ints indicating how far back/forward you can navigate, in that case. Also, if we have relative navigation with goBack and goForward, do we actually need goto at all? The case for absolute navigation seems pretty thin imo.

Also, I know it's come up before, but I think this is a good time to reconsider having canGoBack(), canGoForward(), and maybe something like gotoRange() (which would give the range of valid input for goto) so the app doesn't have the overhead of maintaining these and the possible bugs associated with getting them out of sync. I suppose you could add something like currentHistoryIndex() in here as well to address the indexing issue above.

I think that's part of a larger philosophical discussion of "should we save state in GeckoSession". I still don't think we need that, but we should instead fire initial events for things like onCanGoBack.

In what circumstances would we fire initial events? I thought we already did when loading a saved state, are there other cases?

Comment 9

[Jan Henning [:JanH]]

I think the "history stack" object is very similar to a saved state in practice, and possibly we should be using the same underlying representation for both (albeit perhaps with different exposed interfaces?).

I think I'd agree with that line of thinking.

Also, if we have relative navigation with goBack and goForward, do we actually need goto at all? The case for absolute navigation seems pretty thin imo.

I'd think that for things like the session history popup it would feel much more natural to ideally just call goto with the index of the item that was selected instead of having to calculate a relative offset to the current history entry and then deciding whether that needs to be passed to goBack or to goForward.

Comment 10

[Chris Peterson [:cpeterson]]

P1 and [geckoview:fenix:m2] because this history feature is a must-have for Fenix M2:

https://github.com/mozilla-mobile/fenix/issues/351

Comment 11

[Chris Peterson [:cpeterson]]

Dylan said he would look at this bug as part of his work on the new saveState API.

Comment 12

[Chris Peterson [:cpeterson]]

[geckoview:fenix:m2] not [geckoview:fenix:p1]

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1509110 - Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin!,#geckoview-reviewers!

Comment 15

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1509110 - Add testing for onHistoryStateChange and gotoHistoryIndex. r=#geckoview-reviewers

Depends on D24439

Comment 16

[Chris Peterson [:cpeterson]]

67=wontfix. Fenix MVP will use GeckoView 68, so we don't need to uplift this fix to 67 Beta.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3338782b169
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/81253689d39c
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp

Comment 18

[Andreea Pavel [:apavel]]

Backed out for failing geckoview

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=239707663&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-geckoview-junit-e10s-7%2C%28gv-junit7%29&revision=81253689d39c5a08134cf122dcc0e092e38db643

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239707663&repo=autoland&lineNumber=1966

Backout: https://hg.mozilla.org/integration/autoland/rev/15f3d68875c0d0855a7fdff26ca54d4ce262d03f

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ed5bb5e0cfc
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/1bc29703a90f
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp

Comment 20

[Dylan Roeh (:droeh) (he/him)]

(In reply to Andreea Pavel [:apavel] from comment #18)

Backed out for failing geckoview

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=239707663&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-geckoview-junit-e10s-7%2C%28gv-junit7%29&revision=81253689d39c5a08134cf122dcc0e092e38db643

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239707663&repo=autoland&lineNumber=1966

Backout: https://hg.mozilla.org/integration/autoland/rev/15f3d68875c0d0855a7fdff26ca54d4ce262d03f

Forgot to clear hanging onHistoryStateChange callbacks after each test and introduced some raciness. Should be good now (at least it looks good locally.)

Comment 21

[Oana Pop-Rus]

Backed out for geckoview failures on HistoryDelegateTest

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=1bc29703a90ff8dc7d480cbae976cb02591be4a8&selectedJob=239729164

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239729164&repo=autoland&lineNumber=1941

Backout: https://hg.mozilla.org/integration/autoland/rev/55c2f04c1751ae9d237646cb114b2533b86fc026

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a8350404d6c
Add HistoryItem and HistoryList classes, HistoryDelegate.onHistoryStateChange callback, and GeckoSession.gotoHistoryIndex. r=esawin,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/bc0600ed24ce
Add testing for onHistoryStateChange and gotoHistoryIndex. r=geckoview-reviewers,snorp

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9a8350404d6c
https://hg.mozilla.org/mozilla-central/rev/bc0600ed24ce