1322590 - [geckoview] Rework ContentDelegate

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

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

Remove onReceivedFavicon() and onPageShow().
Add onPageProgress() maybe?
Rename onReceivedTitle() -> onTitleChanged()
Add onSecurityChanged()

Hook everything up.

Comment 1

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

Attachment: geckoview-content.patch

This hooks up ContentDelegate (now ContentListener) and updates the GeckoView example app to log title changes. Mostly based off of Eugen's patch for bug 1322592, updated to address the feedback given there.

Note: progress-related functions (onPageStart, onPageStop, etc) will be handled separately in ProgressDelegate.

Comment 2

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

Comment on attachment 8830493 [details] [diff] [review]
geckoview-content.patch

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

::: mobile/android/chrome/content/GeckoViewContent.jsm
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Rename to "GeckoViewContent.js"

@@ +5,5 @@
> +
> +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Not used?

@@ +9,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "ViewContent");
> +
> +var global = this;

Not used?

@@ +14,5 @@
> +
> +// This is copied from desktop's tab-content.js. See bug 1153485 about sharing this code somehow.
> +var DOMTitleChangedListener = {
> +  init: function() {
> +    addEventListener("DOMContentLoaded", this, false);

Not used?

@@ +19,5 @@
> +    addEventListener("DOMTitleChanged", this, false);
> +  },
> +
> +  receiveMessage: function(message) {
> +    dump("receiveMessage " + message.name);

Only log if we're debugging.

::: mobile/android/chrome/content/geckoview.js
@@ +8,5 @@
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "Log",
>   "resource://gre/modules/AndroidLog.jsm", "AndroidLog");

Remove this if you change dump below.

@@ +25,5 @@
> +           .AndroidLog.d.bind(null, "View");
> +
> +// Define globals.
> +[
> +  ["gBrowser", "content"],

I think you should pass in the browser object to GeckoViewContent, etc, instead of letting it access window.gBrowser. And why do we need a getter/setter here?

@@ +44,5 @@
> +  });
> +});
> +
> +// GeckoView module imports.
> +Cu.import("resource://gre/modules/GeckoViewContent.jsm");

Make it lazy loading

@@ +49,5 @@
>  
>  function startup() {
> +  dump("zerdatime " + Date.now() + " - geckoview chrome startup finished.");
> +
> +  var content = new GeckoViewContent(window, WindowEventDispatcher);

I think you want content to be a global variable.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +44,5 @@
>  
>      private final EventDispatcher eventDispatcher = new EventDispatcher();
>  
>      private ChromeDelegate mChromeDelegate;
> +    private ContentListener mContentListener;

Make package-private

@@ +111,5 @@
>              };
>      }
>  
> +    private class Listener implements BundleEventListener {
> +        /*package */ void registerListeners() {

Space before package

@@ +112,5 @@
>      }
>  
> +    private class Listener implements BundleEventListener {
> +        /*package */ void registerListeners() {
> +            eventDispatcher.registerUiThreadListener(this,

Make eventDispatcher package-private, or call getEventDispatcher()

@@ +117,5 @@
> +                "GeckoView:DOMTitleChanged",
> +                null);
> +        }
> +
> +        /*package */ void unregisterListeners() {

Don't need this if you don't call it.

@@ +126,5 @@
> +
> +        @Override
> +        public void handleMessage(final String event, final GeckoBundle message,
> +                                  final EventCallback callback) {
> +            Log.d(LOGTAG, "handleMessage: event = " + event);

Only log if debugging

@@ +129,5 @@
> +                                  final EventCallback callback) {
> +            Log.d(LOGTAG, "handleMessage: event = " + event);
> +
> +            if ("GeckoView:DOMTitleChanged".equals(event)) {
> +                Log.i(LOGTAG, "Handling GeckoView:DOMTitleChanged message in GeckoView.java");

Same

@@ +141,3 @@
>      protected Window window;
>      private boolean stateSaved;
> +    private Listener listener = new Listener();

final

@@ +378,2 @@
>      */
> +    public void setContentListener(ContentListener content) {

Add getContentListener

@@ +494,4 @@
>          /**
>          * A page title was discovered in the content or updated after the content
>          * loaded.
>          * @param view The GeckoView that initiated the callback.

Update this.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewContent.java
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
> +public class GeckoViewContent implements GeckoView.ContentListener {

We should just remove this file

::: mobile/android/modules/GeckoViewContent.jsm
@@ +22,5 @@
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {})
> +           .AndroidLog.d.bind(null, "ViewContent");
> +
> +var window;
> +var WindowEventDispatcher;

Keep these as members of GeckoViewContent

@@ +44,5 @@
> +    return window.gBrowser.messageManager;
> +  }
> +
> +  handleEvent(event) {
> +    dump("handleEvent: event.type=" + event.type);

Log only when debugging

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

If you're adding this separately because of GeckoView, you should document that in a comment.

Comment 3

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

Attachment: GeckoView ContentListener v1.1

I think this addresses everything you brought up. I also changed onTitleChanged back to taking the GeckoView per a discussion you and Eugen had on irc.

Comment 4

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

Comment on attachment 8832223 [details] [diff] [review]
GeckoView ContentListener v1.1

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

LGTM with nits.

::: mobile/android/chrome/content/GeckoViewContent.js
@@ +6,5 @@
> +var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "ViewContent");
> +
> +var debug = false;

uppercase DEBUG

::: mobile/android/chrome/content/geckoview.js
@@ +18,5 @@
> +XPCOMUtils.defineLazyGetter(this, "WindowEventDispatcher",
> +  () => EventDispatcher.for(window));
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {})
> +           .AndroidLog.d.bind(null, "View");

Note that this will be a little slower to load than `function dump`

::: mobile/android/modules/GeckoViewContent.jsm
@@ +16,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "EventDispatcher",
> +  "resource://gre/modules/Messaging.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "GlobalEventDispatcher",
> + () => EventDispatcher.instance);

Remove "XPCOMUtils", "Services", "EventDispatcher" and "GlobalEventDispatcher" since you're not using them.

@@ +21,5 @@
> +
> +var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {})
> +           .AndroidLog.d.bind(null, "ViewContent");
> +
> +var debug = false;

uppercase

Comment 5

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

Also ThreadUtils is not used in GeckoView.java

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1b19dc25d6
Add and hook up ContentListener for GeckoView. r=jchen

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae1b19dc25d6