548387 - audit Fennec tabs pseudo-API usage in tabs sync engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

http://mxr.mozilla.org/labs-central/source/weave/source/modules/engines/tabs.js

Comment 1

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

A fews things jump out at me, including the need for Fennec to have a first class window/tab system:

* Would something like FUEL (or FUEL itself) with a defined wrapper system encapsulating the window, tab and document be a good model?
* What are you planning with SessionStore? Fennec doesn't current;y ship it.
* Would a window global object, like Browser or gBrowser, be a good entry point? or would you rather see getTabs()? - which is more like the getNotificationBox() entry point used by XPCOM JS components.
* What does tab.lastUsed represent? Fennec has a tab.lastSelected

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #1)
> * What are you planning with SessionStore? Fennec doesn't current;y ship it.

I'm writing a fake SessionStore (in Weave) that implements the APIs that I'm using in sessionstore on Firefox (bug 548939). Right now it's still pretty similar to what's in the link in comment 0, but more like session store - e.g. I'm getting the sessionHistory object off the browser instead of just grabbing the url, in prep for a richer tab syncing experience.

The fake SessionStore (like the fake private browsing) just implements a couple things. setTabData - which is just used for lastUsed now, but potentially a guid in the future, just sets data in an object on the tab object. getBrowserState - which will go through all the tabs and build an object like sessionstore returns (though much more minimal).

> * Would a window global object, like Browser or gBrowser, be a good entry
> point? or would you rather see getTabs()? - which is more like the
> getNotificationBox() entry point used by XPCOM JS components.

Personally, I'd love to see Firefox & Fennec have the same API, but that's obviously not going to happen. If Fennec wants to lead by example with a simple & sane API, all the better.

I guess if Fennec isn't going to have some sort of SessionStore service, then just a way to get the data out is fine. Browser works. gBrowser works. Browser._tabs is a bit silly, but that's ok, so long as it works.

> * What does tab.lastUsed represent? Fennec has a tab.lastSelected

Right now it's either opened or selected, and is used to order the tabs when showed on a different client. I'll look at lastSelected.

What we're looking for here (I think) is more of a "are we doing anything dumb?" "is there an easier way? (and if not, should there be, which you did bring up)".

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Browser._tabs is a bit silly, but that's ok, so long as it works.

We should probably implement .tabs in both Firefox and Fennec.

Also, can bug 525708 be ported to Fennec?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

updated location of code: http://mxr.mozilla.org/labs-central/source/weave/source/modules/util.js#792 - it's all under the fake sessionstore service.

Comment 5

[Mike Connor [:mconnor]]

(In reply to comment #1)
> A fews things jump out at me, including the need for Fennec to have a first
> class window/tab system:
> 
> * Would something like FUEL (or FUEL itself) with a defined wrapper system
> encapsulating the window, tab and document be a good model

I would love to see Fennec and Firefox ship a common API.  The Firefox pseudo-API is a little overgrown, but I think a concerted effort would pay dividends for add-ons.

I think, in general, the actions coming out of this bug are:

* Implement a sane API for Fennec (and hopefully Firefox)
* File a dependent bug to make Weave code use this API.

Mark, other than "we don't have a good API for most of this" is there anything we should do in the current implementation?  I can file the followup bugs and close this if so.

Comment 6

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

I think the current Weave handling of Fennec is reasonable. As Fennec extends it's session store, we'll check back to see what methods Weave needs.

The Browser API in Fennec is incrementally getting better, but I think Weave is doing fine with what we have.

Comment 7

[Mike Connor [:mconnor]]

Filed bug 561270 on Fennec, and bug 561271 on Weave, dependent on that bug.