[geckoview] Add NavigationDelegate

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: snorp, Assigned: esawin)

Tracking

unspecified
mozilla54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments, 7 obsolete attachments)

6.39 KB, patch
Details | Diff | Splinter Review
4.99 KB, patch
esawin
: review+
Details | Diff | Splinter Review
4.76 KB, patch
esawin
: review+
Details | Diff | Splinter Review
5.41 KB, patch
jchen
: review+
Details | Diff | Splinter Review
1.74 KB, patch
jchen
: review+
Details | Diff | Splinter Review
This will contain things like onCanGoBack(), onCanGoForward(), onLocationChange().
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
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.
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
Flags: needinfo?(snorp)
Ah, I follow. Yes, my thinking was that we'd have an internal history observer that stores the state for canGoBack() and friends.
Flags: needinfo?(snorp)
Assignee

Comment 5

3 years ago
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.
Assignee: nobody → esawin
Attachment #8828412 - Flags: feedback?(snorp)
Attachment #8828412 - Flags: feedback?(nchen)
Attachment #8828412 - Flags: feedback?(nalexander)
Attachment #8828412 - Flags: feedback?(droeh)
Assignee

Comment 6

3 years ago
I'm not sure where we would prefer to instantiate it, there are multiple alternatives to this approach.
Attachment #8828413 - Flags: feedback?(snorp)
Attachment #8828413 - Flags: feedback?(nchen)
Attachment #8828413 - Flags: feedback?(nalexander)
Attachment #8828413 - Flags: feedback?(droeh)
Assignee

Comment 8

3 years ago
This is only for testing: it adds a simple search bar to the example app and implements basic navigation.
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.
Attachment #8828412 - Flags: feedback?(nalexander) → feedback+
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.
Attachment #8828413 - Flags: feedback?(nalexander)
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 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.
Assignee

Comment 13

3 years ago
Addressed feedback.
Attachment #8828412 - Attachment is obsolete: true
Attachment #8828412 - Flags: feedback?(snorp)
Attachment #8828412 - Flags: feedback?(nchen)
Attachment #8828412 - Flags: feedback?(droeh)
Attachment #8828854 - Flags: feedback?(snorp)
Attachment #8828854 - Flags: feedback?(nchen)
Attachment #8828854 - Flags: feedback?(droeh)
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?
Attachment #8828854 - Flags: feedback?(nchen) → feedback+
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).
Attachment #8828413 - Flags: feedback?(nchen) → feedback+
Assignee

Comment 16

2 years ago
Attachment #8828854 - Attachment is obsolete: true
Attachment #8828854 - Flags: feedback?(snorp)
Attachment #8828854 - Flags: feedback?(droeh)
Attachment #8831146 - Flags: feedback?(snorp)
Assignee

Comment 17

2 years ago
Attachment #8828413 - Attachment is obsolete: true
Attachment #8828413 - Flags: feedback?(snorp)
Attachment #8828413 - Flags: feedback?(droeh)
Attachment #8831147 - Flags: feedback?(snorp)
Assignee

Comment 18

2 years ago
Attachment #8828414 - Attachment is obsolete: true
Attachment #8831148 - Flags: feedback?(snorp)
Assignee

Updated

2 years ago
Attachment #8831146 - Flags: review?(snorp)
Attachment #8831146 - Flags: review?(nchen)
Attachment #8831146 - Flags: feedback?(snorp)
Assignee

Updated

2 years ago
Attachment #8831147 - Flags: review?(snorp)
Attachment #8831147 - Flags: review?(nchen)
Attachment #8831147 - Flags: feedback?(snorp)
Assignee

Updated

2 years ago
Attachment #8831148 - Flags: review?(snorp)
Attachment #8831148 - Flags: review?(nchen)
Attachment #8831148 - Flags: feedback?(snorp)
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)
Attachment #8831146 - Flags: review?(snorp) → review+
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.
Attachment #8831147 - Flags: review?(snorp) → review+
Attachment #8831148 - Flags: review?(snorp) → review+
Assignee

Comment 21

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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:*"?
Attachment #8831146 - Flags: review?(nchen) → review+
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.
Attachment #8831147 - Flags: review?(nchen) → review+
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.
Attachment #8831148 - Flags: review?(nchen) → feedback+
Assignee

Comment 25

2 years ago
Addressed comments.
Attachment #8831146 - Attachment is obsolete: true
Attachment #8835105 - Flags: review+
Assignee

Comment 26

2 years ago
Addressed comments.
Attachment #8831147 - Attachment is obsolete: true
Assignee

Comment 27

2 years ago
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.
Attachment #8831148 - Attachment is obsolete: true
Attachment #8835115 - Flags: review?(nchen)
Assignee

Comment 28

2 years ago
Refactor the GeckoViewContent module to use the new base class.
Attachment #8835117 - Flags: review?(nchen)
Assignee

Updated

2 years ago
Attachment #8835107 - Flags: review+
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
Attachment #8835115 - Flags: review?(nchen) → review+
Attachment #8835117 - Flags: review?(nchen) → review+

Comment 30

2 years ago
Pushed by esawin@mozilla.com:
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
Assignee

Updated

2 years ago
Duplicate of this bug: 1320586

Updated

3 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.