880123 - Add support for content callback interfaces to GeckoView

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

On it's own, GeckoView won't know how to support many of the HTML5 APIs. It will depend on the host application to provide support for things like Geo-location, vibration, battery status and network offline/online control.

GeckoView can provide both abstract interfaces and simple implementations for such services. This will keep GeckoView lightweight, but optionally support more features. By providing some basic implementations, we can make it easy for most devs to add support for services, where needed.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Morphing this bug to "content callback" since I think the chrome interface in bug 880121 should handle providing support for HTML APIs.

This bug is now about adding callbacks to track browser/page activity.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: GeckoViewContent WIP v0.1

This patch adds a callback interface to GeckoView which allows host applications to be notified when certain events happen in the browser/page. The interface curently looks like:

class GeckoViewContent {
  void onPageStart(GeckoView, Browser, String url)
  void onPageStop(GeckoView, Browser, boolean success)
  void onPageShow(GeckoView, Browser)

  void onReceivedTitle(GeckoView, Browser, String title)
}

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 798286 [details] [diff] [review]
GeckoViewContent WIP v0.1

I expect to add a few more methods in the interface, but it's close enough for feedback. Technically this is not an "interface", just a base class. WebView does the same thing. It allows embedders to only override the parts they care about.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 798286 [details] [diff] [review]
GeckoViewContent WIP v0.1

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

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

again, I'd prefer an interface to a class

@@ +11,5 @@
> +    * @param view The GeckoView that initiated the callback.
> +    * @param browser The Browser that is loading the content.
> +    * @param url The resource being loaded.
> +    */
> +    public void onPageStart(GeckoView view, GeckoView.Browser browser, String Url) {}

I'm wondering if we wouldn't rather have a listener pattern here (addPageStartListener()). Reason being, if there are no listeners we could potentially fast path around these callbacks

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> Comment on attachment 798286 [details] [diff] [review]
> GeckoViewContent WIP v0.1
> 
> Review of attachment 798286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoViewContent.java
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> > +
> > +public class GeckoViewContent {
> 
> again, I'd prefer an interface to a class
> 
> @@ +11,5 @@
> > +    * @param view The GeckoView that initiated the callback.
> > +    * @param browser The Browser that is loading the content.
> > +    * @param url The resource being loaded.
> > +    */
> > +    public void onPageStart(GeckoView view, GeckoView.Browser browser, String Url) {}
> 
> I'm wondering if we wouldn't rather have a listener pattern here
> (addPageStartListener()). Reason being, if there are no listeners we could
> potentially fast path around these callbacks

See https://bugzilla.mozilla.org/show_bug.cgi?id=880121#c10 for some feedback on why I went with base classes instead of interfaces. The "fast path" idea is worth considering. It's a little in conflict with "provide default behavior" though, so we need to balance it. Also, I'm not a fan of making lots of listeners. Thinking about it, are listeners used in situations where two-way control might be involved, like giving onPageStart the ability to cancel the load?

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 798286 [details] [diff] [review]
GeckoViewContent WIP v0.1

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

::: mobile/android/base/GeckoView.java
@@ +80,5 @@
> +        GeckoAppShell.registerEventListener("Content:LoadError", this);
> +        GeckoAppShell.registerEventListener("Content:PageShow", this);
> +        GeckoAppShell.registerEventListener("DOMTitleChanged", this);
> +        GeckoAppShell.registerEventListener("Link:Favicon", this);
> +        GeckoAppShell.registerEventListener("Link:Feed", this);

It worries me that GeckoView will become another dumping ground for messages like GeckoApp...

@@ +151,5 @@
> +                        requestRender();
> +            
> +                        if (mChrome != null) {
> +                            mChrome.onReady(GeckoView.this);
> +                        }

I don't love it as a perfect solution, but you should pull these different if/else-if statements out into their own private methods.

@@ +164,5 @@
> +                            } else if ((state & GeckoAppShell.WPL_STATE_STOP) != 0) {
> +                                if (mContent != null) {
> +                                    int id = message.getInt("tabID");
> +                                    mContent.onPageStop(GeckoView.this, new Browser(id), message.getBoolean("success"));
> +                                }

With brad's listener stuff this could be something prettier like:

notifyContent(STOP, id, success);

Much like what Tabs.java has now (and AFAICT, this is basically just splitting the non-frontend pieces of Tabs.java into GeckoView). In fact, again I kinda wonder if this should just be an interface that Tabs.java would implement. But we'll need to pull it apart into a front/backend part sometime.

::: mobile/android/base/GeckoViewContent.java
@@ +36,5 @@
> +    * @param view The GeckoView that initiated the callback.
> +    * @param browser The Browser that is showing the content.
> +    * @param title The title sent from the content.
> +    */
> +    public void onReceivedTitle(GeckoView view, GeckoView.Browser browser, String title) {}

Not a huge fan of this name. I'd rather just onTitleChanged(GeckoView view, GeckoView.Browser browser, String title). Do we support dynamic title changes?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 798286 [details] [diff] [review]
> GeckoViewContent WIP v0.1
> 
> Review of attachment 798286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoView.java
> @@ +80,5 @@
> > +        GeckoAppShell.registerEventListener("Content:LoadError", this);
> > +        GeckoAppShell.registerEventListener("Content:PageShow", this);
> > +        GeckoAppShell.registerEventListener("DOMTitleChanged", this);
> > +        GeckoAppShell.registerEventListener("Link:Favicon", this);
> > +        GeckoAppShell.registerEventListener("Link:Feed", this);
> 
> It worries me that GeckoView will become another dumping ground for messages
> like GeckoApp...

It will be for a while, but we should have much fewer messages. Most of our messages are for Fennec specific things. Only when we flip the dependencies, and have Fennec code using GeckoView code, will we start to see that change. I am in no hurry to make that flip. We should not block GeckoView on major refactors in Fennec.

> > +                        if (mChrome != null) {
> > +                            mChrome.onReady(GeckoView.this);
> > +                        }
> 
> I don't love it as a perfect solution, but you should pull these different
> if/else-if statements out into their own private methods.

Agreed. I'll move them.

> > +                            } else if ((state & GeckoAppShell.WPL_STATE_STOP) != 0) {
> > +                                if (mContent != null) {
> > +                                    int id = message.getInt("tabID");
> > +                                    mContent.onPageStop(GeckoView.this, new Browser(id), message.getBoolean("success"));
> > +                                }
> 
> With brad's listener stuff this could be something prettier like:
> 
> notifyContent(STOP, id, success);

Sounds like notifyContent would have a big switch in it thought. Also, I do not want to use listeners. The current patch uses the content base class to handle default actions too. The embeddor can override if they want. I am also not a fan of listeners when we'd potentially create so many. Lastly, we want to allow for return values from the methods to change what the default behavior does.

> Much like what Tabs.java has now (and AFAICT, this is basically just
> splitting the non-frontend pieces of Tabs.java into GeckoView). In fact,
> again I kinda wonder if this should just be an interface that Tabs.java
> would implement. But we'll need to pull it apart into a front/backend part
> sometime.

We should do this in the future, when Fennec starts to really use GeckoView. I am not blocking GeckoView on that though.

> ::: mobile/android/base/GeckoViewContent.java

> > +    public void onReceivedTitle(GeckoView view, GeckoView.Browser browser, String title) {}
> 
> Not a huge fan of this name. I'd rather just onTitleChanged(GeckoView view,
> GeckoView.Browser browser, String title). Do we support dynamic title
> changes?

I borrowed the WebView naming convention, but I can change it. Yes, we support dynamic title changes.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Morphing this bug to "content callback" since I think the chrome interface
> in bug 880121 should handle providing support for HTML APIs.

Not sure I agree with that (assuming I'm guessing what you mean). Commenting here, 'cos that bug doesn't seem to be addressing quite what you implied in Comment 0.

Prefer composition over giant bloated inheritance: why should my GV-using class either (a) need to implement an interface full of gamepad/location/printer/contacts/etc. APIs, or (b) subclass a base class full of hundreds of methods that will crowd my docs and IDE? Further, must I always know at compile-time which implementations I want to use?

I'd suggest clustering HTML5 APIs into modules, describing each with an interface and associating each with a set of typical Android APIs, and providing base or null implementations accordingly.

  public interface HTML5Geolocation {
    public void getCurrentPosition(GeoDelegate delegate);
    public void watchPosition(Position p, GeoDelegate delegate);
  }

  /**
   * Requires Android ACCESS_COARSE_LOCATION permission.
   * See class XXX for the equivalent with fine position.
   */
  public class HTML5GeolocationBase implements HTML5Geolocation {
    private final LocationManager locationManager;
    public HTML5GeolocationBase() {
      this.locationManager = (LocationManager) this.getSystemService(Context.LOCATION_SERVICE);
    }
    ...
  }

Then I can control which ones I need, which implementations I provide, etc.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #8)

<snip lots of text>

I basically agree. Look at the "blocks" list of bug 880121 for all the individual bugs where we'll build the individual impls, like your HTML5Geolocation.

The "chrome" callback in bug 880121 is no longer trying to implement all the HTML APIs, only stuff more relevant to the host application itself.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: GeckoViewContent WIP v0.2

Wes - The changes made to this patch were all from you. Take a look to see if it turned out as you hoped.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-content-a v1

This is the basic patch, tweaked with some of the feedback comments. We expect to get more feedback from Android developers and we'll adjust the API more when we do.

This is experimental, but ready for external developers.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 821618 [details] [diff] [review]
geckoview-content-a v1

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

r=blassey with the name bike-shedding

::: mobile/android/base/GeckoView.java
@@ +37,5 @@
>  
>      private static final String LOGTAG = "GeckoView";
>  
>      private GeckoViewChrome mChrome;
> +    private GeckoViewContent mContent;

mContentListener? This isn't really representing the content.

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

I'd like to move this to GeckoView.java and have it be an inner interface (so GeckoView.ContentListener)

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #12)

> > +    private GeckoViewContent mContent;
> 
> mContentListener? This isn't really representing the content.

Since "Listener" is a different kind of thing, I can rename to mContentCallback

> > +public class GeckoViewContent {
> 
> I'd like to move this to GeckoView.java and have it be an inner interface
> (so GeckoView.ContentListener)

It's not an interface though. The current patch  has no implementation, but other patches are likely to have a base implementation. And we don't want to require embedding to implement every method.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #13)

> Since "Listener" is a different kind of thing, I can rename to
> mContentCallback

Delegate delegate delegate delegate

http://iruntheinternet.com/lulzdump/images/gifs/the-other-guys-mark-wahlberg-smashing-computer-monitor-1377038606B.gif?id=254
 

> It's not an interface though. The current patch  has no implementation, but
> other patches are likely to have a base implementation. And we don't want to
> require embedding to implement every method.

"Require" doesn't imply that we don't provide an interface which the base class implements. Binding to an interface is an important discipline to use for embeddable APIs, I feel.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #14)
> (In reply to Mark Finkle (:mfinkle) from comment #13)
> 
> > Since "Listener" is a different kind of thing, I can rename to
> > mContentCallback
> 
> Delegate delegate delegate delegate

Does this mean you want mXxxDelegate? Or a completely different pattern?

> > It's not an interface though. The current patch  has no implementation, but
> > other patches are likely to have a base implementation. And we don't want to
> > require embedding to implement every method.
> 
> "Require" doesn't imply that we don't provide an interface which the base
> class implements. Binding to an interface is an important discipline to use
> for embeddable APIs, I feel.

What's the difference between a binding to a class that optionally allows all methods to be overridden or an interface that required them all to be overridden?

I like binding to an API, which both the class and interface would provide. The class allows us to extend the API in the future (which will happen) without breaking the existing apps.

Are you looking for the purity of an interface? (no impl, fixed API)

Comment 16

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #15)

> Does this mean you want mXxxDelegate? Or a completely different pattern?

The former. If we support multiple delegates (and I suggest we should, each being an interface that handles a particular kind of delegated authority), it should be 

  private LoadDelegate mLoadDelegate;
  private ScriptDelegate mScriptDelegate;

etc.

The names should be descriptive:

  public interface LoadDelegate {
    public boolean onBeforeLoad(URI page, ...);
    public void onLoad(Page page, ...);
    ...
  }

Sidenote: I would be inclined to abandon the mAncientStyle here, if you're feeling adventurous. Its redolent of 1990s text editors, and we don't do it elsewhere in our modern codebase (services and modern JS). The only reason I haven't more forcefully advocated for its removal is because that's what Android's legacy code does, too.


> > "Require" doesn't imply that we don't provide an interface which the base
> > class implements. Binding to an interface is an important discipline to use
> > for embeddable APIs, I feel.
> 
> What's the difference between a binding to a class that optionally allows
> all methods to be overridden or an interface that required them all to be
> overridden?

What do I do when I have to 'implement' two base classes? Java doesn't allow for multiple inheritance. So when I want my app class to be a JimBaseClass and a GeckoPageLoadBaseClass (after all, we're embedding in an app that wants to get something done), I'm screwed.

For example:

https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/GlobalSession.java#L61

Furthermore, one can consider an interface as an "all-abstract" base class -- which is the perfect thing when I *ought* to implement most or all of the methods, and/or when the methods I don't care about can usefully be empty (onFoo? empty method body.)

As a app developer, I always want the ability to fall back to just implementing the interface. And as an architect, I want us to make sure we don't fall back on the crutch of expecting things to work the way our default base class implementations work, without documenting cross-method dependencies.


> I like binding to an API, which both the class and interface would provide.
> The class allows us to extend the API in the future (which will happen)
> without breaking the existing apps.

API changes, arguably, *should* break existing apps. If we ship a new version that does something different, and you're directly implementing an interface, you're *supposed* to break.

I agree with you that we should ship base classes that do the obvious thing; those consumers wouldn't be broken, but they also can't expect to have full control over what's happening.

> Are you looking for the purity of an interface? (no impl, fixed API)

I'm hoping to get the benefits of an interface (specified API, flexibility for consumers) without necessarily imposing its costs (by providing base classes and factories where they make sense).

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #16)
> (In reply to Mark Finkle (:mfinkle) from comment #15)
> 
> > Does this mean you want mXxxDelegate? Or a completely different pattern?
> 
> The former. If we support multiple delegates (and I suggest we should, each
> being an interface that handles a particular kind of delegated authority),
> it should be 
> 
>   private LoadDelegate mLoadDelegate;
>   private ScriptDelegate mScriptDelegate;
> 
> etc.
> 
> The names should be descriptive:
> 
>   public interface LoadDelegate {
>     public boolean onBeforeLoad(URI page, ...);
>     public void onLoad(Page page, ...);
>     ...
>   }
> 
> Sidenote: I would be inclined to abandon the mAncientStyle here, if you're
> feeling adventurous. Its redolent of 1990s text editors, and we don't do it
> elsewhere in our modern codebase (services and modern JS). The only reason I
> haven't more forcefully advocated for its removal is because that's what
> Android's legacy code does, too.
> 
> 
> > > "Require" doesn't imply that we don't provide an interface which the base
> > > class implements. Binding to an interface is an important discipline to use
> > > for embeddable APIs, I feel.
> > 
> > What's the difference between a binding to a class that optionally allows
> > all methods to be overridden or an interface that required them all to be
> > overridden?
> 
> What do I do when I have to 'implement' two base classes? Java doesn't allow
> for multiple inheritance. So when I want my app class to be a JimBaseClass
> and a GeckoPageLoadBaseClass (after all, we're embedding in an app that
> wants to get something done), I'm screwed.

Ah, multiple inheritance! I like it as much as the next C++ developer, and yay to Java for at least supporting multiple interface inheritance. Sure, interfaces would be the way to go for this kind of support. Honesty, prior art around WebView shows aggregation being the preferred way to extend.

> https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/
> org/mozilla/gecko/sync/GlobalSession.java#L61

There are zealots out there that would consider that blasphemy. Thankfully, they don't review our code.

> Furthermore, one can consider an interface as an "all-abstract" base class
> -- which is the perfect thing when I *ought* to implement most or all of the
> methods, and/or when the methods I don't care about can usefully be empty
> (onFoo? empty method body.)
> 
> As a app developer, I always want the ability to fall back to just
> implementing the interface. And as an architect, I want us to make sure we
> don't fall back on the crutch of expecting things to work the way our
> default base class implementations work, without documenting cross-method
> dependencies.

Tell you what. I'll make you and Brad happy-ish. I'll add an interface to GeckoView, GeckoView.ContentDelegate, and implement it with GeckoViewContent. Be forewarned, I will keep changing the interface willy-nilly until we ship an official 1.0 though.

> > I like binding to an API, which both the class and interface would provide.
> > The class allows us to extend the API in the future (which will happen)
> > without breaking the existing apps.
> 
> API changes, arguably, *should* break existing apps. If we ship a new
> version that does something different, and you're directly implementing an
> interface, you're *supposed* to break.

Very draconian. I bet you like XML too! I agree only when an existing part of the API is changed. When adding completely new bits, I prefer to not break.

Comment 18

[Richard Newman [:rnewman]]

When adding completely new bits – a location change delegate, a syncing interface, whatever – it'll be a new delegate altogether, so existing clients won't break. I only advocate breakage when you're changing a contract, not writing a new one.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #18)
> When adding completely new bits – a location change delegate, a syncing
> interface, whatever – it'll be a new delegate altogether, so existing
> clients won't break. I only advocate breakage when you're changing a
> contract, not writing a new one.

I personally detest LoadDelegate2. I come from MSCOM, where interface based programming is the norm, and I have grown to hate IMyCoolThing2 which conceptually adds a slightly different method to IMyCoolThing.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> (In reply to Richard Newman [:rnewman] from comment #18)
> > When adding completely new bits – a location change delegate, a syncing
> > interface, whatever – it'll be a new delegate altogether, so existing
> > clients won't break. I only advocate breakage when you're changing a
> > contract, not writing a new one.
> 
> I personally detest LoadDelegate2.

Me too!

I don't want to support multiple historical layers of delegate interfaces.

When I say "completely new", I mean coverage for a feature for which no delegate exists -- extension of the surface area, not change.

I want to evolve IMyCoolThing if the interface changes. (Users can extend our base class if the meaning is close enough, and otherwise the caller will be broken -- and if the interface is changing, that's how it should be!)

I want to *not* break users of IMyCoolThing by *adding* IMyBoringThing when we add a new feature, rather than having a single delegate interface.

If you don't care about IMyBoringThing, let the default implementation be assigned in the view factory.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-content-a v2

Carrying r+

This has Brad's and Richard's feedback addressed, I think. We ended up using ContentDelegate as the interface name. We still have the simple implementation too.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/0039e0f220bf

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0039e0f220bf