1372681 - Modularize GeckoView handlers

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Eugen Sawin [:esawin]]

Currently, we register for all events in the GeckoView*.jsm modules to serve potential listeners and delegates in GeckoView.java.
We should only register for events relevant to active listeners/delegates.

At the same time, GeckoView.java is growing rapidly and I would like to move the "handlers" out of it. Each handler will correspond to at least one Java listener and to one jsm module. The modular design should help with maintenance and allow us to enable event registration without much code duplication.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1372681-1.0-Ad-GeckoView-module-registration-mec.patch

Add register and unregister methods to GeckoViewModule which are called automatically through <ModuleName>:Register and <ModuleName>:Unregister events respectively.

Allow passing additional data for module-specific handling, e.g., when multiple Java listeners are communicating with one jsm module (GeckoViewContent.jsm) for more fine-grained registration control.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1372681-2.0-Add-GeckoViewHandler-base-class-for-.patch

Base class for GeckoView handlers. It will take care of registration mechanics based on the event types and extra data provided by the handler implementation.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1372681-5.0-Add-GeckoViewNavigationHandler.patch

GeckoViewNavigation handler implementation. This is how the registration mechanics could work out.
There is still some redundancy in the handlers which may be reduced.

I'm holding off the other patches for now, until we agree on whether that's a path we want to go at all.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1372681-3.0-Add-GeckoViewContentHandler.patch

Add the GeckoView content handler for the content delegate and the scroll listener.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1372681-1.1-Add-GeckoView-module-registration-me.patch

Updated patch to simplified module registration handling.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1372681-2.1-Add-GeckoViewHandler-base-class-for-.patch

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1372681-3.1-Add-GeckoViewContentHandler.-r-snorp.patch

Moved scroll handling into its own content module.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1372681-4.1-Add-GeckoViewProgressHandler.-r-jche.patch

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1372681-5.1-Add-GeckoViewNavigationHandler.-r-sn.patch

Minor fixes (whitespace).

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0006-Bug-1372681-6.1-Add-GeckoViewScrollHandler.-r-jchen.patch

Comment 11

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

Comment on attachment 8882011 [details] [diff] [review]
0001-Bug-1372681-1.1-Add-GeckoView-module-registration-me.patch

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

::: mobile/android/modules/geckoview/GeckoViewModule.jsm
@@ +26,5 @@
> +      () => this.onSettingsUpdate(), "GeckoView:UpdateSettings"
> +    );
> +
> +    this.eventDispatcher.registerListener(
> +      () => this.register(), this.moduleName + ":Register"

I think I prefer "GeckoView:RegisterModule" with the module name in the event data.

@@ +30,5 @@
> +      () => this.register(), this.moduleName + ":Register"
> +    );
> +
> +    this.eventDispatcher.registerListener(
> +      () => this.unregister(), this.moduleName + ":Unregister"

Same

Comment 12

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

Comment on attachment 8882013 [details] [diff] [review]
0002-Bug-1372681-2.1-Add-GeckoViewHandler-base-class-for-.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewHandler.java
@@ +27,5 @@
> +
> +        register();
> +    }
> +
> +    protected String[] getEvents() {

Make this abstract.

@@ +32,5 @@
> +        return null;
> +    }
> +
> +    protected GeckoView getGeckoView() {
> +        return mGeckoView;

Just make mGeckoView protected.

@@ +40,5 @@
> +        final EventDispatcher eventDispatcher = mGeckoView.getEventDispatcher();
> +        eventDispatcher.dispatch(mModuleName + ":Register", null);
> +
> +        for (final String event: getEvents()) {
> +            eventDispatcher.registerUiThreadListener(this, event);

eventDispatcher.registerUiThreadListener(this, getEvents());

@@ +49,5 @@
> +        final EventDispatcher eventDispatcher = mGeckoView.getEventDispatcher();
> +        eventDispatcher.dispatch(mModuleName + ":Unregister", null);
> +
> +        for (final String event: getEvents()) {
> +            eventDispatcher.unregisterUiThreadListener(this, event);

eventDispatcher.unregisterUiThreadListener(this, getEvents());

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1372681-1.2-Add-GeckoView-module-registration-me.patch

Addressed comments.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1372681-2.2-Add-GeckoViewHandler-utility-class-f.patch

Replaced the handler base class with a generic utility class to be used for the handler implementations.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1372681-3.2-Add-GeckoView-handlers.-r-snorp-jche.patch

Handlers based on the generic utility class.
Also merged all handler patches for convenience.

Comment 16

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

Comment on attachment 8882371 [details] [diff] [review]
0002-Bug-1372681-2.2-Add-GeckoViewHandler-utility-class-f.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewHandler.java
@@ +18,5 @@
> +
> +    private static final String LOGTAG = "GeckoViewHandler";
> +    private static final boolean DEBUG = false;
> +
> +    /* package */ Listener mListener;

private, and add getListener().

@@ +19,5 @@
> +    private static final String LOGTAG = "GeckoViewHandler";
> +    private static final boolean DEBUG = false;
> +
> +    /* package */ Listener mListener;
> +    /* package */ final GeckoView mGeckoView;

Don't need `mGeckoView`. Just pass `GeckoView` into `setListener`

@@ +30,5 @@
> +        mGeckoView = view;
> +        mEvents = events;
> +    }
> +
> +    public void register(final Listener listener) {

I think you can combine `register` and `unregister` into `setListener()`, which accepts a listener or null.

@@ +70,5 @@
> +            handleMessage(mListener, event, message, callback);
> +        }
> +    }
> +
> +    public abstract void handleMessage(final Listener listener,

protected?

Comment 17

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

Comment on attachment 8882373 [details] [diff] [review]
0003-Bug-1372681-3.2-Add-GeckoView-handlers.-r-snorp-jche.patch

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

Make sure you test each module to make sure they still work.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +92,4 @@
>      private final EventDispatcher mEventDispatcher =
>          new EventDispatcher(mNativeQueue);
>  
> +    /* package */ GeckoViewHandler<ContentListener> mContentHandler =

`private final` here and below.

@@ +107,5 @@
> +                                      final GeckoBundle message,
> +                                      final EventCallback callback) {
> +
> +                if ("GeckoView:DOMTitleChanged".equals(event)) {
> +                    mListener.onTitleChange(mGeckoView,

Use `listener` instead of `mListener` here and below. And use `GeckoView.this` here and below.

@@ +118,5 @@
> +
> +            }
> +        };
> +
> +    /* package */ GeckoViewHandler<NavigationListener> mNavigationHandler =

private

@@ +140,5 @@
> +
> +            }
> +        };
> +
> +    /* package */ GeckoViewHandler<ProgressListener> mProgressHandler =

private

@@ +151,5 @@
> +            }
> +        ) {
> +            @Override
> +            public void handleMessage(final ProgressListener listener,
> +                                       final String event,

Alignment

@@ +168,5 @@
> +                }
> +            }
> +        };
> +
> +    /* package */ GeckoViewHandler<ScrollListener> mScrollHandler =

Private

@@ +679,4 @@
>      * @return The current content callback handler.
>      */
>      public ContentListener getContentListener() {
> +        return mContentHandler.mListener;

Use `getListener`

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1372681-2.3-Add-GeckoViewHandler-utility-class-f.patch

Addressed comments.

Comment 19

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1372681-3.3-Add-GeckoView-handlers.-r-snorp-jche.patch

Addressed comments.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19c2fe5baeb
[1.2] Add GeckoView module registration mechanics. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1813dbfae6
[2.3] Add GeckoViewHandler utility class for event handler registration. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/086a33d9d70f
[3.3] Add GeckoView handlers. r=snorp,jchen

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d19c2fe5baeb
https://hg.mozilla.org/mozilla-central/rev/7e1813dbfae6
https://hg.mozilla.org/mozilla-central/rev/086a33d9d70f