Closed Bug 664697 Opened 13 years ago Closed 10 years ago

Convert newsblog util.js into a module

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: iannbugzilla, Assigned: alta88)

Details

Attachments

(1 file, 4 obsolete files)

Spun off from comment in bug 664612:
[I wonder whether utils.js should be turned into a module...]
Attached patch feedUtilsModule.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #829489 - Flags: review?(mbanner)
Attachment #829489 - Flags: review?(iann_bugzilla)
Attached patch feedUtilsModule.patch (obsolete) — Splinter Review
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)
ping?
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+
(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)
Attached patch feedUtilsModule.patch (obsolete) — Splinter Review
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 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)
Attached patch feedUtilsModule.patch (obsolete) — Splinter Review
updated.
Attachment #8354934 - Attachment is obsolete: true
Attachment #8355796 - Flags: review?(philip.chee)
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+
p.s. [grammar nit] Your commit message should be something like:
"Bug 664697 - Turn newsblog util.js into a module"
Summary: Turn newsblog util.js in module → Convert newsblog util.js into a module
for checkin.
Attachment #8355796 - Attachment is obsolete: true
Attachment #8356086 - Flags: review+
Keywords: checkin-needed
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.