[FUEL] Factor out generic toolkit aspects of FUEL into a reusable form

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
General
P4
normal
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Joey Minta, Assigned: Joey Minta)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
The current FUEL interfaces include both Firefox-specific interfaces (e.g. fuelIBookmark) and generic toolkit interfaces (e.g. fuelIConsole) that other applications would likely find useful when creating a similar library.  The current implementation should be factored in such a way that these other applications can re-use these parts without the need to cut-and-paste the code.

Possible solutions include a js-module, use of mozISubScriptLoader, or even a simple #include file.

I believe the following is a complete list of the current toolkit-generic FUEL interfaces:
    * fuelIConsole
    * fuelIExtensions
    * fuelIExtension
    * fuelIPreference
    * fuelIPreferenceBranch
    * fuelIEvents
    * fuelIEventListener
    * fuelIEventItem

See also bug 380168 for some relevant discussion of the issue.
Flags: blocking-firefox3?
(Assignee)

Comment 1

10 years ago
Despite the general rule against confirmed bugs in Firefox:General, there doesn't seem to be a better place for FUEL bugs...
Component: Extension/Theme Manager → General
QA Contact: extension.manager → general

Updated

10 years ago
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
(Assignee)

Comment 2

10 years ago
Created attachment 292960 [details] [diff] [review]
WIP

This factors out the toolkit elements into a separate js object that can be inherited by an Application implementation.  It works great until I try to move the objects into their own file to be loaded as a subscript.  Then, it only fails because I can't seem to get the build system to package my extra script.

To do:
*Figure out enough Make to package the extra script
*Place extra script in toolkit directory so other applications will check it out from cvs
*Factor out fuelIApplication

Any help people can offer with the Make stuff would be appreciated.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Attachment #292960 - Attachment is patch: true
Attachment #292960 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 3

10 years ago
Created attachment 292964 [details] [diff] [review]
WIP2

This time with interfaces factored out.  All that's left is properly getting these things into the toolkit/ realm and making the build system realize they exist.
Attachment #292960 - Attachment is obsolete: true
(Assignee)

Comment 4

10 years ago
Created attachment 293032 [details] [diff] [review]
patch v1

It seems Make and I have come to a truce.  This patch fully separates the toolkit parts of FUEL into a reusable form.  In a moment, I'll attach the patch I used to get identical toolkit functionality into Thunderbird.  Other toolkit consumers should be able to create analogous patches for their apps.

This patch passes all the mochitests.

mfinkle expressed some concern that the subscript approach may impact startup time.  I don't really know a good way to test this short of landing, but we should definitely keep an eye out.  A less dangerous approach would be to #include all the objects (Console, Extensions, etc), but this would still require re-writing getters into every application's Application object.  I passed on the js-module approach since we don't expect this file to ever be loaded more than once per runtime.
Attachment #292964 - Attachment is obsolete: true
Attachment #293032 - Flags: superreview?(gavin.sharp)
Attachment #293032 - Flags: review?(mark.finkle)
(Assignee)

Comment 5

10 years ago
Created attachment 293034 [details] [diff] [review]
Thunderbird patch

This patch would add FUEL functionality to Thunderbird.  Similar patches could be written for Sunbird and other toolkit consumers.  I won't land this patch until I've also implemented some mail-specific bits into the steel Application.
Comment on attachment 293032 [details] [diff] [review]
patch v1

Can you add a comment to this so its clear to others whats happening?
+    this.__proto__.observe(aSubject, aTopic, aData);

We'll have to watch the Ts hit, if any.
Attachment #293032 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

10 years ago
Blocks: 408370
> +    this.__proto__.observe(aSubject, aTopic, aData);
If you are planning to call the observe method of the base class, shouldn't this be:

> this.__proto__.__proto__.observe.call(this, aSubject, aTopic, aData);

(Assignee)

Comment 8

10 years ago
(In reply to comment #7)
You are exactly right.  Thanks for the catch!  New patch coming soon.

(Assignee)

Updated

10 years ago
Attachment #293032 - Flags: superreview?(gavin.sharp)
(Assignee)

Comment 9

10 years ago
Created attachment 293328 [details] [diff] [review]
patch v2

This version fixes the __proto__ issue and adds a comment as requested by mfinkle.  Additionally, I did some seat-of-the-pants profiling with Date.now() and was seeing an unfortunate hit of about 8-10ms during the call to loadSubScript().  While small, this seemed likely to be unacceptable.  As a result, this patch also switches to #include-ing the toolkit file, rather than loading it at runtime.  The result is, of course, equivalent.

Carrying over r+.
Attachment #293032 - Attachment is obsolete: true
Attachment #293034 - Attachment is obsolete: true
Attachment #293328 - Flags: superreview?(gavin.sharp)
Attachment #293328 - Flags: review+

Updated

10 years ago
Blocks: 411536
Comment on attachment 293328 [details] [diff] [review]
patch v2

You should get Benjamin to approve the addition of toolkit/components/exthelper .

>Index: toolkit/components/exthelper/extIApplication.idl

No license header?
Attachment #293328 - Flags: superreview?(gavin.sharp) → review+
(Assignee)

Comment 11

10 years ago
Comment on attachment 293328 [details] [diff] [review]
patch v2

The fuel idl didn't have a license, so I didn't add one here either.  I can throw one in if people want.

bsmedberg: Please see gavin's comment #10 re adding the exthelper folder.
Attachment #293328 - Flags: superreview?(benjamin)
Comment on attachment 293328 [details] [diff] [review]
patch v2

I just skimmed this: I'm just giving MOA based on existing reviews.
Attachment #293328 - Flags: superreview?(benjamin) → superreview+
(In reply to comment #11)
> (From update of attachment 293328 [details] [diff] [review])
> The fuel idl didn't have a license, so I didn't add one here either.  I can
> throw one in if people want.

I am going to patch fuelIApplication to add a license header - or you could add it as part of this patch :)

up to you
(Assignee)

Comment 14

10 years ago
Comment on attachment 293328 [details] [diff] [review]
patch v2

Requesting approval.  This is fairly low risk in that the vast majority of it is just moving code around.  The patch passes all the FUEL mochitests.
Attachment #293328 - Flags: approval1.9?
Comment on attachment 293328 [details] [diff] [review]
patch v2

a=beltzner for 1.9
Attachment #293328 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 16

10 years ago
I'm in the midst of exams so I'm going to get someone else to land this.  Re: the license header, the fuelApplication.js contributors list should be sufficient.
Keywords: checkin-needed
Added missing license header to fuelIApplication.idl and extIApplication.idl.

Checking in browser/fuel/public/fuelIApplication.idl;
/cvsroot/mozilla/browser/fuel/public/fuelIApplication.idl,v  <--  fuelIApplication.idl
new revision: 1.10; previous revision: 1.9
done
Checking in browser/fuel/src/fuelApplication.js;
/cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v  <--  fuelApplication.js
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.76; previous revision: 1.75
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/Makefile.in,v
done
Checking in toolkit/components/exthelper/Makefile.in;
/cvsroot/mozilla/toolkit/components/exthelper/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/extApplication.js,v
done
Checking in toolkit/components/exthelper/extApplication.js;
/cvsroot/mozilla/toolkit/components/exthelper/extApplication.js,v  <--  extApplication.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/extIApplication.idl,v
done
Checking in toolkit/components/exthelper/extIApplication.idl;
/cvsroot/mozilla/toolkit/components/exthelper/extIApplication.idl,v  <--  extIApplication.idl
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed this out due to orange. See http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unix&logfile=1200210780.1200211853.15867.gz&buildtime=1200210780&buildname=Linux%20fxdbug-linux-tbox%20Depend&fulltext=1 for full log.


  ###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file nsWeakReference.cpp, line 109
  NS_GetWeakReference(nsISupports*, unsigned int*)+0x000000A3 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0002C8F1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004A622]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000495F0]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004AC82]
  NS_InvokeByIndex_P+0x00000029 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000BFF45]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0005D27C]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0006C128]
  js_Invoke+0x00000BC7 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00064B47]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0007669D]
  js_Invoke+0x00000C51 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00064BD1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000561DC]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0004D7A5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000C0DD6]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x00049497]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004A985]
  NS_ShutdownXPCOM_P+0x000001DA [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x00039A94]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxul.so +0x00027F0E]
  XRE_main+0x000031B1 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxul.so +0x0002E0C1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/firefox-bin +0x00000DC1]
  __libc_start_main+0x000000DC [/lib/libc.so.6 +0x00015DEC]
  nsDOMNodeAllocator leaked 118616 bytes 
  nsStringStats
   => mAllocCount:          27190
   => mReallocCount:         5189
   => mFreeCount:           22466  --  LEAKED 4724 !!!
   => mShareCount:          28373
   => mAdoptCount:           2062
   => mAdoptFreeCount:       2059  --  LEAKED 3 !!!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oh, and please don't forget to update toolkit/toolkit-makefiles.sh in your next revision of the patch.
While the patch was landed, it did cause a 8-9ms Ts win on Linux. Hopefully the fixed patch will do the same thing. :)
Depends on: 413094
Created attachment 310402 [details] [diff] [review]
FUEL refactoring patch, v3 (thanks to emre)
Attachment #293328 - Attachment is obsolete: true
(In reply to comment #21)
> Created an attachment (id=310402) [details]
> FUEL refactoring patch, v3 (thanks to emre)
> 

+    os.addObserver(this, "quit-application-granted", false);
+    os.addObserver(this, "quit-application", false);

Thanks for updating this patch. Note, we removed the use of these notification on trunk and the leak is probably still present.

Comment 23

9 years ago
Created attachment 310811 [details] [diff] [review]
FUEL refactoring patch, v4

- This patch does merge the latest (trunk) fuel library and Jminta's extHelper library.
- It doesn't fix the memory leaks, it only fixes the WeakReference problem. The root of this problem is passing extApplication reference to nsIObserverService. Even when we pass extApplication's reference to addObserver with ownsWeak = false, it gives this error.  Adding XPCOMUtils.generateQI([Ci.nsISupportsWeakReference]) to extApplication fixes the problem. Don't know why -- if somebody explains would be great.
- When I run mochitest on the trunk (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032010 Minefield/3.0b5pre), I get leaks. I guess these leaks nothing have to do wih this refactored library. You can find the mochitest results before and after the patch.
Attachment #310811 - Flags: review?(mark.finkle)

Comment 24

9 years ago
Created attachment 310812 [details]
Mochitest results before and after the patch.
Comment on attachment 310811 [details] [diff] [review]
FUEL refactoring patch, v4

>Index: browser/fuel/public/fuelIApplication.idl

>-
>-/**
>- * Interface representing a container for bookmark roots. Roots
>- * are the top level parents for the various types of bookmarks in the system.
>- */
>-[scriptable, uuid(c9a80870-eb3c-11dc-95ff-0800200c9a66)]
>-interface fuelIBookmarkRoots : nsISupports
>-{
>-  /**
>-   * The folder for the 'bookmarks menu' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder menu;
>-
>-  /**
>-   * The folder for the 'personal toolbar' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder toolbar;
>-
>-  /**
>-   * The folder for the 'tags' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder tags;
>-
>-  /**
>-   * The folder for the 'unfiled bookmarks' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder unfiled;
>+/**                                                               
>+ * Interface representing a container for bookmark roots. Roots   
>+ * are the top level parents for the various types of bookmarks in the system.  
>+ */                                                               
>+[scriptable, uuid(c9a80870-eb3c-11dc-95ff-0800200c9a66)]          
>+interface fuelIBookmarkRoots : nsISupports                        
>+{                                                                 
>+  /**                                                             
>+   * The folder for the 'bookmarks menu' root.                    
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder menu;                    
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'personal toolbar' root.                  
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder toolbar;                 
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'tags' root.                              
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder tags;                    
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'unfiled bookmarks' root.                 
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder unfiled;                 
> };

Weird diff, but should be ok

> //=================================================
> // Application implementation
> Application.prototype = {
>   // for nsIClassInfo + XPCOMUtils
>   classDescription: "Application",
>   classID:          Components.ID("fe74cf80-aa2d-11db-abbd-0800200c9a66"),
>   contractID:       "@mozilla.org/fuel/application;1",
> 
>   // redefine the default factory for XPCOMUtils
>   _xpcom_factory: ApplicationFactory,
> 
>-  // get this contractID registered for certain categories via XPCOMUtils
>-  _xpcom_categories: [
>-    // make Application a startup observer
>-    { category: "app-startup", service: true },
>-
>-    // add Application as a global property for easy access
>-    { category: "JavaScript global privileged property" }
>-  ],
>-
>-  get id() {
>-    return this._info.ID;
>-  },
>-
>-  get name() {
>-    return this._info.name;
>-  },
>+  // for nsISupports
>+  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIApplication, Ci.nsIObserver, Ci.nsIClassInfo]),

nsIClassInfo is here, but do you impl all the methods?

>+  getInterfaces : function app_gi(aCount) {
>+    var interfaces = [Ci.fuelIApplication, Ci.nsIObserver, Ci.nsIClassInfo];
>+    aCount.value = interfaces.length;
>+    return interfaces;

Here too, in fact this is a nsIClassInfo method, but others have been removed.


>+#include ../../../toolkit/components/exthelper/extApplication.js
>Index: toolkit/components/Makefile.in

>+  // for nsIClassInfo
>+  flags : Ci.nsIClassInfo.SINGLETON,
>+  implementationLanguage : Ci.nsIProgrammingLanguage.JAVASCRIPT,
>+
>+  getInterfaces : function app_gi(aCount) {
>+    var interfaces = [Ci.extIApplication, Ci.nsIObserver, Ci.nsIClassInfo];
>+    aCount.value = interfaces.length;
>+    return interfaces;
>+  },
>+
>+  getHelperForLanguage : function app_ghfl(aCount) {
>+    return null;
>+  },

So the methods are here, but getInterfaces is sorta duplicated from fuelApplication. I wonder what the proper thing to do here is? Its probably ok, but fuelApplication should have the other methods too, I think.

>+
>+  QueryInterface : XPCOMUtils.generateQI([Ci.nsISupportsWeakReference])
>+};

You should probably include sxtIApplication here too.

>Index: toolkit/components/exthelper/extIApplication.idl

>+/**
>+ * Interface holds information about an event.
>+ */
>+[scriptable, function, uuid(05281820-ab62-11db-abbd-0800200c9a66)]
>+interface extIEventItem : nsISupports
>+{
>+  /**
>+   * The name of the event
>+   */
>+  readonly attribute AString type;
>+
>+  /**
>+   * Can hold extra details and data associated with the event. This
>+   * is optional and event specific. If the event does not send extra
>+   * details, this is null.
>+   */
>+  readonly attribute AString data;

This was changed to a nsIVariant in another patch (jminta did the change)

Try cleaning up the QI's and the nsIClassInfo situation. Also fix (sync) the extIEventItem.data change.

r- for now
Attachment #310811 - Flags: review?(mark.finkle) → review-
(In reply to comment #25)
> Weird diff, but should be ok

Actually it looks like that's adding a bunch of trailing whitespace, which should be avoided. There are other places in the patch where you're adding extra space on blank lines, too.
(Assignee)

Comment 27

9 years ago
(In reply to comment #25)
> 
> nsIClassInfo is here, but do you impl all the methods?
extApplication implements all of the methods, and the FUEL Application object overrides those methods it wants to change.

> Here too, in fact this is a nsIClassInfo method, but others have been removed.
Not removed, just moved to the proto.

> This was changed to a nsIVariant in another patch (jminta did the change)
Hasn't landed yet. 

(In reply to comment #27)
> > This was changed to a nsIVariant in another patch (jminta did the change)
> Hasn't landed yet. 

(just adding extra info)
patch is in attachment 310833 [details] [diff] [review] on bug 424203 

Comment 29

9 years ago
Created attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

Based on Jminta's comments I didn't changed the nsIClassInfo interface implementation in Application and extApplication. This is what is different for revision 5:

- clears the white spaces as much as possible
- adds extIApplication into QueryInterface property
- changes event attribute to nsIVariant (bug 424203 - thanks Bryan )

thanks for the review
Attachment #310811 - Attachment is obsolete: true
Attachment #310812 - Attachment is obsolete: true
Attachment #311426 - Flags: review?

Updated

9 years ago
Attachment #311426 - Flags: review? → review?(mark.finkle)
Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

Looks better. Let's do this!
Attachment #311426 - Flags: review?(mark.finkle) → review+
Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

Requesting approval for 1.9b5.  

Reward: this allows all xulrunner consumers to use and extend FUEL.  It's a pre-requisite for STEEL, the Thunderbird version of this, which we want for Tb3.  Additionally, extensions which are compatible with both (e.g.) Firefox and Thunderbird can use the generic parts of FUEL in both.

Risk: low: passes all tests, this is mostly code motion, the main substantive change is the nsIVariant switch (already approved by mfinkle).  Because of an extension compatibility question*, it would be slightly preferable for this to have a beta to bake in.

* if an existing consumer of FUEL cares about the names of the interfaces that the FUEL objects implement (as I understand it, most [all?] shouldn't), this patch changes many of those.
Attachment #311426 - Flags: approval1.9b5?
Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

a1.9b5=beltzner
Attachment #311426 - Flags: approval1.9b5? → approval1.9b5+
Landed; I go to commute, and jminta is keeping an eye on the tree.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 425062
From what I can tell, the Firefox 3 (and later) FUEL documentation missed the interface name changes that went on here (https://developer.mozilla.org/en/FUEL) and the fact these interfaces moved to toolkit so that all apps have access to them. Therefore adding dev-doc-needed keyword as I believe devmo needs an update for this bug.
Keywords: dev-doc-needed
I've seen a lot of work done on the FUEL docs in the last few weeks. Does that work result in this being done?
FUEL is dying (if not dead), we plan to archive its documentation as soon as it will be removed. We are not maintaining the current one.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.