880119 - Improve the API for GeckoView

Status: RESOLVED FIXED

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

Description

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

GeckoView itself should have a basic APIs for loading, viewing and manipulating HTML, either locally or from the Web.

Comment 1

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

Attachment: GeckoView Browser API v0.1

This patch adds the concept of a GeckoView.Browser to the widget. Since GeckoView can really support multiple browsers (or tabs), we need a way to allow developers to access and manipulate the browsers.

In Gecko, we have the tabbrowser.xml widget which acts as a wrapper for <browser>s and <tab>s, but GeckoView only support <browser>s, so this API is limited to GeckoView.Browser objects.

The API adds:

class GeckoView {
  Browser add(String url)
  void remove(Browser browser)

  void setSelected(Browser browser)
  Browser getSelected()

  ArrayList<Browser> getBrowsers()
}

class Browser {
  void loadUrl(String url)
  void reload()
  void stop()

  boolean canGoBack()
  void goBack()
  boolean canGoForward()
  void goForward()
}

Comment 2

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

Comment on attachment 798284 [details] [diff] [review]
GeckoView Browser API v0.1

Looking for some feedback here. This patch makes GeckoView act a little like tabbrowser.xml in that it manages a set of Browsers. Those Browsers are matched to Tabs under the covers by the ID, which is used in Tab & Browser (in Java) and Tab (in JS). I specifically wanted this to remain very lightweight. If an embedder wants to add more state, they can build their own system to do it.

Feel free to ignore the request, but I wanted to get more eyes on it.

Comment 3

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

Comment on attachment 798284 [details] [diff] [review]
GeckoView Browser API v0.1

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

This looks fine to me. I suspect that the majority of consumers of GeckoView will only be showing one tab, so my primary concern is to not overcomplicate the API for that use case and I think this accomplishes that.

::: mobile/android/base/GeckoView.java
@@ +97,3 @@
>      }
>  
> +    public void remove(Browser browser) {

perhaps add/remove should be open/close

@@ +109,5 @@
> +            Tabs.getInstance().selectTab(tab.getId());
> +        }
> +    }
> +
> +    public Browser getSelected() {

getCurrentBrowser()? getSelectedBrowser()? same for set, add and remove.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 798284 [details] [diff] [review]
GeckoView Browser API v0.1

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

I'm a bit confused about Browser/Tab model. We already have a Tab class that does the things in Browser (plus a lot more). We have the Tabs class that acts as a Tab manager. This patch creates a Browser class that wraps Tab, and acts as a kind of Browser manager itself by returning a transient Browser/set of Browsers every time getX() is called. Creating a new on-the-fly Browser every single time we need one here and in bug 880123 is the part I really don't like.

I think it would make more sense to have a Tab own a Browser (like we do in XUL) instead of the other way around. Browser could be a simple object to act on page navigation like you have now, but it could actually implement those methods itself rather than wrapping the more complex Tab. That would make our API classes more well-defined: a Browser holds the basic methods for manipulating a page, and a Tab holds any extra metadata specific to that tab.

There wouldn't be all of these temporary Browser classes being added everywhere; there would be exactly one Browser for one Tab. Then your getSelected() would just be 'Tabs.getInstance().getSelectedTab().getBrowser()'.

::: mobile/android/base/GeckoView.java
@@ +156,5 @@
>      public static GeckoAppShell.GeckoInterface getGeckoInterface() {
>          return GeckoAppShell.getGeckoInterface();
>      }
> +
> +    public class Browser {

Since you're creating new Browsers everywhere, you'll want to override equals() to compare IDs. Otherwise, clients doing getSelected() and then iterating over getBrowsers() testing selected equals index will always return false.

@@ +159,5 @@
> +
> +    public class Browser {
> +        private final int mId;
> +        public Browser(int Id) {
> +            mId = Id;

Rather than holding the ID, could you just pass the Tab object itself to Browser? Then you could act on the tab directly in all of the wrapper methods below without doing the tab retrieval work every time, and without needing all of the null checks.

Comment 5

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

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

> > +    public void remove(Browser browser) {
> 
> perhaps add/remove should be open/close

Yeah, worth considering

> > +    public Browser getSelected() {
> 
> getCurrentBrowser()? getSelectedBrowser()? same for set, add and remove.

I felt using "Browser" in all of those names was too redundant. We won't be adding or removing anything else.

Comment 6

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

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 798284 [details] [diff] [review]
> GeckoView Browser API v0.1
> 
> Review of attachment 798284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit confused about Browser/Tab model. We already have a Tab class that
> does the things in Browser (plus a lot more). We have the Tabs class that
> acts as a Tab manager. This patch creates a Browser class that wraps Tab,
> and acts as a kind of Browser manager itself by returning a transient
> Browser/set of Browsers every time getX() is called. Creating a new
> on-the-fly Browser every single time we need one here and in bug 880123 is
> the part I really don't like.

The big reason why: We do not want GeckoView to be tied to Tabs/Tab. That is a Fennec-ism, not a widget. My end goal to eventually flip the relationship and have Tabs/Tab use GeckoView/Browser.

We do not want embedders to use our Tabs/Tab code. During this transition, GeckoView/Browser will use Tabs/Tab, but only until we convert the code in Fennec to use more of GeckoView. Does this makes sense?


> I think it would make more sense to have a Tab own a Browser (like we do in
> XUL) instead of the other way around. Browser could be a simple object to
> act on page navigation like you have now, but it could actually implement
> those methods itself rather than wrapping the more complex Tab. That would
> make our API classes more well-defined: a Browser holds the basic methods
> for manipulating a page, and a Tab holds any extra metadata specific to that
> tab.

I hope my above comments make it clear why we do not want GeckoView to depend on Tabs in the future.

> There wouldn't be all of these temporary Browser classes being added
> everywhere; there would be exactly one Browser for one Tab. Then your
> getSelected() would just be
> 'Tabs.getInstance().getSelectedTab().getBrowser()'.

Temporary Browser objects are better for thread safety. If Fennec wants to store Tabs long term, we can, but we should not force everyone else to do it too.

> ::: mobile/android/base/GeckoView.java
> @@ +156,5 @@
> >      public static GeckoAppShell.GeckoInterface getGeckoInterface() {
> >          return GeckoAppShell.getGeckoInterface();
> >      }
> > +
> > +    public class Browser {
> 
> Since you're creating new Browsers everywhere, you'll want to override
> equals() to compare IDs. Otherwise, clients doing getSelected() and then
> iterating over getBrowsers() testing selected equals index will always
> return false.

Good catch

> > +    public class Browser {
> > +        private final int mId;
> > +        public Browser(int Id) {
> > +            mId = Id;
> 
> Rather than holding the ID, could you just pass the Tab object itself to
> Browser? Then you could act on the tab directly in all of the wrapper
> methods below without doing the tab retrieval work every time, and without
> needing all of the null checks.

Holding a Tab is the Devil's way. We need to be stateless.

Comment 7

[Richard Newman [:rnewman]]

GeckoView is a confusing name because it's really a multiplexer for a collection of loaded pages. One would naturally think, coming from Android, that one instance of GeckoView = one loaded page.

It's really a kind of browser. And we want to track the pages inside the browser from Java.

In Fennec we have Tabs, which manage Tab instances, which are a bundle of Java-side metadata and keys into Gecko.

My terminology (which doesn't quite match up to the Gecko terminology, because legacy) would be:

  GeckoView: an Android view.
    has-a: GeckoBrowser: a collection of pages. Can exist independent of the view.
      has-many: GeckoPage: a handle to a Gecko-side loaded page. Create one of these by asking a GeckoBrowser for a new page.


  GeckoApp
    has-a GeckoView
    has-a Tabs
      has-many Tab
        has-a GeckoPage

Correct my misconceptions?

Comment 8

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

(In reply to Richard Newman [:rnewman] from comment #7)
> GeckoView is a confusing name because it's really a multiplexer for a
> collection of loaded pages. One would naturally think, coming from Android,
> that one instance of GeckoView = one loaded page.

Thinking strictly of Android Views, yes, however WebView breaks this metaphor. "page" is already overloaded too. A "page" is commonly thought of as a "webpage" and a "browser" holds a collection of "pages" as session history. You can move back and forward through the session history.

A WebView is more comparable to a <browser> in XUL, which is a GeckoView.Browser in this patch. Yes, GeckoView is a collection, where WebView is a single.

Another constraint is that we are wrapping what Gecko gives us. Currently, Gecko opens browser.xul (a <window> in XUL terms) and this <window> has a collection of <browser> elements in it.

> In Fennec we have Tabs, which manage Tab instances, which are a bundle of
> Java-side metadata and keys into Gecko.
> 
> My terminology (which doesn't quite match up to the Gecko terminology,
> because legacy) would be:
> 
>   GeckoView: an Android view.
>     has-a: GeckoBrowser: a collection of pages. Can exist independent of the
> view.
>       has-many: GeckoPage: a handle to a Gecko-side loaded page. Create one
> of these by asking a GeckoBrowser for a new page.

I am confused, probably by the use of GeckoPage. Also by GeckoView containing a single GeckoBrowser. Also by my brain being too tied to the current Java/XUL mapping.

>   GeckoApp
>     has-a GeckoView
>     has-a Tabs
>       has-many Tab
>         has-a GeckoPage

This kinda looks like Firefox. Again, due to existing naming, let's rename GeckoApp to EmbeddingApp. And rename GeckoPage to GeckoBrowser (called Browser in my patch), because of Page being too overloaded. With those changes, this organization is how I would want Firefox to be structured once we get GeckoView to be fully implemented as a widget.

Other EmbeddingApps might forgo the need for Tabs and Tab, not wanting that level of UI or structure. Those apps might just want to display a Twitter stream. 

 TwitterApp
   has-a GeckoView
     has-one GeckoBrowser (but could temporarily open others to handle Persona logins)

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 798284 [details] [diff] [review]
GeckoView Browser API v0.1

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

::: mobile/android/base/GeckoView.java
@@ +87,5 @@
>              GeckoThread.createAndStart();
>          }
>      }
>  
> +    public Browser add(String Url) {

lowercase 'url'

@@ +114,5 @@
> +        Tab tab = Tabs.getInstance().getSelectedTab();
> +        if (tab != null) {
> +            return new Browser(tab.getId());
> +        }
> +        return null;

I wonder if its worth throwing in these cases instead of assuming callers will all null check the result. 

We should
1.) Javadoc this stuff. It will be exposed to outside callers. and
2.) Document the null result if you leave it.

@@ +156,5 @@
>      public static GeckoAppShell.GeckoInterface getGeckoInterface() {
>          return GeckoAppShell.getGeckoInterface();
>      }
> +
> +    public class Browser {

I'm tempted to say we just make Tab.java implement a Browser interface tbh. I vaguely get that frontends will have to wrap this class in their own owner/list? eventually, but that class will likely end up looking more like Browser.java does now and Browser.java will end up looking like Tab.java, so this seems closer to the end goal...

Still parsing and thinking about the earlier comments though...

@@ +166,5 @@
> +        public int getId() {
> +            return mId;
> +        }
> +
> +        public void loadUrl(String Url) {

lowercase?

Comment 10

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

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

> > +    public Browser add(String Url) {
> 
> lowercase 'url'

Yep

> > +        Tab tab = Tabs.getInstance().getSelectedTab();
> > +        if (tab != null) {
> > +            return new Browser(tab.getId());
> > +        }
> > +        return null;
> 
> I wonder if its worth throwing in these cases instead of assuming callers
> will all null check the result. 
> 
> We should
> 1.) Javadoc this stuff. It will be exposed to outside callers. and
> 2.) Document the null result if you leave it.

I might leave the null for now, but I am open to throwing instead. Let's get some real world feedback and decide.

> > +    public class Browser {
> 
> I'm tempted to say we just make Tab.java implement a Browser interface tbh.
> I vaguely get that frontends will have to wrap this class in their own
> owner/list? eventually, but that class will likely end up looking more like
> Browser.java does now and Browser.java will end up looking like Tab.java, so
> this seems closer to the end goal...

Yes. The end goal would have Tab holding, not implementing, a Browser object. I agree that IceFoxApp would probably have some sort of Tab object, used to maintain it's tab list. But we should not assume that it would look exactly like the GeckoApp Tab object. It could be more heavy, if that's possible, or less so. It could hold very different information too. I want to make sure that GeckoView only supports the minimal amount of code, nothing more. 

It's easy to want to add convenience helpers to the code, but we need to try to avoid that.

> > +        public void loadUrl(String Url) {
> 
> lowercase?

Yep

Comment 11

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

Attachment: GeckoView Browser API v0.2

Brian - I made your suggested changes. I saw that when overriding equals() you should also override hashCode(), so I did.

I also made Wes' lowercase fixes. I still need to add Javadocs to the API.

Comment 12

[Ben Bucksch (:BenB)]

> Looking for some feedback here. This patch makes GeckoView act a little like tabbrowser.xml in that it manages a set of Browsers.

I like the
Foo add(String url)
idea. This allows to load a URL without having to create a callback.
I'd rename it to addBrowser(url), though.

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> The big reason why: We do not want GeckoView to be tied to Tabs/Tab. That is
> a Fennec-ism, not a widget. My end goal to eventually flip the relationship
> and have Tabs/Tab use GeckoView/Browser.

I think this was partly what threw me off in my previous comment -- it sounds like the end game is almost the exact opposite of what you're implementing here.

If we don't want them to be tied together, why wrap Tabs/Tab in the first place? If we don't want Tab to be a dependency of GeckoView before we release GeckoView 1.0, it seems simpler to implement this correctly from the ground up rather than implementing GeckoView as a Tab/Tabs wrapper now, only to have to untangle the dependencies later.

> We do not want embedders to use our Tabs/Tab code. During this transition,
> GeckoView/Browser will use Tabs/Tab, but only until we convert the code in
> Fennec to use more of GeckoView. Does this makes sense?

I'm still not clear on the role of Tab/Tabs once Fennec is using this new GeckoView API. If Richard's relationship model from comment 7 is correct, we'll still have a Tabs object, and it will still contain a Tab set, independent from GeckoView. This is the other thing that threw me off in the last comment -- we'll basically have two separate managers for the same set of pages, where GeckoView holds a set of Browsers and Tabs holds a set of Tab objects.

If this is the plan, it sounds like we're going to have a pretty messy, half-baked model with GeckoView thrown in as an afterthought instead of it being at the core of Fennec as I had hoped. When loading a page, for example, I would expect GeckoView clients to go through Browser#loadUrl. But in Fennec, we go through Tabs#loadUrl with a a bunch of obscure custom flags. We can't use Browser#loadUrl unless we provide those same flags in the Browser API, and that's obviously not an option if we want to keep GeckoView simple. So we'll need to keep this separate channel, independent of Browser/GeckoView, to load a URL.

Can't we drop Tab/Tabs entirely and use Browser/GeckoView instead? Obviously, we'll have lots of Fennec-specific code that we don't want moved into GeckoView/Browser, so this seems like the perfect place to use the factory pattern. GeckoView can generate/return the simple Browser class by default, but apps that want extended functionality can instead have GeckoView generate/return a custom subclass of Browser (which is where we could dump all of the Fennec-specific code in Tab). So in Fennec, GeckoView would hold full-on Tab-equivalent Browser objects, making Tabs (and Tab) unnecessary.

> Temporary Browser objects are better for thread safety. If Fennec wants to
> store Tabs long term, we can, but we should not force everyone else to do it
> too.

I don't think this helps with thread safety. If Tab isn't thread-safe, we're screwed -- whether we hold a reference to it directly or get it by ID, we're still acting on the same Tab object in the end. Did you mean you're concerned about memory leaks, where we're still potentially holding on to a Tab object after it has been removed from Tabs?

I think this is a strong reason to implement these GeckoView methods directly now instead of as wrappers as I mentioned before. If GeckoView won't be using Tab in the future, then what will these methods even look like? If GeckoView is no longer a wrapper for Tabs, then it sounds like GeckoView will be holding an internal list of Browsers itself, so these transient ID-based Browsers may not make sense.

Comment 14

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

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > The big reason why: We do not want GeckoView to be tied to Tabs/Tab. That is
> > a Fennec-ism, not a widget. My end goal to eventually flip the relationship
> > and have Tabs/Tab use GeckoView/Browser.
> 
> I think this was partly what threw me off in my previous comment -- it
> sounds like the end game is almost the exact opposite of what you're
> implementing here.

Yes, it's pretty much the opposite implementation, but it will be the same interface. Just because we implement using the code we have right now doesn't mean we can't change it later. In fact, that should be a goal for any code we ever write. EVAR.

> If we don't want them to be tied together, why wrap Tabs/Tab in the first
> place? If we don't want Tab to be a dependency of GeckoView before we
> release GeckoView 1.0, it seems simpler to implement this correctly from the
> ground up rather than implementing GeckoView as a Tab/Tabs wrapper now, only
> to have to untangle the dependencies later.

Because we want to ship ASAP, not in a few years. The implementation dependencies we hide behind a solid interface are of little consequence. We are free to evolve the code as time permits.

It doesn't matter if Tab is an implementation detail of GeckoView v1.0, because developers using GeckoView will never know about Tab or care about Tab - as long as we do our job right.

> > We do not want embedders to use our Tabs/Tab code. During this transition,
> > GeckoView/Browser will use Tabs/Tab, but only until we convert the code in
> > Fennec to use more of GeckoView. Does this makes sense?
> 
> I'm still not clear on the role of Tab/Tabs once Fennec is using this new
> GeckoView API. If Richard's relationship model from comment 7 is correct,
> we'll still have a Tabs object, and it will still contain a Tab set,
> independent from GeckoView. This is the other thing that threw me off in the
> last comment -- we'll basically have two separate managers for the same set
> of pages, where GeckoView holds a set of Browsers and Tabs holds a set of
> Tab objects.

Yes, this is correct. GeckoView is a collection of Browsers. Plain and simple. If an embedding application wants to turn these Browsers into Tabs (or some other UI metaphor) they are free to do that, but GeckoView will take no part in that.

Right now the Fennec Tab object leans on the tabID which connects it to browser.js, but in the future, when using GeckoView, Tab will lean on the Browser object, which is a very think wrapper around the tabID.

Any embedding application that does not want to deal with Tabs is free to just worry about Browsers.

> If this is the plan, it sounds like we're going to have a pretty messy,
> half-baked model with GeckoView thrown in as an afterthought instead of it
> being at the core of Fennec as I had hoped. When loading a page, for
> example, I would expect GeckoView clients to go through Browser#loadUrl. But
> in Fennec, we go through Tabs#loadUrl with a a bunch of obscure custom
> flags. We can't use Browser#loadUrl unless we provide those same flags in
> the Browser API, and that's obviously not an option if we want to keep
> GeckoView simple. So we'll need to keep this separate channel, independent
> of Browser/GeckoView, to load a URL.

I think we'll need some of those flags, just like XBL <browser> has a loadURLWithFlags method. Some of those flags might be easy for GeckoView to ignore, but others won't because GeckoView will have to implement the details. An example is private browsing.

> Can't we drop Tab/Tabs entirely and use Browser/GeckoView instead?
> Obviously, we'll have lots of Fennec-specific code that we don't want moved
> into GeckoView/Browser, so this seems like the perfect place to use the
> factory pattern. GeckoView can generate/return the simple Browser class by
> default, but apps that want extended functionality can instead have
> GeckoView generate/return a custom subclass of Browser (which is where we
> could dump all of the Fennec-specific code in Tab). So in Fennec, GeckoView
> would hold full-on Tab-equivalent Browser objects, making Tabs (and Tab)
> unnecessary.

I'd be open to seeing how this factory idea would be implemented, but I have my doubts we'd be able to make it work as easy as you think.

> > Temporary Browser objects are better for thread safety. If Fennec wants to
> > store Tabs long term, we can, but we should not force everyone else to do it
> > too.
> 
> I don't think this helps with thread safety. If Tab isn't thread-safe, we're
> screwed -- whether we hold a reference to it directly or get it by ID, we're
> still acting on the same Tab object in the end. Did you mean you're
> concerned about memory leaks, where we're still potentially holding on to a
> Tab object after it has been removed from Tabs?
> 
> I think this is a strong reason to implement these GeckoView methods
> directly now instead of as wrappers as I mentioned before. If GeckoView
> won't be using Tab in the future, then what will these methods even look
> like? If GeckoView is no longer a wrapper for Tabs, then it sounds like
> GeckoView will be holding an internal list of Browsers itself, so these
> transient ID-based Browsers may not make sense.

So far, I have never thought GeckoView would ever hold a list of objects. Instead I have been thinking of holding the list of integer tabIDs. It keeps things simple. browser.js holds the actual objects, and I'd really like to not add any state on top of what browser.js already does. I suppose we could make GeckoView hold a list of Browser objects if we wanted to stop making temporary Browser objects. I still feel it's the embedding application's job to manage any state and only if it cares about state. There is no reason for GeckoView to manage state if an application never cares about state.

Maybe you should look at how the stock Android browser creates a browser application out of the WebView object. It might be helpful to see how the "widget" and "app" parts are separated.

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> Right now the Fennec Tab object leans on the tabID which connects it to
> browser.js, but in the future, when using GeckoView, Tab will lean on the
> Browser object, which is a very think wrapper around the tabID.
...
> So far, I have never thought GeckoView would ever hold a list of objects.
> Instead I have been thinking of holding the list of integer tabIDs. It keeps
> things simple. browser.js holds the actual objects, and I'd really like to
> not add any state on top of what browser.js already does. I suppose we could
> make GeckoView hold a list of Browser objects if we wanted to stop making
> temporary Browser objects. I still feel it's the embedding application's job
> to manage any state and only if it cares about state. There is no reason for
> GeckoView to manage state if an application never cares about state.

We still have to maintain some state in some situations, right? For instance, consider canGoBack()/canGoForward(). Right now, these methods lean on Tab for the implementation, and Tab needs state to implement these methods (namely, mHistoryIndex and mHistorySize). If Browser won't be using Tab in the future, where will this state come from?

We could end up using JNI to pull the state directly from browser.js, but we probably won't want to do that for the same reason we aren't doing it now. So if we don't wrap Tab, we'll need to hold this state in Browser directly, meaning we'll also need to keep that state updated (so Browser will probably need its own handleSessionHistoryMessage equivalent).

I'm fine with doing this if that's what you're planning on -- just wanted to make sure we're on the same page since I didn't see this reflected in the API.

Comment 16

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

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

> We still have to maintain some state in some situations, right? For
> instance, consider canGoBack()/canGoForward(). Right now, these methods lean
> on Tab for the implementation, and Tab needs state to implement these
> methods (namely, mHistoryIndex and mHistorySize). If Browser won't be using
> Tab in the future, where will this state come from?

Good eye! Yes, that's my current issue with Browser. I had been trying to decide which of the 3 ideas I had for handling it would be optimal:
1. Hold the state in a Browser
2. Make the calls be async, and query the browser.js code
3. Pass the state along with a different event, like onPageShow

I really didn't want #1 since I am in an anti-state frame of mind. I was close to trying #2, but it felt too weird, and I remembered back when Fennec supported e10s (do you remember those days?). Since the "state" was kept in the child process, we had to keep sending the back/forward information along with some JSON messages. This is exactly #3 and I think I'll propose doing that. GeckoView don't care about canGoBack/canGoForward, but we can send that state along in the onPageShow callback. The application can track it in it's own version of a Tab class.

> I'm fine with doing this if that's what you're planning on -- just wanted to
> make sure we're on the same page since I didn't see this reflected in the
> API.

You're right. We need to make sure the API we end up with will work OK with the current Tab-based implementation as well as the future raw-browser.js based implementation. I think moving the back/forward state to onPageShow will make that possible.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 815023 [details] [diff] [review]
GeckoView Browser API v0.2

All my lingering questions have been answered now, so this looks fine to me.

Comment 18

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

Attachment: geckoview-browser-api 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 19

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

Comment on attachment 821615 [details] [diff] [review]
geckoview-browser-api v1

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

::: mobile/android/base/GeckoView.java
@@ +30,5 @@
>  import android.view.SurfaceView;
>  
> +import java.util.ArrayList;
> +
> + public class GeckoView extends LayerView

ws

@@ +90,5 @@
> +    /**
> +    * Add a Browser to the GeckoView container.
> +    * @param url The URL resource to load into the new Browser.
> +    */
> +    public Browser add(String url) {

I don't like just a naked add. addBrowser? createBrowser?

@@ +102,5 @@
> +    /**
> +    * Remove a Browser from the GeckoView container.
> +    * @param browser The Browser to remove.
> +    */
> +    public void remove(Browser browser) {

same here, removeBrowser

@@ +113,5 @@
> +    /**
> +    * Set the active/visible Browser.
> +    * @param browser The Browser to make selected.
> +    */
> +    public void setSelected(Browser browser) {

selectBrowser

@@ +176,5 @@
> +    * element in the Gecko system.
> +    */
> +    public class Browser {
> +        private final int mId;
> +        public Browser(int Id) {

this constructor should probably not be public

@@ +185,5 @@
> +        * Get the ID of the Browser. This is the same ID used by Gecko for it's underlying
> +        * browser element.
> +        * @return The integer ID of the Browser.
> +        */
> +        public int getId() {

why do we want to expose the id of the browser? I feel like this should be treated as internal state.

@@ +194,5 @@
> +        * Load a URL resource into the Browser.
> +        * @param url The URL string.
> +        */
> +        public void loadUrl(String url) {
> +            // TODO: We need to load the URL into the tab with our Id, not the selected tab

seems like this is a TODO that needs to be resolved before landing

@@ +239,5 @@
> +        */
> +        public void goBack() {
> +            Tab tab = Tabs.getInstance().getTab(mId);
> +            if (tab != null) {
> +                tab.doBack();

what if we can't go back? perhaps this should return a bool

@@ +260,5 @@
> +
> +        /**
> +        * Move forward in the session history, if that's possible.
> +        */
> +        public void goForward() {

same, return a bool.

or, perhaps the new url/NULL

Comment 20

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

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

> > +    public Browser add(String url) {
> 
> I don't like just a naked add. addBrowser? createBrowser?

> > +    public void remove(Browser browser) {
> 
> same here, removeBrowser

> > +    public void setSelected(Browser browser) {
> 
> selectBrowser

Naming OCD/Bikeshed: When the only thing GeckoView contains is Browsers, I find it very redundant to add "Browser" suffixes to all the methods. Not to mention the "Browser" data type helps out too.

> > +        public Browser(int Id) {
> 
> this constructor should probably not be public

Good point. I'll try to make it private.

> @@ +185,5 @@
> > +        * Get the ID of the Browser. This is the same ID used by Gecko for it's underlying
> > +        * browser element.
> > +        * @return The integer ID of the Browser.
> > +        */
> > +        public int getId() {
> 
> why do we want to expose the id of the browser? I feel like this should be
> treated as internal state.

I want to allow embedding apps to use something to track/hash a Browser. If you think using Browser is good enough, we can try removing this public method. I'll remove it, since we can always add it back if needed.

> > +        public void loadUrl(String url) {
> > +            // TODO: We need to load the URL into the tab with our Id, not the selected tab
> 
> seems like this is a TODO that needs to be resolved before landing

As long as we don't need to boil the ocean to do it. I'll take a quick look.

> > +        public void goBack() {
> > +            Tab tab = Tabs.getInstance().getTab(mId);
> > +            if (tab != null) {
> > +                tab.doBack();
> 
> what if we can't go back? perhaps this should return a bool

It's an async call. We can't return something. We could pass a callback, but I really don't think it's worth it.

If a goBack or goForward call fails (because you ignored checking the state), then the rest of the callbacks won't fire. I mean, you won't get a onPageStarted/onPageEnded/onPageShow callback. As long as you depend on those and not the result of goBack/goForward, you should be fine. And you should depend on the callbacks since pages can start loading for many different ways, even web content JS or use link clicks.

> > +        public void goForward() {
> 
> same, return a bool.
> 
> or, perhaps the new url/NULL

See above

Comment 21

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

(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #19)
> 
> > > +    public Browser add(String url) {
> > 
> > I don't like just a naked add. addBrowser? createBrowser?
> 
> > > +    public void remove(Browser browser) {
> > 
> > same here, removeBrowser
> 
> > > +    public void setSelected(Browser browser) {
> > 
> > selectBrowser
> 
> Naming OCD/Bikeshed: When the only thing GeckoView contains is Browsers, I
> find it very redundant to add "Browser" suffixes to all the methods. Not to
> mention the "Browser" data type helps out too.

I tried to turn off my OCD fo a bit and do some research into how other Android UI "containers" APIs are structured. Looking at TabHost and ViewGroup, it seems like they favor your approach. I'll make the new patch use the following API:

Browser addBrowser(...)
void removeBrowser(Browser)
void setCurrentBrowser(Browser)
Browser getCurrentBrowser()

Comment 22

[Ben Bucksch (:BenB)]

> my OCD fo a bit

"for"

(just kidding!)

Comment 23

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

Attachment: geckoview-browser-api v2

This should have your feedback addressed except for returning something from goBack and goForward, since they are async. We can re-work those APIs if needed, but the "content" delegate onPageShow acts as a signal right now.

Comment 24

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

Comment on attachment 827589 [details] [diff] [review]
geckoview-browser-api v2

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

::: mobile/android/base/GeckoView.java
@@ +141,5 @@
> +    /**
> +    * Get the list of current Browsers in the GeckoView container.
> +    * @return An ArrayList of Browser objects.
> +    */
> +    public ArrayList<Browser> getBrowsers() {

I'd rather be more generic here, either return a Collection<Browser> or Browser[] (just call browsers.toArray())

@@ +199,5 @@
> +        * Load a URL resource into the Browser.
> +        * @param url The URL string.
> +        */
> +        public void loadUrl(String url) {
> +            JSONObject args = new JSONObject();    

ws

@@ +208,5 @@
> +                args.put("tabID", mId);
> +            } catch (Exception e) {
> +                Log.w(LOGTAG, "Error building JSON arguments for loadUrl.", e);
> +            }
> +    

extra blank line and ws

Comment 25

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

Landed with unmodifiable List<T> since it's indexable

https://hg.mozilla.org/integration/fx-team/rev/34f82e024599

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34f82e024599

Comment 27

[Ali Kasimoglu]

Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards