880121 - Add support for Browser (Chrome) application interfaces to GeckoView

Status: RESOLVED FIXED

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

Description

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

GeckoView can be used in simple or complex ways. Using GeckoView as the basis of a real browser requires the hosting application have tighter integration with GeckoView. Things like connecting a prompt system, pageload progress and events, storing history, and dealing with favicons are some examples.

WebView uses WebChromeClient to perform similar support:
http://developer.android.com/reference/android/webkit/WebChromeClient.html

Comment 1

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

Attachment: errors-while-running.log

This is a simple logcat capture of error and warnings that occur when running an Application that uses GeckoView

Comment 2

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

This error was fixed by calling HardwareUtils.init(getApplicationContext()); in the Application. We should not require Apps to call this explicitly. I think this could be handled in GeckoView itself.

W/System.err( 2966): java.lang.NullPointerException
W/System.err( 2966): 	at org.mozilla.gecko.util.HardwareUtils.isLargeTablet(HardwareUtils.java:41)
W/System.err( 2966): 	at org.mozilla.gecko.util.HardwareUtils.isTablet(HardwareUtils.java:36)
W/System.err( 2966): 	at org.mozilla.gecko.GeckoAppShell.isTablet(GeckoAppShell.java:2310)
W/System.err( 2966): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
W/System.err( 2966): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
W/System.err( 2966): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:337)
W/System.err( 2966): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

Comment 3

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

Shane filed a lot of bugs that are essentially "find a way for the Application to provide implementations" so I am making them block this bug.

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #2)
> This error was fixed by calling HardwareUtils.init(getApplicationContext());
> in the Application. We should not require Apps to call this explicitly. I
> think this could be handled in GeckoView itself.
> 
> W/System.err( 2966): java.lang.NullPointerException
> W/System.err( 2966): 	at
> org.mozilla.gecko.util.HardwareUtils.isLargeTablet(HardwareUtils.java:41)
> W/System.err( 2966): 	at
> org.mozilla.gecko.util.HardwareUtils.isTablet(HardwareUtils.java:36)
> W/System.err( 2966): 	at
> org.mozilla.gecko.GeckoAppShell.isTablet(GeckoAppShell.java:2310)
> W/System.err( 2966): 	at
> org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
> W/System.err( 2966): 	at
> org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
> W/System.err( 2966): 	at
> org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:337)
> W/System.err( 2966): 	at
> org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

Filed bug 911018 with a patch

Comment 5

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

Attachment: GeckoViewChrome WIP v0.1

Initial GeckoViewChrome class. It acts as a base class for other implementations to extend. For now, it only has onReady, which lets the app know Gecko is ready to handle requests. Trying to use a GeckoView before onReady will likely crash.

Comment 6

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

To use this patch in your own app you need to extend GeckoViewChrome:

    private class MyGeckoViewChrome extends GeckoViewChrome {
        @Override
        public void onReady(GeckoView view) {
        	Log.i("GeckoBrowser", "Gecko is ready");
        	mGeckoView.loadUrlInNewTab("http://starkravingfinkle.org");
        }
    }

and give it to your GeckoView, in onCreate for example:

    mGeckoView.setGeckoViewChrome(new MyGeckoViewChrome());

Comment 7

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

Comment on attachment 797663 [details] [diff] [review]
GeckoViewChrome 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.

At a minimum, before releasing, I think we need to add support for dealing with webapi permissions, prompts, and enabling remote debugging.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 797663 [details] [diff] [review]
GeckoViewChrome WIP v0.1

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

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Trying to use a GeckoView before will likely crash.

Since we're exposing this as an API, we should consider robustifying our code a bit to enforce these constraints. For example, we might want to keep track of Gecko's ready state, check it at the beginning of each Gecko-dependent method, and throw an IllegalStateException with useful information if these constrains are violated.

Comment 9

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

Comment on attachment 797663 [details] [diff] [review]
GeckoViewChrome WIP v0.1

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

::: mobile/android/base/GeckoViewChrome.java
@@ +10,5 @@
> +    * Tell the host application that Gecko is ready to handle requests.
> +    * @param view The GeckoView that initiated the callback.
> +    */
> +    public void onReady(GeckoView view) {}
> +}

I'd make this an interface since java doesn't have multiple inheritance. Also, just stick it in the GeckoView.java rather than a new file.

Comment 10

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9)
> Comment on attachment 797663 [details] [diff] [review]
> GeckoViewChrome WIP v0.1
> 
> Review of attachment 797663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoViewChrome.java
> @@ +10,5 @@
> > +    * Tell the host application that Gecko is ready to handle requests.
> > +    * @param view The GeckoView that initiated the callback.
> > +    */
> > +    public void onReady(GeckoView view) {}
> > +}
> 
> I'd make this an interface since java doesn't have multiple inheritance.
> Also, just stick it in the GeckoView.java rather than a new file.

Using an interface was my first thought, but I changed my mind. There are a few reason why:
* Interfaces are "all or nothing" in that the embedding dev would need to stub out all the methods, whether they need them or not.
* A base class allows Mozilla to provide some "default" behavior for methods, which the embedding dev and override if they want. This was one of the things you wanted to support IIRC.
* A base class won't immediately break an embedders code if we tweak the interface.
* The base class pattern is also used by WebView, so I think devs will feel more comfortable switching to GeckoView, even though we don't match the exact functionality.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 797663 [details] [diff] [review]
GeckoViewChrome WIP v0.1

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

::: mobile/android/base/GeckoView.java
@@ +112,4 @@
>          }
>      }
>  
> +    public void setGeckoViewChrome(GeckoViewChrome chrome) {

The more I look at these, the more I think being more specific and calling this a real listener feels better (and would handle letting you have multiple listeners if you needed them, which we already do in places... I might try to avoid the word "chrome" as well, since its a bit overloaded nowadays. Maybe something like

setGeckoListener(GeckoListener listener);
setBrowserListener(BrowserListener listener); or setPageListener(PageListener listener); ?

Comment 12

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

(In reply to Wesley Johnston (:wesj) from comment #11)

> > +    public void setGeckoViewChrome(GeckoViewChrome chrome) {
> 
> The more I look at these, the more I think being more specific and calling
> this a real listener feels better (and would handle letting you have
> multiple listeners if you needed them, which we already do in places... I
> might try to avoid the word "chrome" as well, since its a bit overloaded
> nowadays. Maybe something like
> 
> setGeckoListener(GeckoListener listener);
> setBrowserListener(BrowserListener listener); or
> setPageListener(PageListener listener); ?

setBrowserCallback and setContentCallback would work for me too. I don't like using "page" since it's overloaded too, but I could be convinced otherwise in this case.

I don't like "listener" since these are not typical Java listeners.

Comment 13

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

(In reply to Wesley Johnston (:wesj) from comment #11)

> The more I look at these, the more I think being more specific and calling
> this a real listener feels better (and would handle letting you have
> multiple listeners if you needed them, which we already do in places...

Also, we can't allow multiple "listeners". There can be only one implementer of these methods. They are not just "events", they define behavior, and the application needs to decide when to allow more than one "listener", not GeckoView.

Comment 14

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

(In reply to Brian Nicholson (:bnicholson) from comment #8)

> Since we're exposing this as an API, we should consider robustifying our
> code a bit to enforce these constraints. For example, we might want to keep
> track of Gecko's ready state, check it at the beginning of each
> Gecko-dependent method, and throw an IllegalStateException with useful
> information if these constrains are violated.

That might be a good idea. I'd like to wait for some real dev feedback before needing to assert before every method.

Comment 15

[Richard Newman [:rnewman]]

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

> Also, we can't allow multiple "listeners". There can be only one implementer
> of these methods. They are not just "events", they define behavior, and the
> application needs to decide when to allow more than one "listener", not
> GeckoView.

The term you're looking for is "Delegate".

Comment 16

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> (In reply to Brian Nicholson (:bnicholson) from comment #8)
> 
> > Since we're exposing this as an API, we should consider robustifying our
> > code a bit to enforce these constraints. For example, we might want to keep
> > track of Gecko's ready state, check it at the beginning of each
> > Gecko-dependent method, and throw an IllegalStateException with useful
> > information if these constrains are violated.
> 
> That might be a good idea. I'd like to wait for some real dev feedback
> before needing to assert before every method.

Assuming that we can trust Gecko's lifecycle, and tie it to Java, it seems the smartest thing to do is to have two different objects: a GV wrapper, and a Gecko-ey object.

Don't even allow access to an instance which exposes Gecko-necessary methods until Gecko is ready -- pass that instance as an argument to the delegate's onReady method.

Of course, if Gecko can die at any time we need a way to invalidate that handle, and we might be back to square one... but perhaps this cuts the knot a little.

Comment 17

[Richard Newman [:rnewman]]

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

> Using an interface was my first thought, but I changed my mind.

Why not do both? Define interfaces and expose sensible common-case base classes. We do this successfully throughout services code, and it's a great way to illustrate the dotted-line composition of your app:

  public class MyGeckoController
    extends GeckoPageLoadDelegateBase
    implements GeckoViewLifecycleDelegate {

    /*
     * Lifecycle methods: handle a GeckoView becoming available.
     */
    @Override
    public void onReady(GeckoView view) {
      ...
    }

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 797663 [details] [diff] [review]
GeckoViewChrome WIP v0.1

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

f- 'cos strictly speaking neither of the two changes are in quite the right direction :P

::: mobile/android/base/GeckoView.java
@@ +33,5 @@
>      implements GeckoEventListener, ContextGetter {
>  
>      private static final String LOGTAG = "GeckoView";
>  
> +    private GeckoViewChrome mChrome;

This should at least be volatile or final (set in the constructor), but there might be other concurrency issues hiding in this design. We'll see when GV gets more mature.

Also, to continue the earlier theme: this should be an explicit delegate, and ideally it shouldn't be a member at all.

That is, if you *only* call onReady() on this thing, and thus you don't need to tie the lifecycles together, then just do this:

  public void initialize(final GeckoReadyDelegate delegate) {
    ...
      ...  // Some bullshit with callbacks and runnables
        delegate.onReady();    // maybe onReady(someHandle);

Comment 19

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

Attachment: GeckoViewChrome WIP v0.2

This version adds more methods to the Chrome API. The first patch didn't do a good enough job showing that this API was not for initialization only.

Hopefully this version show more of what we expect the API to handle.

I also want help on thinking about how to convert the current Prompt based system into Prompt, Permissions and Debugger APIs. The code in browser.js (and friends) use Promtp.jsm or Doorhangers to convert the context to a "prompting" context by the time Java sees the messages. Therefore, we can't convert the message to a Permission-specfic API or a Debugger-specific API.

We'll need to change these messages (or code in general) to be more specific.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 815006 [details] [diff] [review]
GeckoViewChrome WIP v0.2

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

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

This still makes sense to me as a delegate, or even a small family of delegates.

  public interface GeckoViewPermissionDelegate {
    public void onPermissionRequest(...);
  }

  public interface GeckoViewDialogDelegate {
    public boolean onConfirm(GeckoView view, GeckoView.Browser browser, String message);
    public String onPrompt(GeckoView view, GeckoView.Browser browser, String message, String defaultValue);
  }

  ...

The idea of splitting these out as interfaces is:

* It forces you (and end-developers) to think about partitioning logic into named categories, without preventing you from clumping things into a single class if you need to. (Yay interfaces!)

* It's conceptually cleaner, which simplifies conveyance of concepts and code reuse. Think about how you might want an abstract class which implements most of the 'bogus' interfaces -- permissions and dialogs -- but leaves you to implement only the one you care about (page load events, say).

* It allows you to ignore the bits that aren't relevant without having a big base class that implements every method (which leads to confusion about which methods you *do* need to override together, as well as introducing testing and code evolution concerns).

Generally speaking, I think if we end up with a giant Fennec-shaped class that clients must subclass in order to work with a view, bristling with interdependent methods and default implementations, then we've failed from an architecture perspective. Let's try to bake in clarity from the ground up.

Comment 21

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

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

> This still makes sense to me as a delegate, or even a small family of
> delegates.

<snip>

> * It forces you (and end-developers) to think about partitioning logic into
> named categories, without preventing you from clumping things into a single
> class if you need to. (Yay interfaces!)

Agreed, and because of this point, I am not against moving in that direction as we get the code more mature. I do not fear breaking this shit before we "release" GeckoView 1.0

> * It allows you to ignore the bits that aren't relevant without having a big
> base class that implements every method (which leads to confusion about
> which methods you *do* need to override together, as well as introducing
> testing and code evolution concerns).

We need a simple way to add default behavior into this system in a way that doesn't make me sick. The current base class approach, while a bit broad means:
* Devs only override what we want, not everything.
* Anything not overriden has a default implementation

> Generally speaking, I think if we end up with a giant Fennec-shaped class
> that clients must subclass in order to work with a view, bristling with
> interdependent methods and default implementations, then we've failed from
> an architecture perspective. Let's try to bake in clarity from the ground up.

Interdepent methods are a potential problem no matter how we implement it. We must be vigilant. Default implements are a good thing, IMO. Better than default implementation the embeddor needs to remember to add themselves.

Comment 22

[Richard Newman [:rnewman]]

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

> Interdepent methods are a potential problem no matter how we implement it.
> We must be vigilant. Default implements are a good thing, IMO. Better than
> default implementation the embeddor needs to remember to add themselves.

I'd rather achieve that via default delegate initializers (or null checks) in GeckoView -- you get the modularity/separation of concerns *and* default behavior, and you also get 'free' base classes that you can override.

One could even go so far as to have a GeckoViewFactory.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 815006 [details] [diff] [review]
GeckoViewChrome WIP v0.2

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

I think we need to revamp these prompt/permission interfaces a bit.

::: mobile/android/base/GeckoViewChrome.java
@@ +28,5 @@
> +    * Tell the host application to display a confirmation dialog.
> +    * @param view The GeckoView that initiated the callback.
> +    * @param browser The Browser that is loading the content.
> +    * @param message The string to display in the dialog.
> +    * @return The true or false response. Defaults to false.

Hmm... WebChromeClient has one of these for alert dialogs as well. But I wonder if we need to also handle checkboxes/multiple buttons.

@@ +40,5 @@
> +    * @param view The GeckoView that initiated the callback.
> +    * @param browser The Browser that is loading the content.
> +    * @param message The string to display in the dialog.
> +    * @param defaultValue The string to use as default input.
> +    * @return The string response. Use null if not handled. Defaults to null.

Android dialogs aren't blocking, so I don't think we want to force callers to spin the thread and return a result here. We probably need some sort of callback. This should probably just return true or false to enable/disable the default prompt implementation.

@@ +42,5 @@
> +    * @param message The string to display in the dialog.
> +    * @param defaultValue The string to use as default input.
> +    * @return The string response. Use null if not handled. Defaults to null.
> +    */
> +    public String onPrompt(GeckoView view, GeckoView.Browser browser, String message, String defaultValue) {

What's the purpose of passing a GeckoView here?

@@ +51,5 @@
> +    * Tell the host application that a web site is requesting a permission.
> +    * @param view The GeckoView that initiated the callback.
> +    * @param browser The Browser that is requesting the permission.
> +    * @param origin The site requesting the permission.
> +    * @param callback Used to accept or deny the request without blocking.

This should probably return true or false as well for "handling" or "ignoring"?

@@ +53,5 @@
> +    * @param browser The Browser that is requesting the permission.
> +    * @param origin The site requesting the permission.
> +    * @param callback Used to accept or deny the request without blocking.
> +    */
> +    public void onPermissionRequest(GeckoView view, GeckoView.Browser browser, String origin, PermissionType type, PermissionCallback callback) {

Some of our permissions also require something more complex (camera comes to mind). I wonder if we need to pass a more generic Permission object that we can have different types of and attach metadata to. i.e.

public class Permission {
  public PermissionType type;
  public String description?;
  public string origin;
  pubic Browser browser;

  public void accept():
  public void deny();
}

@@ +59,5 @@
> +
> +    /**
> +    * Tell the host application to display a remote debugging request dialog.
> +    * @param view The GeckoView that initiated the callback.
> +    * @return Accept or deny the request to start a debugging session. Defaults to false.

A callback here would be better as well (in case they want to show UI).

Comment 24

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

Attachment: geckoview-chrome-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 25

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

Comment on attachment 821617 [details] [diff] [review]
geckoview-chrome-a v1

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

Same comments as the content listener, r+ with nits

::: mobile/android/base/GeckoView.java
@@ +35,5 @@
>      implements GeckoEventListener, ContextGetter {
>  
>      private static final String LOGTAG = "GeckoView";
>  
> +    private GeckoViewChrome mChrome;

mChromeListener

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

Move this into GeckoView as an inner interface GeckoView.ChromeListener

Comment 26

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

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

> > +    private GeckoViewChrome mChrome;
> 
> mChromeListener

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

> > +public class GeckoViewChrome {
> 
> Move this into GeckoView as an inner interface GeckoView.ChromeListener

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

Comment 27

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

Attachment: geckoview-chrome-a v2

Carrying r+

This has Brad's feedback addressed, except ChromeListener is ChromeDelegate.

Comment 28

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

https://hg.mozilla.org/integration/fx-team/rev/43c4eda97a57

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43c4eda97a57