Last Comment Bug 411536 - create SMILE (SeaMonkey Interface Library for Extensions) as a combo of FUEL and STEEL
: create SMILE (SeaMonkey Interface Library for Extensions) as a combo of FUEL ...
Status: RESOLVED FIXED
: dev-doc-needed, fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.0b2
Assigned To: Jorge Villalobos [:jorgev]
:
Mentors:
: 483702 (view as bug list)
Depends on: 407963 steel 464260 470163 511096
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-09 11:24 PST by Robert Kaiser (not working on stability any more)
Modified: 2009-09-18 06:18 PDT (History)
13 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch V1, FUEL refactored to SMILE (79.77 KB, patch)
2009-03-18 09:26 PDT, Jorge Villalobos [:jorgev]
no flags Details | Diff | Review
Patch V2, moved to common and removed bookmarks (66.66 KB, patch)
2009-03-18 13:28 PDT, Jorge Villalobos [:jorgev]
no flags Details | Diff | Review
Patch V3, with passing Mochitests (55.97 KB, patch)
2009-03-24 13:21 PDT, Jorge Villalobos [:jorgev]
no flags Details | Diff | Review
Patch V4, review changes (54.44 KB, patch)
2009-03-25 16:58 PDT, Jorge Villalobos [:jorgev]
no flags Details | Diff | Review
Patch V5 Fix bit-rot, review comments. (55.05 KB, patch)
2009-08-01 09:41 PDT, Philip Chee
no flags Details | Diff | Review
Patch V5.1 Removed usage of Ci/Cc (55.67 KB, patch)
2009-08-02 11:03 PDT, Philip Chee
no flags Details | Diff | Review
Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com> (55.55 KB, patch)
2009-08-10 00:37 PDT, Philip Chee
neil: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2008-01-09 11:24:37 PST
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 :)
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2008-01-09 11:35:08 PST
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.
Comment 2 Jorge Villalobos [:jorgev] 2009-02-18 12:16:40 PST
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?
Comment 3 Robert Kaiser (not working on stability any more) 2009-02-18 12:22:24 PST
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.
Comment 4 Philip Chee 2009-03-17 07:27:54 PDT
*** Bug 483702 has been marked as a duplicate of this bug. ***
Comment 5 Jorge Villalobos [:jorgev] 2009-03-18 09:26:17 PDT
Created attachment 368037 [details] [diff] [review]
Patch V1, FUEL refactored to SMILE

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 6 Robert Kaiser (not working on stability any more) 2009-03-18 09:32:19 PDT
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.
Comment 7 Justin Wood (:Callek) 2009-03-18 09:33:01 PDT
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.
Comment 8 Jorge Villalobos [:jorgev] 2009-03-18 09:38:38 PDT
I thought Places support was landing for 2.0. Isn't that right?
Comment 9 Justin Wood (:Callek) 2009-03-18 09:41:46 PDT
Places history, not bookmarks.
Comment 10 Serge Gautherie (:sgautherie) 2009-03-18 09:50:37 PDT
SMILE rules ! ;-D
Comment 11 Philip Chee 2009-03-18 10:03:26 PDT
> 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.
Comment 12 Philip Chee 2009-03-18 10:05:03 PDT
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.
Comment 13 Jorge Villalobos [:jorgev] 2009-03-18 11:06:33 PDT
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.
Comment 14 Jorge Villalobos [:jorgev] 2009-03-18 13:28:29 PDT
Created attachment 368092 [details] [diff] [review]
Patch V2, moved to common and removed bookmarks

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?
Comment 15 neil@parkwaycc.co.uk 2009-03-18 13:48:37 PDT
(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.
Comment 16 Jorge Villalobos [:jorgev] 2009-03-19 10:36:02 PDT
(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.
Comment 17 Robert Kaiser (not working on stability any more) 2009-03-19 15:33:37 PDT
(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.
Comment 18 Jorge Villalobos [:jorgev] 2009-03-24 13:21:47 PDT
Created attachment 369133 [details] [diff] [review]
Patch V3, with passing Mochitests

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 19 neil@parkwaycc.co.uk 2009-03-25 10:20:37 PDT
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?
Comment 20 Jorge Villalobos [:jorgev] 2009-03-25 16:58:23 PDT
(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?
Comment 21 Jorge Villalobos [:jorgev] 2009-03-25 16:58:50 PDT
Created attachment 369395 [details] [diff] [review]
Patch V4, review changes

Here's the patch with the review changes.
Comment 22 neil@parkwaycc.co.uk 2009-03-26 03:40:03 PDT
(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.
Comment 23 Philip Chee 2009-03-26 04:05:25 PDT
> 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.
Comment 24 Robert Kaiser (not working on stability any more) 2009-03-26 06:04:53 PDT
(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.
Comment 25 Jorge Villalobos [:jorgev] 2009-03-26 08:04:33 PDT
(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.
Comment 26 neil@parkwaycc.co.uk 2009-03-31 07:48:58 PDT
(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.
Comment 27 Robert Kaiser (not working on stability any more) 2009-05-10 09:36:00 PDT
Jorge, any progress on this?
Comment 28 Jorge Villalobos [:jorgev] 2009-05-10 11:00:18 PDT
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.
Comment 29 Philip Chee 2009-08-01 09:41:42 PDT
Created attachment 392098 [details] [diff] [review]
Patch V5 Fix bit-rot, review comments.

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
Comment 30 Philip Chee 2009-08-01 09:43:33 PDT
> +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.
Comment 31 Philip Chee 2009-08-02 11:03:31 PDT
Created attachment 392171 [details] [diff] [review]
Patch V5.1 Removed usage of Ci/Cc

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 32 neil@parkwaycc.co.uk 2009-08-04 09:30:18 PDT
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); ?
Comment 33 Robert Kaiser (not working on stability any more) 2009-08-04 11:23:07 PDT
(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.
Comment 34 Philip Chee 2009-08-04 20:42:02 PDT
> 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.
Comment 35 Robert Kaiser (not working on stability any more) 2009-08-05 03:33:06 PDT
(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 ;-)
Comment 36 Philip Chee 2009-08-05 08:10:39 PDT
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?
Comment 37 Robert Kaiser (not working on stability any more) 2009-08-05 08:21:31 PDT
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.
Comment 38 Philip Chee 2009-08-10 00:37:23 PDT
Created attachment 393483 [details] [diff] [review]
Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com>

> (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 39 neil@parkwaycc.co.uk 2009-08-10 04:21:44 PDT
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.
Comment 40 Robert Kaiser (not working on stability any more) 2009-08-10 06:15:33 PDT
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?
Comment 41 Robert Kaiser (not working on stability any more) 2009-09-18 06:18:20 PDT
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query

Note You need to log in before you can comment on or make changes to this bug.