Replace module in nsMicrosummaryService.js with XPCOMUtils.jsm's generateNSGetModule

RESOLVED FIXED in Firefox 3 alpha7

Status

Firefox Graveyard
Microsummaries
--
enhancement
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: Richard Klein, Assigned: rflint)

Tracking

unspecified
Firefox 3 alpha7
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a5pre) Gecko/20070516 Minefield/3.0a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a5pre) Gecko/20070516 Minefield/3.0a5pre

Replace module registration code in nsMicrosummaryService.js component with the
Components.utils.import("rel:XPCOMUtils.jsm") and generateNSGetModule.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Reporter)

Updated

11 years ago
Blocks: 381189

Updated

11 years ago
Blocks: 381191
Created attachment 269355 [details] [diff] [review]
Patch
Assignee: nobody → ryan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #269355 - Flags: review?(myk)
Created attachment 269382 [details] [diff] [review]
Patch v2

Missed a few places we can switch over to generateQI - never patch while half asleep! ;)
Attachment #269355 - Attachment is obsolete: true
Attachment #269382 - Flags: review?(myk)
Attachment #269355 - Flags: review?(myk)
Comment on attachment 269382 [details] [diff] [review]
Patch v2

Looks great.  My only question is whether the argument to generateModule needs to be a named top-level variable, i.e.:

var components = [MicrosummaryService];
function NSGetModule(compMgr, fileSpec) {
  return XPCOMUtils.generateModule(components);
}

That's how XPCOMUtils.jsm suggests doing it in the documentation at the top of that file:

 * 2. Create an array of component constructors (like the one
 * created in step 1):
 *  var components = [MyComponent];
 *
 * 3. Define the NSGetModule entry point:
 *  function NSGetModule(compMgr, fileSpec) {
 *    // components is the array created in step 2.
 *    return XPCOMUtils.generateModule(components);
 *  }

And I know there are some tricky issues with leaks in XPCOM component registration.  I'm not sure if that's why the docs suggest the more complicated approach, but sayrer should know, so I've cc:ed him on this bug.
Attachment #269382 - Flags: review?(myk) → review+
Comment on attachment 269382 [details] [diff] [review]
Patch v2

Asking sayrer for additional review to make sure it's ok to use an array literal as an argument to the generateModule method.
Attachment #269382 - Flags: review?(sayrer)
Just on first principles (yeah, we have those, even with fragile XPCOM shutdown), it can't matter whether you pass an array initialiser to generateModule directly, or name it with a var and pass that. Either generateModule gets elements from the array and keeps strong refs to those things, or it keeps the array -- or both, depending on certain conditions, as it appears from a glance at the code. But in any event, a strong ref is all you can use in JS, and it would be a bug in generateModule if it needed its caller to retain a reference typed parameter in a global variable.

/be
Comment on attachment 269382 [details] [diff] [review]
Patch v2

Brendan, thanks for the info.  In that case, I don't need sayrer's additional review on this after all.
Attachment #269382 - Flags: review?(sayrer)
mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js 	1.65
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M7
Depends on: 411186
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.