Now that Firefox has FUEL and Thunderbird gets STEEL, I think it would be a good idea to do something like that for SeaMonkey as well, perhaps as a combo of the two. I propose to name it SMILE - SeaMonkey Interface Library for Extensions :)

Part of the STEEL work is to move toolkit parts of FUEL into Toolkit. This would help the SeaMonkey cause too. See bug 407963 for more details.

Given that some extension developers such as myself are beginning to use FUEL in their extensions, I think it'd be good to port FUEL to SeaMonkey in order to make it easier to build cross-application extensions. Is the plan to create a new library on top of the toolkit common code, or is it going to be a FUEL + STEEL port?

Nobody is working on it yet, but I think starting with a port of whatever from FUEL and STEEL is applicable would be best. We can extend on top of that, we surely should try to support the same APIs where possible though.

Here's the same patch from bug 483702, with all FUEL references changed to SMILE, as well as new UUID for the interfaces. It's all working fine, as far as I can tell. I still have the doubts I posted on bug 483702 comment #1, plus the following: - Is "@mozilla.org/smile/application;1" appropriate for the SMILE component? Particularly the mozilla.org part.

Comment on attachment 368037 [details] [diff] [review] Patch V1, FUEL refactored to SMILE Thanks for the patch, you should actually ask for review to get feedback on the questions you asked. I haven't really waded through the patch, but I think it would be better to place this in suite/common rather than suite/browser as it should be developed in a direction where it offers mailnews stuff or even other things as well.

nowhere near a real review (and I'm not the one to review anyway), but the bookmark features of <s>FUEL</s>SMILE should probably be ported to use the HTML, *or* throw *or* be no-ops (in descending preference); if this is to land. Having it operate on SQL when the SQL is not actually used by us would be counter-intuitive to what many would expect, users and devs, both.

I thought Places support was landing for 2.0. Isn't that right?

Places history, not bookmarks.

SMILE rules ! ;-D

> bookmark features of <s>FUEL</s>SMILE should probably be ported to use the > HTML, *or* throw *or* be no-ops (in descending preference); if this is to land. This would break the ported FUEL tests as well right? Unless those tests were commented out. Otherwise I would think throwing NS_ERROR_NOT_IMPLEMENTED would be best.

And there might well be Firefox extensions (that could be ported to SeaMonkey) that use Places Bookmarks to store their metadata - Feed readers come to mind.

I agree it might be better to throw an error instead of implementing an incompatible API, specially if it's going to be updated for 2.1 anyway.

This moves the library to suite/common, and disables bookmark functionality. Application.bookmarks now throws a NS_ERROR_NOT_IMPLEMENTED. Pending issues: - The tests in the smile/test directory were copied and adapted from FUEL. Can I run these tests using the SeaMonkey build/test system? How? - Is "@mozilla.org/smile/application;1" appropriate for the SMILE component? Particularly the mozilla.org part. - The Places bookmarks functionality is supposed to be implemented in SM 2.1, so 2.0 will use the current HTML/RDF storage. I think it's best to wait for 2.1 and introduce a compatible bookmarks API for the library, but I can also try to re-implement it using the old system. What do you think?

(In reply to comment #14) > - The tests in the smile/test directory were copied and adapted from FUEL. Can > I run these tests using the SeaMonkey build/test system? How? No idea, please ask someone else to review the tests. Sorry. > - Is "@mozilla.org/smile/application;1" appropriate for the SMILE component? > Particularly the mozilla.org part. Sure, we're all mozilla.org projects here :-) > - The Places bookmarks functionality is supposed to be implemented in SM 2.1, > so 2.0 will use the current HTML/RDF storage. I think it's best to wait for 2.1 > and introduce a compatible bookmarks API for the library, but I can also try to > re-implement it using the old system. What do you think? I would wait. In fact, I wouldn't even declare a bookmarks property yet.

(In reply to comment #15) > (In reply to comment #14) > > - The tests in the smile/test directory were copied and adapted from FUEL. Can > > I run these tests using the SeaMonkey build/test system? How? > No idea, please ask someone else to review the tests. Sorry. Who knows about tests? > > - The Places bookmarks functionality is supposed to be implemented in SM 2.1, > > so 2.0 will use the current HTML/RDF storage. I think it's best to wait for 2.1 > > and introduce a compatible bookmarks API for the library, but I can also try to > > re-implement it using the old system. What do you think? > I would wait. In fact, I wouldn't even declare a bookmarks property yet. I think I agree with Phillip Chee on this. It's more informative that Application.bookmarks throws an exception instead of just being undefined. And this way we keep an interface that is equivalent to the one in FUEL.

(In reply to comment #14) > - The tests in the smile/test directory were copied and adapted from FUEL. Can > I run these tests using the SeaMonkey build/test system? How? SeaMonkey tests can be run the same as Firefox tests, depending on what test suite they are in, the commands vary slightly, but all are now available through make targets from the objdir. See https://developer.mozilla.org/en/Mozilla_automated_testing on more info about the testsuite we have in Mozilla projects, SeaMonkey supports all of them.

This new patch fixes the Mochitests so that they work in the new location. Other than fixing a few paths in the Makefile and within the test files themselves, I removed the bookmarks test file given that it won't be implemented in this version. All tests are passing now. The only pending issue so far is whether Application.bookmarks should throw an exception (current behavior), or be undefined.

Comment on attachment 369133 [details] [diff] [review] Patch V3, with passing Mochitests >diff -r bd9dc914d935 suite/common/Makefile.in >--- a/suite/common/Makefile.in Fri Mar 13 13:08:08 2009 +0100 >+++ b/suite/common/Makefile.in Tue Mar 24 14:13:41 2009 -0600 >@@ -37,34 +37,34 @@ > > DEPTH = ../.. > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > >-PARALLEL_DIRS = public src >+PARALLEL_DIRS = public src smile smile doesn't belong as subdir of suite/common; please either move the files to common/* or create suite/smile/* >+# The Original Code is FUEL. I doubt you can claim Makefiles as originally FUEL, as they're so generic. >+DIRS = public src >+ >+ifdef ENABLE_TESTS >+DIRS += test >+endif PARALLEL_DIRS >+ smileIBrowserTab open(in nsIURI aURI); Weird. I guess FUEL uses a URI here? >+ /** >+ * The root bookmarks object for the application. >+ * Contains all the bookmark roots in the system. >+ * Currently unsupported, returns NS_ERROR_NOT_IMPLEMENTED. >+ */ >+ readonly attribute smileIBookmarkRoots bookmarks; I'd prefer that it either simply wasn't there, or returned null. >+const Ci = Components.interfaces; >+const Cc = Components.classes; Personally I don't like these shorthands... >+ for (var i=0; i<browsers.length; i++) Nit: spaces around operators, i.e. i = 0; i < etc. >+ /* >+ * Helper used to determine the index offset of the browsertab >+ */ Hmm, this is ugly. Would it be possible to either a) store the tab only, and use .linkedBrowser when you need the browser, or b) store both?

(In reply to comment #19) > (From update of attachment 369133 [details] [diff] [review]) > >diff -r bd9dc914d935 suite/common/Makefile.in > >--- a/suite/common/Makefile.in Fri Mar 13 13:08:08 2009 +0100 > >+++ b/suite/common/Makefile.in Tue Mar 24 14:13:41 2009 -0600 > >@@ -37,34 +37,34 @@ > > > > DEPTH = ../.. > > topsrcdir = @top_srcdir@ > > srcdir = @srcdir@ > > VPATH = @srcdir@ > > > > include $(DEPTH)/config/autoconf.mk > > > >-PARALLEL_DIRS = public src > >+PARALLEL_DIRS = public src smile > smile doesn't belong as subdir of suite/common; please either move the files to > common/* or create suite/smile/* Done, moved to suite/smile. > >+# The Original Code is FUEL. > I doubt you can claim Makefiles as originally FUEL, as they're so generic. > > >+DIRS = public src > >+ > >+ifdef ENABLE_TESTS > >+DIRS += test > >+endif > PARALLEL_DIRS I set them as Original Code is SMILE. Is that OK? > >+ smileIBrowserTab open(in nsIURI aURI); > Weird. I guess FUEL uses a URI here? They use nsIURI, I don't know why. It's not very friendly IMO. > >+ /** > >+ * The root bookmarks object for the application. > >+ * Contains all the bookmark roots in the system. > >+ * Currently unsupported, returns NS_ERROR_NOT_IMPLEMENTED. > >+ */ > >+ readonly attribute smileIBookmarkRoots bookmarks; > I'd prefer that it either simply wasn't there, or returned null. Removed. > >+const Ci = Components.interfaces; > >+const Cc = Components.classes; > Personally I don't like these shorthands... I guess it's a matter of preference. Since they're inside a component, the won't do harm to the global scope. > >+ for (var i=0; i<browsers.length; i++) > Nit: spaces around operators, i.e. i = 0; i < etc. Done. > >+ /* > >+ * Helper used to determine the index offset of the browsertab > >+ */ > Hmm, this is ugly. Would it be possible to either a) store the tab only, and > use .linkedBrowser when you need the browser, or b) store both? I'm not sure, but there might be a case where the tab element is destroyed and created again. Perhaps when a tab is moved to a different position, or a different window?

Here's the patch with the review changes.

(In reply to comment #20) >(In reply to comment #19) >>(From update of attachment 369133 [details] [diff] [review]) >>>+ /* >>>+ * Helper used to determine the index offset of the browsertab >>>+ */ >>Hmm, this is ugly. Would it be possible to either a) store the tab only, and >>use .linkedBrowser when you need the browser, or b) store both? >I'm not sure, but there might be a case where the tab element is destroyed and >created again. Perhaps when a tab is moved to a different position, or a >different window? I don't think a tab's .linkedBrowser ever changes; moving a tab doesn't break the link, and moving a tab to another window has to create a new tab and browser in that window anyway.

> moving a tab to another window has to create a new tab and > browser in that window anyway. I think that bz did some back-end magic recently that allows Firefox to actually move the <browser> to another window rather than recreating it.

(In reply to comment #23) > > moving a tab to another window has to create a new tab and > > browser in that window anyway. > > I think that bz did some back-end magic recently that allows Firefox to > actually move the <browser> to another window rather than recreating it. Bug 449728 is about getting this to work on SeaMonkey, backend is there, frontend needs work.

(In reply to comment #23) > > moving a tab to another window has to create a new tab and > > browser in that window anyway. > > I think that bz did some back-end magic recently that allows Firefox to > actually move the <browser> to another window rather than recreating it. Yeah, that's fine, and that's probably why the BrowserTab object stores the browser, not the tab. That was my point, if we store the tab in the object, it may be an invalid pointer after it is moved.

(In reply to comment #23) > > moving a tab to another window has to create a new tab and > > browser in that window anyway. > I think that bz did some back-end magic recently that allows Firefox to > actually move the <browser> to another window rather than recreating it. No, it doesn't, it does what I said it does, OK? If you want to get technical, it swaps frame loaders.

Jorge, any progress on this?

Patch V4 is awaiting review. On comment #25 I'm giving a possible explanation why we store the browser and not the tab object, which hasn't been addressed by anyone. My point was that when a tab is moved to a different window, the tab object changes, while the browser object doesn't, so that's why it makes more sense to store the more persistent object, so that BrowserTab remains valid. Might be worth asking the original devs for input.

1. Fixed bit-rot. 2. Ported two FUEL bugs: Bug 464260 FUEL: Change nsIDOMHTMLDocument check to nsIDOMDocument check Bug 470163 FUEL: pass BrowserTab object as event data for Tab* events Review Comments: >>+ /* >>+ * Helper used to determine the index offset of the browsertab >>+ */ > Hmm, this is ugly. Would it be possible to either a) store the tab only, and > use .linkedBrowser when you need the browser, or b) store both? Now stores the tab in addition and references the stored value when needed. Passes all mochitests: Browser Chrome Test Summary Pass: 73 Fail: 0 Todo: 0

> +function BrowserTab(aSMILEWindow, aBrowser) { > + this._window = aSMILEWindow; > + this._tabbrowser = aSMILEWindow._tabbrowser; Firefox changed aWindow to aFUELWindow so I followed suite but I'm not sure about the aesthetics.

Changes since the last patch: 1. Relative path in the #include line was wrong. 2. Removed usage of Cc/Ci. I left the definitions in as the toolkit part pulled in by the #include needs them. 3. Reformatted the long lines caused by replacing Cc/Ci with their full representations.

Comment on attachment 392171 [details] [diff] [review] Patch V5.1 Removed usage of Ci/Cc >+function BrowserTab(aSMILEWindow, aBrowser) { >+ this._window = aSMILEWindow; >+ this._tabbrowser = aSMILEWindow._tabbrowser; >+ this._browser = aBrowser; >+ this._tab = this._tabbrowser.mTabs[this.index] || null; function BrowserTab(aSMILEWindow, aTab) { this._window = aSMILEWindow; this._tabbrowser = aSMILEWindow._tabbrowser; this._browser = aTab.linkedBrowser; this._tab = aTab; (And fix the callers and null out _tab on shutdown of course.) (Only remove _browser if you really feel like it.) >+ get index() { return this._tabbbrowser.getTabIndex(this._tab); ?

(In reply to comment #32) > (From update of attachment 392171 [details] [diff] [review]) > >+function BrowserTab(aSMILEWindow, aBrowser) { > function BrowserTab(aSMILEWindow, aTab) { Does that result in different outward-facing interface than FUEL? We'd like to avoid that, if possible.

> Does that result in different outward-facing interface than FUEL? We'd like to > avoid that, if possible. This is an internal implementation detail. As far as I can see, it isn't directly exposed in the IDL.

(In reply to comment #34) > This is an internal implementation detail. As far as I can see, it isn't > directly exposed in the IDL. OK, as long as BrowserTab() is not (expected to be) exposed to and used by extensions, it's OK to work differently, I just wanted to be sure ;-)

Talking about the IDL. The following works in Minefield: var foo = Application.activeWindow.activeTab; (foo instanceof Components.interfaces.fuelIBrowserTab); But in our SMILE version the above will complain to the error console. An extension author would have to do: var foo = Application.activeWindow.activeTab; (foo instanceof Components.interfaces.smileIBrowserTab); So how compatible do we have to be with FUEL at the API/IDL level?

I think we can punt on that issue for now, we should be as similar as possible, but we are not Firefox, so we might not be able to give people the same interfaces, just ones that implement some of the same functions, so having different interface names might just be the right thing. Where we offer the same functions, we should make them act the same as much as possible, though.

> (From update of attachment 392171 [details] [diff] [review]) >>+function BrowserTab(aSMILEWindow, aBrowser) { >>+ this._window = aSMILEWindow; >>+ this._tabbrowser = aSMILEWindow._tabbrowser; >>+ this._browser = aBrowser; >>+ this._tab = this._tabbrowser.mTabs[this.index] || null; > function BrowserTab(aSMILEWindow, aTab) { > this._window = aSMILEWindow; > this._tabbrowser = aSMILEWindow._tabbrowser; > this._browser = aTab.linkedBrowser; > this._tab = aTab; > (And fix the callers and null out _tab on shutdown of course.) Fixed. >>+ get index() { > return this._tabbbrowser.getTabIndex(this._tab); ? Fixed. getTabIndex() can throw, so I used a try/catch block to catch the exception and return -1.

Comment on attachment 393483 [details] [diff] [review] Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com> I guess that only happens for a deleted tab so it's a rare occurrence anyway.

Pushed as http://hg.mozilla.org/comm-central/rev/23414c2c2381 - I think this can be marked FIXED now and further improvements like integration of things from STEEL should be done in followups. Is Jorge still around to do the honors of resolving the bug or will Philip do that?

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query