1494713 - Add HistoryDelegate

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

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

This delegate would be responsible for delivering relevant history events.

Comment 2

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

I think this thing is going to be pretty simple really:

interface HistoryDelegate {
    void onVisited(String uri, ...other stuff?);

    GeckoResult<List<Boolean>> onGetVisited(List<String> uris);
}

onVisited() would be called whenever Gecko deems it appropriate to commit a visit to the history. This would probably just be nsISHistoryListener::OnHistoryNewEntry(), possibly with some additional metadata.

onGetVisitedLinks() would be called with a set of URIs for which Gecko needs the visited status. The app is expected to map the list into Booleans representing whether or not a given URI has been visited. The operation is asynchronous via GeckoResult.

Comment 3

[Lina Butler [:lina]]

Thanks, Snorp!

Our schema for the Rust sync and storage library is based on Places, and records extra metadata for each visit (https://github.com/mozilla/application-services/blob/ce433d55b7230d1ec11b1648cb86a13d6447b923/places/src/observation.rs#L23-L30), so I think we're looking for an equivalent of https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/docshell/base/IHistory.h#110-112,124.

In particular, for `onVisited`, it would also be helpful to know the referring URI, page title, and some info about the visit (https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/toolkit/components/places/History.cpp#2200,2204,2207,2214,2222,2228,2231,2237). We'd also like to know about page title changes, either via a separate method, or as an argument to `onVisited`.

`onGetVisited` looks good, I think we can implement that pretty easily, and it avoids having to expose IHistory's visited callbacks.

Will GeckoView ever cache the results of `onGetVisited`, or can we expect that it'll call it for every page? (If cached, should we have a way for the app to invalidate that cache, like when the user removes visits from their history?)

Comment 4

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

(In reply to Lina Cambridge (she/her) [:lina] from comment #3)
> Thanks, Snorp!
> 
> Our schema for the Rust sync and storage library is based on Places, and
> records extra metadata for each visit
> (https://github.com/mozilla/application-services/blob/
> ce433d55b7230d1ec11b1648cb86a13d6447b923/places/src/observation.rs#L23-L30),
> so I think we're looking for an equivalent of
> https://searchfox.org/mozilla-central/rev/
> a11c128b90ea85d7bb68f00c5de52c0794e0b215/docshell/base/IHistory.h#110-112,
> 124.
> 
> In particular, for `onVisited`, it would also be helpful to know the
> referring URI, page title, and some info about the visit
> (https://searchfox.org/mozilla-central/rev/
> 3c85ea2f8700ab17e38b82d77cd44644b4dae703/toolkit/components/places/History.
> cpp#2200,2204,2207,2214,2222,2228,2231,2237).

Yeah, that should be fine. Let's just put all of that into a 'Visit' class.

> We'd also like to know about
> page title changes, either via a separate method, or as an argument to
> `onVisited`.

We support title changes via ContentDelegate currently.

> 
> `onGetVisited` looks good, I think we can implement that pretty easily, and
> it avoids having to expose IHistory's visited callbacks.
> 
> Will GeckoView ever cache the results of `onGetVisited`, or can we expect
> that it'll call it for every page? (If cached, should we have a way for the
> app to invalidate that cache, like when the user removes visits from their
> history?)

I think we'd definitely want to cache the results in some kind of LRU map. We'd then only call this for links that have misses or negative values in the cache.

For eviction I was thinking we'd have a single clearCaches() method on GeckoSession (or GeckoRuntime more likely) with flags for which things to clear. The visited links would be one of these.

We should also consider disconnecting the internal history storage along with this work.

Comment 5

[Lina Butler [:lina]]

Hi Snorp! I've been poking at this bug for a bit, and came up with a questions and a vague plan.

For new visits, I think we can have our implementation of https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/docshell/base/IHistory.h fire a `GeckoView:VisitURI` event. The Java handler for that event would call into our history delegate, which would do whatever it needs to store the visit.

For checking the visit statuses of links, I was thinking we'd have our implementation of https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/docshell/base/IHistory.h#51 add to an in-memory map of `dom::Link`s, and dispatch a `GeckoView:GetVisited` event with the URLs (on a timer, so that we could coalesce multiple links). We'd then call the delegate's `onGetVisited` method, and attach a value listener that sends a `GeckoView:GotVisited` event back to Gecko, with the URLs and visited states for each. The `GotVisited` handler in Gecko would map those URLs back to `dom::Link`s, update the visited status, and notify the document (as we do in Places, in https://searchfox.org/mozilla-central/source/toolkit/components/places/History.cpp#1593,1609,1614,1622,1643,1670).

Does that sound reasonable to you? Are there things we can simplify? You mentioned using `nsISHistoryListener::OnHistoryNewEntry()` (I'm guessing piggybacking on https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/mobile/android/chrome/content/browser.js#4734?), would that be easier instead of trying to implement `IHistory`?

I thought I'd also try to dispatch a `VisitURI` event from C++, just to see if this would even work.

First, I changed `nsAndroidHistory` to forward the visit to the parent process if we're in content, like https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/toolkit/components/places/History.cpp#2142-2154. Then, in the parent, instead of calling into `java::GlobalHistory`, I'm using the `AndroidBridge` dispatcher to dispatch `GeckoView:VisitURI`, and rejiggered `AndroidBridge` to expose `widget::EventDispatcher` so that `nsAndroidHistory::VisitURI` could call https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/widget/android/EventDispatcher.cpp#853-884 instead of going through `nsIAndroidEventDispatcher`.

On the Java side, I added a listener for `GeckoView:VisitURI`, and a `HistoryDelegate` interface. I tried both using a new `GeckoSessionHandler<HistoryDelegate>`, as we do for `GeckoSessionHandler<ContentDelegate>`, and storing the delegate directly on `GeckoSession` and adding the event to https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#844-847.

It looks like the Java side is getting the event, but not calling the Java listener. I tried dumping listeners for other message types, and I see things like:

> 10-11 01:01:02.975  5577  5592 W GeckoEventDispatcher: Dispatching GeckoView:ClearAutoFill; available events: GeckoView:ProgressChanged, GeckoView:AccessibilityEvent, GeckoView:ContextMenu, GeckoView:AddAutoFill, GeckoView:DOMWindowClose, GeckoView:PinOnScreen, GeckoView:AndroidPermission, GeckoView:OnLoadRequest, GeckoView:SecurityChanged, GeckoView:ClearAutoFill, GeckoView:OnLoadError, GeckoView:PageStop, GeckoView:ShowSelectionAction, GeckoView:OnAutoFillFocus, GeckoView:DOMTitleChanged, GeckoView:HideSelectionAction, GeckoView:LocationChange, GeckoView:OnNewSession, GeckoView:DOMWindowFocus, GeckoView:ExternalResponse, GeckoView:FullScreenExit, GeckoView:VisitURI, GeckoView:MediaPermission, GeckoView:FullScreenEnter, GeckoView:Prompt, GeckoView:ContentCrash, GeckoView:PageStart, GeckoView:ContentPermission, GeckoView:TrackingProtectionBlocked
> 10-11 01:01:02.978  5577  5592 W GeckoEventDispatcher: Dispatching GeckoView:VisitURI; available events: Wifi:GetIPAddress, Wifi:Enable
> 10-11 01:01:02.978  5577  5592 W GeckoEventDispatcher: No listener for GeckoView:VisitURI

So, it looks like it's being called from the main process, and on the UI thread, and Java receives the `GeckoView:VisitURI` message...but why are there two different sets of listeners?

I guess my questions are:

1. Does that plan for forwarding history visits to the delegate make sense?
2. How does the event dispatcher work? :-)

Comment 6

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

(In reply to Lina Cambridge (she/her) [:lina] from comment #5)
> 
> Does that sound reasonable to you? Are there things we can simplify? You
> mentioned using `nsISHistoryListener::OnHistoryNewEntry()` (I'm guessing
> piggybacking on
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/mobile/android/chrome/content/
> browser.js#4734?), would that be easier instead of trying to implement
> `IHistory`?

My original plan was to use nsISHistoryListener, yes, but that wouldn't have handled the onGetVisited() case anyways, so implementing IHistory seems better from that standpoint. It also looks like we get the visit flags in IHistory, which is apparently critical information for a places-like implementation.

One thing to consider, though, is that we'll need to keep nsAndroidHistory as-is for Fennec. We don't have any compile-time checks because they use the same libxul, but we have some runtime checks[3].

> 
> I thought I'd also try to dispatch a `VisitURI` event from C++, just to see
> if this would even work.
> 
> First, I changed `nsAndroidHistory` to forward the visit to the parent
> process if we're in content, like
> https://searchfox.org/mozilla-central/rev/
> 80ac71c1c54af788b32e851192dfd2de2ec18e18/toolkit/components/places/History.
> cpp#2142-2154.

Great!

> Then, in the parent, instead of calling into
> `java::GlobalHistory`, I'm using the `AndroidBridge` dispatcher to dispatch
> `GeckoView:VisitURI`, and rejiggered `AndroidBridge` to expose
> `widget::EventDispatcher` so that `nsAndroidHistory::VisitURI` could call
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/widget/android/EventDispatcher.
> cpp#853-884 instead of going through `nsIAndroidEventDispatcher`.
> 

This is where you're having a problem. We have a global EventDispatcher and then one dispatcher per top-level nsWindow, which lives in GeckoSession on the Java side. The delegates for GeckoSession use the latter.

We don't actually use the GeckoSession dispatcher from C++ anywhere yet, so first there is some plumbing to do there. We receive the EventDispatcher instance from Java in the open() native call[0], then pass that instance to our impl of nsIAndroidView which implements the nsIAndroidEventDispatcher. That's how we get at the session dispatcher from JS. We can probably just store the mozilla::widget::EventDispatcher() in the nsWindow instead, add a getter, and pass that instance over to the nsWindow::AndroidView instance for exposing nsIAndroidEventDispatcher.

You'd then need to find a way to access the nsWindow and therefore the EventDispatcher from within your IHistory implementation. It looks like this may be possible through dom::Link, but I'm not totally sure. Probably something like dom::Link::GetElement[1], then nsContentUtils::WidgetForContent[2], then cast to nsWindow (ugh).

> On the Java side, I added a listener for `GeckoView:VisitURI`, and a
> `HistoryDelegate` interface. I tried both using a new
> `GeckoSessionHandler<HistoryDelegate>`, as we do for
> `GeckoSessionHandler<ContentDelegate>`, and storing the delegate directly on
> `GeckoSession` and adding the event to
> https://searchfox.org/mozilla-central/rev/
> 80ac71c1c54af788b32e851192dfd2de2ec18e18/mobile/android/geckoview/src/main/
> java/org/mozilla/geckoview/GeckoSession.java#844-847.
> 
> It looks like the Java side is getting the event, but not calling the Java
> listener. I tried dumping listeners for other message types, and I see
> things like:
> 
> > 10-11 01:01:02.975  5577  5592 W GeckoEventDispatcher: Dispatching GeckoView:ClearAutoFill; available events: GeckoView:ProgressChanged, GeckoView:AccessibilityEvent, GeckoView:ContextMenu, GeckoView:AddAutoFill, GeckoView:DOMWindowClose, GeckoView:PinOnScreen, GeckoView:AndroidPermission, GeckoView:OnLoadRequest, GeckoView:SecurityChanged, GeckoView:ClearAutoFill, GeckoView:OnLoadError, GeckoView:PageStop, GeckoView:ShowSelectionAction, GeckoView:OnAutoFillFocus, GeckoView:DOMTitleChanged, GeckoView:HideSelectionAction, GeckoView:LocationChange, GeckoView:OnNewSession, GeckoView:DOMWindowFocus, GeckoView:ExternalResponse, GeckoView:FullScreenExit, GeckoView:VisitURI, GeckoView:MediaPermission, GeckoView:FullScreenEnter, GeckoView:Prompt, GeckoView:ContentCrash, GeckoView:PageStart, GeckoView:ContentPermission, GeckoView:TrackingProtectionBlocked
> > 10-11 01:01:02.978  5577  5592 W GeckoEventDispatcher: Dispatching GeckoView:VisitURI; available events: Wifi:GetIPAddress, Wifi:Enable
> > 10-11 01:01:02.978  5577  5592 W GeckoEventDispatcher: No listener for GeckoView:VisitURI
> 
> So, it looks like it's being called from the main process, and on the UI
> thread, and Java receives the `GeckoView:VisitURI` message...but why are
> there two different sets of listeners?
> 
> I guess my questions are:
> 
> 1. Does that plan for forwarding history visits to the delegate make sense?

Yes, assuming we can route to the correct EventDispatcher as outlined above.

> 2. How does the event dispatcher work? :-)

It's more complex than necessary, but most of your problems you're facing stem from the fact that we have two of them and you happened to be using the wrong one. Hopefully with that cleared up it'll be smooth sailing!

[0] https://searchfox.org/mozilla-central/source/widget/android/nsWindow.cpp#1252
[1] https://searchfox.org/mozilla-central/source/dom/base/Link.h#96
[2] https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.h#2238
[3] https://searchfox.org/mozilla-central/source/widget/android/jni/Utils.h#151

Comment 7

[Lina Butler [:lina]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> This is where you're having a problem. We have a global EventDispatcher and
> then one dispatcher per top-level nsWindow, which lives in GeckoSession on
> the Java side. The delegates for GeckoSession use the latter.

Ah! OK, that makes sense now. Thanks for the great explanation! :-)

> You'd then need to find a way to access the nsWindow and therefore the
> EventDispatcher from within your IHistory implementation. It looks like this
> may be possible through dom::Link, but I'm not totally sure. Probably
> something like dom::Link::GetElement[1], then
> nsContentUtils::WidgetForContent[2], then cast to nsWindow (ugh).

I'll give that a try for `onGetVisited`. `onVisited` is trickier, because we don't pass a link to `VisitURI`. I guess the Docshell has a document and a content window; maybe we can get at the widget from there, and change `IHistory::VisitURI`?

Alternatively, do you think it's worth making that a regular JNI call, instead of going through the message manager?

Comment 8

[Lina Butler [:lina]]

(In reply to Lina Cambridge (she/her) [:lina] from comment #7)
> Alternatively, do you think it's worth making that a regular JNI call,
> instead of going through the message manager?

I meant event dispatcher, not message manager. 😅

Comment 9

[Lina Butler [:lina]]

So...`WidgetForContent` bails because there's no primary frame. `WidgetForDocument` on the link's owner document works, but happens in the wrong process. (I'm also not sure if that's giving me the content or chrome document, or if that matters for getting the widget. There's also this note about preferring `WidgetForContent`: https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/dom/base/nsContentUtils.h#2227-2228). With e10s, we only have a link in the content process, and dispatching directly from content doesn't work.

But, `nsWindow::TopWindow()` in the parent gives me the widget. After adding a getter for the dispatcher, I was able to send an event to the session dispatcher, and have the `GeckoSession` handler receive it. \o/

Can we ever have multiple top-level `nsWindow`s, though? If so, how do we know which dispatcher to use? Do we need to broadcast the event to every window? Or do we need to find the widget in the content process, but dispatch to that same widget in the parent?

Comment 10

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

> Alternatively, do you think it's worth making that a regular JNI call, instead of going through the message manager?

We generally prefer messages to direct JNI calls when able. This will make it easier to move Gecko entirely out of process from the app, which we'll begin working on soon.

> Can we ever have multiple top-level `nsWindow`s, though? If so, how do we know which dispatcher to use? Do we need to broadcast the event to every window? Or do we need to find the widget in the content process, but dispatch to that same widget in the parent?

Yeah, we have multiple top-level `nsWindow`s. You only want to call `onGetVisited()` on the single `GeckoSession` that's affected, so you need to find the widget in the parent for the affected document.

Comment 11

[Lina Butler [:lina]]

Got the plumbing working 🎉, so snagging this bug. I'll push up a patch for feedback soon, but now 😴.

Comment 12

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Pass the parent chrome window to `IHistory::VisitURI`.

In the content process, or the parent in non-e10s mode, this is the
outer chrome window for the DocShell's content viewer. In the parent
in e10s mode, this is the outer window for the top-level `TabParent`
associated with the child content process.

This lets us find the top-level widget for the chrome window, so that
GeckoView can dispatch events to it.

MozReview-Commit-ID: DhKMX7MU5ok

Comment 13

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Expose `EventDispatcher::GetGlobalObject()` and `nsWindow::GetEventDispatcher()`.

`GetGlobalObject()` returns the global object for JSAPI, which we'll
use in the next patch to unpack JS objects passed to
`nsIAndroidEventCallback::OnSuccess`.

`GetEventDispatcher()` lets us access the per-window dispatcher, s
that we can send history events to the correct session.

MozReview-Commit-ID: AOIWAGPMvCL

Depends on D9342

Comment 14

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Implement a `GeckoViewHistory` backend.

This commit adds the plumbing to support a per-session history
delegate in Gecko. It works like Places on Desktop:

* We track links and their URIs in `mTrackedLinksByURI`. For new URIs
  passed to `RegisterVisitedCallback`, we send a new `GetVisited` IPC
  message to the parent. In the parent, we find the widget for the
  content window, and dispatch a `GeckoView:GetVisited` message
  containing the URIs to look up.
* We expect the handler on the Java side to call our `GetVisited`
  callback with an array of Booleans indicating whether the URI is
  visited. We parse the statuses out of the bundle, match them up to
  the requested URIs, then broadcast visit notifications to all content
  processes.
* We notify visited links about their visit status changes
  asynchronously, dispatching one runnable per DocGroup, and remove
  visited links from `mTrackedLinksByURI`.
* When the DocShell records a new visit, we send a `VisitURI` IPC
  message to the parent, as on Desktop. In the parent, we find the
  event dispatcher for the content window, and dispatch a
  `GeckoView:Visited` message.
* When the Java handler calls our `Visited` callback, we broadcast
  a link visit status change, just as we do for `GetVisited`.

MozReview-Commit-ID: 1rYrtDYO8QQ

Depends on D9343

Comment 15

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Add `HistoryDelegate` and wire up the Java history handlers.

MozReview-Commit-ID: IPcj0uMsfRP

Depends on D9344

Comment 16

[Lina Butler [:lina]]

This works for real now! 🎉🎉🎉 There's a pile o' TODOs that still need fixing, plus a few questions...but, if you build the example app, and visit a site with simple styles and lots of links, every other link should be marked as visited. I've been using https://danluu.com to test.

You should also see `Visited URI: ` messages scroll by in logcat.

[1]: `mach gradle geckoview_example:installLocalWithGeckoBinariesNoMinApiDebug`. You'll also need a full Fennec build, unfortunately, since this touches C++. 😅

Comment 17

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Pass the widget for the DocShell to `IHistory::VisitURI`. r?smaug!

This lets GeckoView's history implementation get the top-level
`nsIWidget` for the DocShell's outer window, so that it can
forward visits to the widget's event dispatcher.

Comment 18

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Expose `EventDispatcher::GetGlobalObject()` and `nsWindow::GetEventDispatcher()`. r?snorp!

`GetGlobalObject()` returns the global object for JSAPI, which we'll
use to unpack JS objects passed to
`nsIAndroidEventCallback::OnSuccess`.

`GetEventDispatcher()` lets us access the per-window event dispatcher,
so that we can forward history events to the correct session.

Depends on D11287

Comment 19

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Add `nsWindow::From`. r?jchen!

This centralizes the logic for casting `nsIWidget` to `nsWindow`.

Depends on D11288

Comment 20

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Implement a `GeckoViewHistory` backend. r?jchen!,snorp!,smaug!

This commit adds the plumbing to support a per-session history
delegate in Gecko. It works like Places on Desktop:

* We track links and their URIs in `mTrackedURIs`. For new URIs passed
  to `RegisterVisitedCallback`, we send a `GetVisited` IPC message to
  the parent, batching URIs to avoid IPC and JNI overhead. In the
  parent, we find the widget for the DocShell's outer window, and
  dispatch a `GeckoView:GetVisited` event with the URIs to look up.
* We expect the handler on the Java side to call our
  `GetVisitedCallback` with an array of Booleans indicating whether
  each URI is visited or not. We then get the statuses from the bundle,
  match them up to the URIs, then broadcast visit notifications to all
  content processes.
* We notify visited links about their visit status changes
  asynchronously, dispatching one runnable per DocGroup, then remove
  the visited links from `mTrackedURIs`.
* When the DocShell records a new visit, we send a `VisitURI` IPC
  message to the parent, as on Desktop. In the parent, we find the
  event dispatcher for the DocShell's outer window, and fire a
  `GeckoView:OnVisited` event.
* When the Java handler calls our `VisitedCallback`, we broadcast a
  link visit status change, just as we do for `GetVisited`.

Depends on D11289

Comment 21

[Lina Butler [:lina]]

Attachment: Bug 1494713 - Add `HistoryDelegate` and wire up the Java history handlers. r?jchen!,snorp!

Depends on D11290

Comment 22

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c2b16e0b7dfe52296d99c1f8877d728fad88b1a

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ab98d066db0
Pass the widget for the DocShell to `IHistory::VisitURI`. r=smaug
https://hg.mozilla.org/integration/autoland/rev/155387956608
Expose `EventDispatcher::GetGlobalObject()` and `nsWindow::GetEventDispatcher()`. r=snorp
https://hg.mozilla.org/integration/autoland/rev/6644b6e14bde
Add `nsWindow::From`. r=jchen
https://hg.mozilla.org/integration/autoland/rev/a1fe5c7791a7
Implement a `GeckoViewHistory` backend. r=jchen,snorp,smaug
https://hg.mozilla.org/integration/autoland/rev/900cd4bf995f
Add `HistoryDelegate` and wire up the Java history handlers. r=jchen,snorp

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3ab98d066db0
https://hg.mozilla.org/mozilla-central/rev/155387956608
https://hg.mozilla.org/mozilla-central/rev/6644b6e14bde
https://hg.mozilla.org/mozilla-central/rev/a1fe5c7791a7
https://hg.mozilla.org/mozilla-central/rev/900cd4bf995f

Comment 25

[Chris Peterson [:cpeterson]]

64=wontfix because we don't need to uplift this new API to Beta.

Comment 26

[Chris Peterson [:cpeterson]]

status-geckoview64=wontfix