Closed
Bug 664697
Opened 13 years ago
Closed 10 years ago
Convert newsblog util.js into a module
Categories
(MailNews Core :: Feed Reader, defect)
MailNews Core
Feed Reader
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 29.0
People
(Reporter: iannbugzilla, Assigned: alta88)
Details
Attachments
(1 file, 4 obsolete files)
21.35 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
Spun off from comment in bug 664612: [I wonder whether utils.js should be turned into a module...]
Assignee: nobody → alta88
Attachment #829489 -
Flags: review?(mbanner)
Attachment #829489 -
Flags: review?(iann_bugzilla)
export extra symbols and get rid of scriptloader in newsblog.js.
Attachment #829489 -
Attachment is obsolete: true
Attachment #829489 -
Flags: review?(mbanner)
Attachment #829489 -
Flags: review?(iann_bugzilla)
Attachment #829753 -
Flags: review?(mbanner)
Attachment #829753 -
Flags: review?(iann_bugzilla)
Comment 4•10 years ago
|
||
Comment on attachment 829753 [details] [diff] [review] feedUtilsModule.patch Review of attachment 829753 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in looking at this. This all looks fine to me, however feed-subscriptions needs two lines adding, as I have done here: https://hg.mozilla.org/try-comm-central/rev/0a1e4a21be07 I've pushed the complete patch to try server so that MozMill tests run to ensure the changes haven't broken anything in the mail window that rely on Cc/Cr etc. https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=0a1e4a21be07 Assuming that passes, then r=Standard for the mailnews/ and mail/ parts.
Attachment #829753 -
Flags: review?(mbanner) → review+
Comment 5•10 years ago
|
||
(In reply to Mark Banner (:standard8) (away until 3 Jan) from comment #4) > Assuming that passes, then r=Standard for the mailnews/ and mail/ parts. (with the additional change included)
updated; try looks fine.
Attachment #829753 -
Attachment is obsolete: true
Attachment #829753 -
Flags: review?(iann_bugzilla)
Attachment #8354934 -
Flags: review?(iann_bugzilla)
Attachment #8354934 -
Flags: review?(iann_bugzilla) → review?(philip.chee)
Comment 7•10 years ago
|
||
Comment on attachment 8354934 [details] [diff] [review] feedUtilsModule.patch > +++ b/mail/base/content/mailWindowOverlay.js > +Components.utils.import("resource:///modules/FeedUtils.jsm"); > Components.utils.import("resource:///modules/gloda/dbview.js"); > Components.utils.import("resource:///modules/mailServices.js"); > Components.utils.import("resource:///modules/MailUtils.js"); > Components.utils.import("resource://gre/modules/Services.jsm"); > Components.utils.import("resource://gre/modules/PluralForm.jsm"); I noticed that this file imports Services.jsm and MailUtils.js twice. While you're changing this file please delete the duplicate lines further down. > +++ b/mailnews/extensions/newsblog/content/feed-subscriptions.js > +Components.utils.import("resource:///modules/gloda/log4moz.js"); > +Components.utils.import("resource:///modules/FeedUtils.jsm"); > Components.utils.import("resource:///modules/mailServices.js"); Sort alphabetically within this block (FeedUtils, gloda, mailServices, > +Components.utils.import("resource://gre/modules/FileUtils.jsm"); > Components.utils.import("resource://gre/modules/PluralForm.jsm"); > +Components.utils.import("resource://gre/modules/Services.jsm"); Same here. > +var Cc = Components.classes; > +var Ci = Components.interfaces; Doesn't var {classes: Cc, interfaces: Ci} = Components; work here? > +++ b/mailnews/extensions/newsblog/js/newsblog.js > -Components.utils.import("resource://gre/modules/Services.jsm"); > -Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; [The following line(s) is a remark and not a review nit] [[ I notice that in this patch you sometimes you use var {} and here you use const {} instead. I suggest a bit of consistency and stick to "var". ]] > +XPCOMUtils.defineLazyGetter(this, "FeedUtils", function() { > + Components.utils.import("resource:///modules/FeedUtils.jsm"); > + return FeedUtils; > +}); Any reason you aren't using |XPCOMUtils.defineLazyModuleGetter| instead? > +++ b/suite/mailnews/mailWindowOverlay.js > if (wintype == "mail:3pane") { > // Get quickmode per feed pref from feeds.rdf > var quickMode, targetRes; > if (!("FeedUtils" in window)) > - Services.scriptloader.loadSubScript("chrome://messenger-newsblog/content/utils.js"); > + Services.scriptloader.loadSubScript("resource:///modules/FeedUtils.jsm"); Firstly, this should be Components.utils.import("resource:///modules/FeedUtils.jsm"). Secondly, this should be at the top together with all the other Cu.import lines like in the TB version of mailWindowOverlay.js. (Note Suite style is to Cu.import /gre/ modules first then application specific javascript modules). Also please remove the check for "FeedUtils" (not needed any more). > try > { > var targetRes = FeedUtils.getParentTargetForChildResource( targetRes has already been defined a few lines up so this should just be: targetRes = FeedUtils.getParentTargetForChildResource( > gFolderDisplay.displayedFolder.URI, > FeedUtils.FZ_QUICKMODE, > gFolderDisplay.displayedFolder.server); > } > catch (ex) {};
Attachment #8354934 -
Flags: review?(philip.chee)
updated.
Attachment #8354934 -
Attachment is obsolete: true
Attachment #8355796 -
Flags: review?(philip.chee)
Comment 9•10 years ago
|
||
Comment on attachment 8355796 [details] [diff] [review] feedUtilsModule.patch r= me with the following change: >> +XPCOMUtils.defineLazyGetter(this, "FeedUtils", function() { >> + Components.utils.import("resource:///modules/FeedUtils.jsm"); >> + return FeedUtils; >> +}); > Any reason you aren't using |XPCOMUtils.defineLazyModuleGetter| instead? Now that I'm more awake I realized that I had misled you. TL;DR: get rid of the defineLazyGetter/defineLazyModuleGetter and move the Components.utils.import("resource:///modules/FeedUtils.jsm") to the top together with the other imports. Longer explanation. The FeedUtils.jsm exports four symbols of which newsblog.js needs "Feed" and "FeedUtils". defineLazy[Module]Getter exports only one symbol. The only reason your code works is that it isn't doing what you think. Since you didn't supply a second parameter to Components.utils.import(), the module and all its exports were loaded into the global scope of the component. An example of the proper lazy getter syntax is: XPCOMUtils.defineLazyGetter(this, "DynamicResizeWatcher", function() { let tmp = {}; Cu.import("resource:///modules/Social.jsm", tmp); return tmp.DynamicResizeWatcher; }); BTW defineLazyModuleGetter is just a wrapper that does the above. You could use defineLazyGetter() to export each of the four items one at a time. However since components are loaded lazily, lazy getters don't buy you much if any. So just drop the lazygetter and do the normal Cu.import()
Attachment #8355796 -
Flags: review?(philip.chee) → review+
Comment 10•10 years ago
|
||
p.s. [grammar nit] Your commit message should be something like: "Bug 664697 - Turn newsblog util.js into a module"
Updated•10 years ago
|
Summary: Turn newsblog util.js in module → Convert newsblog util.js into a module
Assignee | ||
Comment 11•10 years ago
|
||
for checkin.
Attachment #8355796 -
Attachment is obsolete: true
Attachment #8356086 -
Flags: review+
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/4b1338309c26
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in
before you can comment on or make changes to this bug.
Description
•