381189 - [meta] Reduce code duplication in js xpcom by using the import module (XPCOMUtils.jsm)

Status: RESOLVED WORKSFORME

(Firefox :: General, enhancement)

Description

[Richard Klein]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4pre) Gecko/20070518 Firefox/2.0.0.3 Navigator/9.0a13
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4pre) Gecko/20070518 Firefox/2.0.0.3 Navigator/9.0a13

This is the Firefox bug to start using the new the new js code sharing system from bug 238324.  This should greatly reduce the amount of code duplication for javascript xpcom module registration.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Richard Klein]

The thunderbird version of this is bug 381190 and the toolkit version is bug 381191.

Comment 2

[Richard Klein]

Attachment: Bookmark Transaction Manager Patch

This patch replaces module registration in nsBookmarkTransactionManager.js with
the new XPCOMUtils.jsm's generateNSGetModule.  File size was reduced from
13895 bytes to 13068 bytes.  I've built with this applied and it does not seem to
have any ill affects.

Asking review from Myk Melez since he was the last to update this file.

Comment 3

[Richard Klein]

Attachment: Microsummary Service Patch

This patch replaces module registration in nsMicrosummaryService.js with
the new XPCOMUtils.jsm's generateNSGetModule.  File size was reduced from
89187 bytes to 87938 bytes.  I've built with this applied and it does not seem to
have any ill affects.

Asking review from thunder since he was the last to update this file.

Comment 4

[Alex Vincent [:WeirdAl]]

confirming for bug radar.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 265297 [details] [diff] [review]
Bookmark Transaction Manager Patch

Wow, great refactoring!  I'm looking forward to seeing more of this throughout the code.

Overall this is straightforward and looks fine, but I have some style nits.  The primary one is that there's too much inlining of literals, which makes it hard to read the data structures and method calls.

This would be easier to grok (at a slight increase in code size) if the data structures were declared before being used, i.e.:

function bkmkTxnMgrConstructor() {
  return new bookmarkTransactionManager();
},

var bkmkTxnMgrInterfaces = [Ci.nsIBookmarkTransactionManager];

var bkmkTxnMgrFactory = 
  XPCOMUtils.generateFactory(bkmkTxnMgrConstructor, bkmkTxnMgrInterfaces);

var bkmkTxnMgrModule = {
  className:  "Bookmark Transaction Manager",
  cid:        Components.ID("{8be133d0-681d-4f0b-972b-6a68e41afb62}"),
  contractID: "@mozilla.org/bookmarks/transactionmanager;1",
  factory:    bkmkTxnMgrFactory
};

var NSGetModule = XPCOMUtils.generateNSGetModule([bkmkTxnMgrModule]);

A few more nits (mostly reflected in the sample code above):

* Now's a good time to define a Ci helper constant in this file and use it to shorten Components.interfaces.nsIBookmarkTransactionManager to Ci.nsIBookmarkTransactionManager.

* I think it's worth spelling out "Constructor" in the name of the constructor function, although I realize that XPCOMUtils.generateFactory names its first argument "ctor".

* Since XPCOMUtils isn't an XPCOM component, and you aren't calling it through XPCOM, you don't have to include values for unused arguments, so you can leave off the two null values for the postRegister and preUnregister arguments to generateNSGetModule.

* Make sure there's a newline at the end of the file.

FWIW, I'm not sure touching this file last makes me the most appropriate reviewer for it, although changes to the component registration code are certainly within my range.  But since my review consists entirely of style nits, it'd be worth having the module owner take a look at this, so requesting super-review from mconnor.

Comment 6

[Alex Vincent [:WeirdAl]]

Comment on attachment 265297 [details] [diff] [review]
Bookmark Transaction Manager Patch

>Index: browser/components/bookmarks/src/nsBookmarkTransactionManager.js

>+    factory: XPCOMUtils.generateFactory(
>+      function bkmkCtor() {
>+        return new bookmarkTransactionManager();
>+      },

Note a patch for the original XPCOMUtils (r=sayrer, sr=bsmedberg) would require you change this first argument to simply be "bookmarkTransactionManager" (the function, not the string).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

> Note a patch for the original XPCOMUtils (r=sayrer, sr=bsmedberg) would require
> you change this first argument to simply be "bookmarkTransactionManager" (the
> function, not the string).

What's the bug number for this change?

Comment 8

[Richard Klein]

Per Alex's comment #4 in bug 381190, I'm going to morph this into a tracking bug and file individual bugs for each component.

Comment 9

[Richard Klein]

There has been some discussion that one bug is sufficient for each major area of change.  I can file individual bugs or place all the patches here.  Below is the list of files for Firefox that should be changed:

browser/components/bookmarks/src/nsBookmarkTransactionManager.js
browser/components/feeds/src/FeedConverter.js
browser/components/feeds/src/FeedWriter.js
browser/components/feeds/src/WebContentConverter.js
browser/components/microsummaries/src/nsMicrosummaryService.js
browser/components/safebrowsing/src/nsSafebrowsingApplication.js
browser/components/search/src/nsSearchService.js
browser/components/search/src/nsSearchSuggestion.js
browser/components/sessionstore/src/nsSessionStartup.js
browser/components/sessionstore/src/nsSessionStore.js
browser/components/shell/src/nsSetDefaultBrowser.js
browser/components/sidebar/src/nsSidebar.js
browser/components/nsBrowserContentHandler.js
browser/components/nsBrowserGlue.js
browser/fuel/fuelApplication.js

If anyone else wants to take on some of these files feel free.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Update: bug 381651 is changing XPCOMUtils' API.  See attachment 269013 [details] [diff] [review] in bug 385087 for an example of how to use the new API in a JS component file.

Comment 11

[Richard Klein]

Thanks for the update I've been holding off doing anything on this until the apis stabilize a bit more.

Comment 12

[Richard Klein]

Comment on attachment 265298 [details] [diff] [review]
Microsummary Service Patch

Removed the review request on an old patch.

Comment 13

[Mark Banner (:standard8)]

All the files referenced in comment 9 are using the XPCOMUtils API now, or have been removed. Hence I think this can be marked as WFM.