create SMILE (SeaMonkey Interface Library for Extensions) as a combo of FUEL and STEEL

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
General
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Robert Kaiser, Assigned: jorgev)

Tracking

({dev-doc-needed, fixed-seamonkey2.0})

Trunk
seamonkey2.0b2
dev-doc-needed, fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
in-testsuite +
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 2

9 years ago
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?
(Reporter)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 483702

Updated

9 years ago
Duplicate of this bug: 483702
(Assignee)

Comment 5

9 years ago
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.
Assignee: general → jorge.villalobos
Status: NEW → ASSIGNED
(Reporter)

Comment 6

9 years ago
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.
Attachment #368037 - Flags: review?(neil)
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.
(Assignee)

Comment 8

9 years ago
I thought Places support was landing for 2.0. Isn't that right?
Places history, not bookmarks.
No longer depends on: 483702
SMILE rules ! ;-D
Flags: wanted-seamonkey2?
Target Milestone: --- → seamonkey2.0b1

Comment 11

9 years ago
> 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

9 years ago
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.
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
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?
Attachment #368037 - Attachment is obsolete: true
Attachment #368092 - Flags: review?(neil)
Attachment #368037 - Flags: review?(neil)
(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.
Depends on: 483702
Target Milestone: seamonkey2.0b1 → ---
(Assignee)

Comment 16

9 years ago
(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.
(Reporter)

Comment 17

9 years ago
(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.
(Assignee)

Comment 18

9 years ago
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.
Attachment #368092 - Attachment is obsolete: true
Attachment #369133 - Flags: review?(neil)
Attachment #368092 - Flags: review?(neil)
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?
(Assignee)

Comment 20

8 years ago
(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?
(Assignee)

Comment 21

8 years ago
Created attachment 369395 [details] [diff] [review]
Patch V4, review changes

Here's the patch with the review changes.
Attachment #369133 - Attachment is obsolete: true
Attachment #369395 - Flags: review?(neil)
Attachment #369133 - Flags: review?(neil)
(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

8 years ago
> 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.
(Reporter)

Comment 24

8 years ago
(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.
(Assignee)

Comment 25

8 years ago
(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.
(Reporter)

Comment 27

8 years ago
Jorge, any progress on this?
(Assignee)

Comment 28

8 years ago
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.
No longer depends on: 483702
Depends on: 408370

Comment 29

8 years ago
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
Attachment #369395 - Attachment is obsolete: true
Attachment #392098 - Flags: review?(neil)
Attachment #369395 - Flags: review?(neil)

Comment 30

8 years ago
> +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

8 years ago
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.
Attachment #392098 - Attachment is obsolete: true
Attachment #392171 - Flags: review?(neil)
Attachment #392098 - Flags: review?(neil)
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); ?
(Reporter)

Comment 33

8 years ago
(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

8 years ago
> 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.
(Reporter)

Comment 35

8 years ago
(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

8 years ago
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?
(Reporter)

Comment 37

8 years ago
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

8 years ago
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.
Attachment #392171 - Attachment is obsolete: true
Attachment #393483 - Flags: review?(neil)
Attachment #392171 - Flags: review?(neil)
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.
Attachment #393483 - Flags: review?(neil) → review+

Updated

8 years ago
Attachment #393483 - Attachment description: Patch v5.1 Fixed Nits. → [for check-in] Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com> r=neil

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 40

8 years ago
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?
Flags: wanted-seamonkey2? → wanted-seamonkey2+
Keywords: checkin-needed

Updated

8 years ago
Attachment #393483 - Attachment description: [for check-in] Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com> r=neil → Patch v5.2 Fixed Nits. p=Jorge Villalobos <jorge.villalobos@gmail.com>

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 464260, 470163
Target Milestone: --- → seamonkey2.0b2
Flags: in-testsuite+
(Reporter)

Updated

8 years ago
Keywords: dev-doc-needed

Updated

8 years ago
Depends on: 511096
(Reporter)

Comment 41

8 years ago
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Keywords: fixed-seamonkey2.0
You need to log in before you can comment on or make changes to this bug.