1322592 - [geckoview] Add NavigationDelegate

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

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

This will contain things like onCanGoBack(), onCanGoForward(), onLocationChange().

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #0)
> This will contain things like onCanGoBack(), onCanGoForward(),
> onLocationChange().

Android's WebView has synchronous APIs for some of these things, I believe.  We most naturally support asynchronous APIs.  I have thought about this a little and conclude that most consumers will *want* synchronous APIs, but such APIs are likely to be a little flaky.  Do you (or others) have strong feelings here?

Comment 2

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

Can you elaborate a little? The things I name above don't appear to have much benefit of being synchronous to me, unless you want onLocationChange() to be able to somehow abort a location change. I'm definitely thinking about async stuff here.

If you're thinking of the navigation-related methods on GeckoView, I think those should also be async, for the normal reasons.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> Can you elaborate a little? The things I name above don't appear to have
> much benefit of being synchronous to me, unless you want onLocationChange()
> to be able to somehow abort a location change. I'm definitely thinking about
> async stuff here.

I was thinking about things like https://developer.android.com/reference/android/webkit/WebView.html#canGoBack(), which is synchronous.  Is the intention to have a built-in async NavigationDelegate that maintains state (using onCanGo{Back,Forward}()) such we can answer canGoBack() synchronously?  We pay a performance overhead for such things, but take an API burden off consumers by doing so.

I do think we want to be able to hook the request flow in general, but I don't think we want NavigationDelegate to be that API :)

Is that more clear?

Comment 4

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

Ah, I follow. Yes, my thinking was that we'd have an internal history observer that stores the state for canGoBack() and friends.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1322592-1.0-Add-GeckoView-navigation.patch

This adds the navigation delegate interface and also GeckoView.goBack and GeckoView.goForward.

It would make sense to move go{Back|Forward} into the delegate, but there could be some cases where the app doesn't need Gecko's opinion on canGo{Back|Forward} assuming its own authority on that state. Requiring to implement and instantiate the navigation delegate could be considered cumbersome in this case.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1322592-2.0-Add-GeckoViewNavigation.jsm.patch

I'm not sure where we would prefer to instantiate it, there are multiple alternatives to this approach.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1322592-3.0-Integrate-GeckoViewNavigation.patch

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0004-no-bug-Add-search-bar-and-navigation-to-GeckView-exa.patch

This is only for testing: it adds a simple search bar to the example app and implements basic navigation.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8828412 [details] [diff] [review]
0001-Bug-1322592-1.0-Add-GeckoView-navigation.patch

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

This is looking good!  Just some hygiene concerns.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +47,4 @@
>  
>      private ChromeDelegate mChromeDelegate;
>      private ContentDelegate mContentDelegate;
> +    private NavigationDelegate mNavigationDelegate;

These all need to be synchronized/guarded.  You should be explicit about which threads are allowed to call the relevant setters.

@@ +575,5 @@
> +        */
> +        public void onLocationChange(GeckoView view, String url);
> +
> +        /**
> +        * A status update on a view's ability to go back to a history item.

nit: use the active form.  "The view's ability to go back has changed."

@@ +577,5 @@
> +
> +        /**
> +        * A status update on a view's ability to go back to a history item.
> +        * @param view The GeckoView that initiated the callback.
> +        * @param value The new value for the status.

nit: embed the interpretation into the variable name, like "canGoBack".

@@ +583,5 @@
> +        public void onCanGoBack(GeckoView view, boolean value);
> +
> +        /**
> +        * A status update on a view's ability to go forward to a history item.
> +        * @param view The GeckoView that initiated the callback.

nit: same as above.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8828413 [details] [diff] [review]
0002-Bug-1322592-2.0-Add-GeckoViewNavigation.jsm.patch

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

::: mobile/android/modules/GeckoViewNavigation.jsm
@@ +27,5 @@
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {})
> +           .AndroidLog.d.bind(null, "ViewNavigation");
> +
> +class GeckoViewNavigation {

nit: class comment explaining what this is and how it fits into the window/process/<browser> model, please.  Who owns it, what does it own.  Is there a 1:1 relationship with GV windows?

@@ +52,5 @@
> +  onEvent(event, data, callback) {
> +    dump("onEvent: event=" + event + ", data=" + JSON.stringify(data));
> +
> +    switch (event) {
> +      case "GeckoViewNavigation:Active":

I'm missing something here.  I don't see the other end of these messages; was that dropped from a patch somewhere?  (From this patch or the previous one?)

::: mobile/android/modules/moz.build
@@ +32,5 @@
>      'WebsiteMetadata.jsm'
>  ]
> +
> +EXTRA_JS_MODULES += [
> +    'GeckoViewNavigation.jsm'

At some point we're going to want to put this GV-specific code in android://assets/... and not in omni.ja.  Doesn't have to be your problem now, I guess.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8828413 [details] [diff] [review]
0002-Bug-1322592-2.0-Add-GeckoViewNavigation.jsm.patch

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

::: mobile/android/modules/GeckoViewNavigation.jsm
@@ +52,5 @@
> +  onEvent(event, data, callback) {
> +    dump("onEvent: event=" + event + ", data=" + JSON.stringify(data));
> +
> +    switch (event) {
> +      case "GeckoViewNavigation:Active":

Sorry, my mistake, previous patch does have this.  OK.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8828414 [details] [diff] [review]
0003-Bug-1322592-3.0-Integrate-GeckoViewNavigation.patch

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

::: mobile/android/chrome/content/geckoview.js
@@ +21,5 @@
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {})
> +           .AndroidLog.d.bind(null, "View");
> +
> +// Define globals.

Comment on why you need to do this fancy exposure.

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1322592-1.1-Add-GeckoView-navigation.patch

Addressed feedback.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8828854 [details] [diff] [review]
0001-Bug-1322592-1.1-Add-GeckoView-navigation.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +39,4 @@
>  import android.view.inputmethod.InputConnection;
>  
>  public class GeckoView extends LayerView
> +    implements ContextGetter, BundleEventListener {

Put BundleEventListener in a child class, because we don't want GeckoView to have a public "handleMessage" method.

@@ +145,5 @@
>          initializeView();
> +        registerListeners();
> +    }
> +
> +    void registerListeners() {

/* package */ void ...

@@ +291,4 @@
>          mInputConnectionListener = icl;
>      }
>  
> +    public void goBack() {

Add javadoc.

@@ +294,5 @@
> +    public void goBack() {
> +        eventDispatcher.dispatch("GeckoView:GoBack", null);
> +    }
> +
> +    public void goForward() {

Add javadoc.

@@ +398,5 @@
> +    * Set the navigation callback handler.
> +    * This will replace the current handler.
> +    * @param navigation An implementation of NavigationDelegate.
> +    */
> +    public synchronized void setNavigationDelegate(NavigationDelegate navigation) {

Don't need synchronized?

@@ +571,5 @@
>          */
>          public void onReceivedFavicon(GeckoView view, String url, int size);
>      }
> +
> +    public interface NavigationDelegate {

This should be "NavigationListener" IMO. A delegate is for requesting operations. A listener is for notifying events. This interface is obviously for notifying events, as the method names all start with "on".

@@ +577,5 @@
> +        * A view has started loading content from the network.
> +        * @param view The GeckoView that initiated the callback.
> +        * @param url The resource being loaded.
> +        */
> +        public void onLocationChange(GeckoView view, String url);

Why do we need a GeckoView parameter here?

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8828413 [details] [diff] [review]
0002-Bug-1322592-2.0-Add-GeckoViewNavigation.jsm.patch

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

::: mobile/android/modules/GeckoViewNavigation.jsm
@@ +13,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm", "Services");
> +
> +XPCOMUtils.defineLazyGetter(this, "window",
> + () => Services.wm.getMostRecentWindow("navigator:browser"));

This won't work with multiple GeckoViews. You'd need geckoview.js to pass you the window (and potentially the WindowEventDispatcher).

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1322592-1.2-Add-GeckoView-navigation.patch

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1322592-2.1-Add-GeckoViewNavigation.jsm.patch

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1322592-3.1-Integrate-GeckoViewNavigation.patch

Comment 19

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

Comment on attachment 8831146 [details] [diff] [review]
0001-Bug-1322592-1.2-Add-GeckoView-navigation.patch

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

good with nits

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +125,5 @@
> +
> +            if ("GeckoView:LocationChange".equals(event)) {
> +                if (mNavigationListener == null) {
> +                    // We shouldn't be getting this event.
> +                    eventDispatcher.dispatch("GeckoViewNavigation:Inactive", null);

I think you should use a debug assert or just ignore. We should only do the active/inactive stuff when listeners are added/removed.

@@ +413,5 @@
> +    * Set the navigation callback handler.
> +    * This will replace the current handler.
> +    * @param navigation An implementation of NavigationListener.
> +    */
> +    public synchronized void setNavigationDelegate(NavigationListener listener) {

Rename to setNavigationListener (since you pass a NavigationListener)

Comment 20

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

Comment on attachment 8831147 [details] [diff] [review]
0002-Bug-1322592-2.1-Add-GeckoViewNavigation.jsm.patch

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

I think it would be nice if we factored out the listener active/inactive message handling into a base class, that way other modules don't have to do that by hand. They can just implement some activate() and deactivate() methods. This is ok for now.

Comment 21

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> Comment on attachment 8831146 [details] [diff] [review]
> 0001-Bug-1322592-1.2-Add-GeckoView-navigation.patch
> 
> Review of attachment 8831146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> good with nits
> 
> ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
> @@ +125,5 @@
> > +
> > +            if ("GeckoView:LocationChange".equals(event)) {
> > +                if (mNavigationListener == null) {
> > +                    // We shouldn't be getting this event.
> > +                    eventDispatcher.dispatch("GeckoViewNavigation:Inactive", null);
> 
> I think you should use a debug assert or just ignore. We should only do the
> active/inactive stuff when listeners are added/removed.

We can change that to an assertion once we have the "GeckoView:Ready" mechanics in place, until then that's the way to deactivate the listener since it's active by default.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8831146 [details] [diff] [review]
0001-Bug-1322592-1.2-Add-GeckoView-navigation.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +113,4 @@
>              };
>      }
>  
> +    private class EventListener implements BundleEventListener {

You'll want to merge this later with Dylan's patch in bug 1322590

@@ +140,5 @@
> +                "GeckoView:LocationChange",
> +                null);
> +        }
> +
> +        private void unregisterListeners() {

Don't need unregisterListeners if you're not using it.

@@ +413,5 @@
> +    * Set the navigation callback handler.
> +    * This will replace the current handler.
> +    * @param navigation An implementation of NavigationListener.
> +    */
> +    public synchronized void setNavigationDelegate(NavigationListener listener) {

Provide a getNavigationListener method, and I don't think we need synchronized? This should only be used on the UI thread anyways

@@ +418,5 @@
> +        if (mNavigationListener == listener) {
> +            return;
> +        }
> +        if (listener == null) {
> +            eventDispatcher.dispatch("GeckoViewNavigation:Inactive", null);

Maybe we want to keep internal GV events all under "GeckoView:*"?

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8831147 [details] [diff] [review]
0002-Bug-1322592-2.1-Add-GeckoViewNavigation.jsm.patch

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

::: mobile/android/modules/GeckoViewNavigation.jsm
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm", "Services");

Not using "Services"

@@ +56,5 @@
> +    return this.browser.messageManager;
> +  }
> +
> +  get eventDispatcher() {
> +    return EventDispatcher.for(this.window);

Pass in a WindowEventDispatcher from geckoview.js instead of creating one here.

@@ +93,5 @@
> +  }
> +
> +  // nsIBrowserDOMWindow::canClose implementation.
> +  canClose() {
> +    debug("canClose");

You should be returning something here. I guess false.

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8831148 [details] [diff] [review]
0003-Bug-1322592-3.1-Integrate-GeckoViewNavigation.patch

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

I think you'll want to wait till Dylan lands his patch, and then rebase yours, because his patch already addressed some comments that also apply to your patch.

::: mobile/android/chrome/content/geckoview.js
@@ +12,2 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +  "resource://gre/modules/Services.jsm", "Services");

"Services" is not used.

@@ +24,4 @@
>  
> +// Creates and manages GeckoView modules.
> +// This is more convenient than keeping one global reference for each module.
> +class ModuleManager {

Make it into an object? Since you'll only have one ModuleManager

@@ +30,5 @@
> +  }
> +
> +  add(resource, type, ...args) {
> +    this.remove(type);
> +    Cu.import(resource);

You should use a sandbox for this

@@ +47,5 @@
>  function startup() {
> +  dump("zerdatime " + Date.now() + " - geckoview chrome startup finished.");
> +
> +  // Modules depend on the browser global, attach to window for convenience.
> +  window.browser = document.getElementById("content");

Pass the browser into the modules instead of using window.browser.

Comment 25

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1322592-1.3-Add-GeckoView-navigation.-r-snorp-jc.patch

Addressed comments.

Comment 26

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1322592-2.2-Add-GeckoViewNavigation.jsm.-r-snorp.patch

Addressed comments.

Comment 27

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1322592-3.2-Add-GeckoViewModule-base-class-and-i.patch

I've added the GeckoViewModule base class to provide a clean way to initialize modules.
We can extend it to handle activation/deactivation and more eventually.

Comment 28

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1322592-4.0-Refactor-GeckoViewContent-to-extend-.patch

Refactor the GeckoViewContent module to use the new base class.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8835115 [details] [diff] [review]
0003-Bug-1322592-3.2-Add-GeckoViewModule-base-class-and-i.patch

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

::: mobile/android/modules/GeckoViewModule.jsm
@@ +12,5 @@
> +  constructor(window, browser, eventDispatcher) {
> +    this.window = window;
> +    this.browser = browser;
> +    this.eventDispatcher = eventDispatcher;
> +    

Extra spaces

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac925c366df9
[1.3] Add GeckoView navigation. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/091d2306c630
[2.2] Add GeckoViewNavigation.jsm. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8811e5e54bdf
[3.3] Add GeckoViewModule base class and integrate GeckoViewNavigation. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1c6e63f349
[4.0] Refactor GeckoViewContent to extend GeckoViewModule. r=jchen

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac925c366df9
https://hg.mozilla.org/mozilla-central/rev/091d2306c630
https://hg.mozilla.org/mozilla-central/rev/8811e5e54bdf
https://hg.mozilla.org/mozilla-central/rev/7c1c6e63f349