Closed Bug 407963 Opened 14 years ago Closed 14 years ago
[FUEL] Factor out generic toolkit aspects of FUEL into a reusable form
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.
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
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
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
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
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.
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+
> + 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);
(In reply to comment #7) You are exactly right. Thanks for the catch! New patch coming soon.
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+.
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+
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
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+
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.
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
Closed: 14 years ago
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. :)
(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.
- 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)
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.
(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
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 #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
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
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.
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.
You need to log in before you can comment on or make changes to this bug.