Closed Bug 732811 Opened 12 years ago Closed 12 years ago

convert mailnews/news/content/ to MailServices.js

Categories

(MailNews Core :: Networking: NNTP, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

5.53 KB, patch
Details | Diff | Splinter Review
downloadheaders.js:var prefs = Components.classes['@mozilla.org/preferences-service;1'].getService();
downloadheaders.js:        var accountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);

The prefs variable seems unused so I'll remove it entirely.
Attached patch patch (obsolete) — Splinter Review
Also removes some other unused variables.

There is some strange codeflow:
if ("arguments" in window && window.arguments[0]) is not true then nntpServer is undefined. But it is still dereferenced later in the function and also in others. That should probably be reworked.
Attachment #602734 - Flags: review?(Pidgeot18)
Attachment #602734 - Flags: feedback?(iann_bugzilla)
(In reply to :aceman from comment #1)
> Created attachment 602734 [details] [diff] [review]
> patch
> 
> Also removes some other unused variables.
> 
> There is some strange codeflow:
> if ("arguments" in window && window.arguments[0]) is not true then
> nntpServer is undefined. But it is still dereferenced later in the function
> and also in others. That should probably be reworked.

Agreed. Is there any chance that an argument would not be passed from http://mxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPNewsgroupList.cpp#436 ?
You also have references to "args" which is defined within the if
What would an early return false do if the argument does not exist?

function setupDownloadUI could probably make used of the global definitions for the elements but doesn't.
Comment on attachment 602734 [details] [diff] [review]
patch

I would say on the right track, but the JS in this file needs a rework.
Attachment #602734 - Flags: feedback?(iann_bugzilla) → feedback+
Comment on attachment 602734 [details] [diff] [review]
patch

Review of attachment 602734 [details] [diff] [review]:
-----------------------------------------------------------------

r+, with nits:

::: mailnews/news/content/downloadheaders.js
@@ +46,5 @@
>  var args = null;
>  
>  function OnLoad()
>  {
> +    let NewsBundle = document.getElementById("bundle_news");

Nit: should be newsBundle, not NewsBundle

@@ +55,5 @@
>          args.hitOK = false; /* by default, act like the user hit cancel */
>          args.downloadAll = false; /* by default, act like the user did not select download all */
>  
> +        nntpServer = MailServices.accounts.getIncomingServer(args.serverKey)
> +          .QueryInterface(Components.interfaces.nsINntpIncomingServer);

Nit: line up the . for QueryInterface
Attachment #602734 - Flags: review?(Pidgeot18)
Attachment #602734 - Flags: review+
Attachment #602734 - Flags: feedback?(iann_bugzilla)
Attachment #602734 - Flags: feedback+
Side note:
I'd rather see more use of 2-space indentation than 4-space; I'm also not the best person to ask for review if you're just doing JS code cleanup...
Attachment #602734 - Flags: feedback?(iann_bugzilla) → feedback+
(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> @@ +55,5 @@
> >          args.hitOK = false; /* by default, act like the user hit cancel */
> >          args.downloadAll = false; /* by default, act like the user did not select download all */
> >  
> > +        nntpServer = MailServices.accounts.getIncomingServer(args.serverKey)
> > +          .QueryInterface(Components.interfaces.nsINntpIncomingServer);
> 
> Nit: line up the . for QueryInterface

I think this is a valid indentation. You probably mean to move it under .getIncomingServer, but that would cause it to take another line.

So who is good for review?
Attached patch patch v2Splinter Review
Sorry, I can't rework the logical problems in this bug.
Attachment #602734 - Attachment is obsolete: true
Attachment #602975 - Flags: review?(mbanner)
Comment on attachment 602975 [details] [diff] [review]
patch v2

Review of attachment 602975 [details] [diff] [review]:
-----------------------------------------------------------------

As Joshua's already reviewed this, I don't need to.
Attachment #602975 - Flags: review?(mbanner)
Well, I understood him he specifically disclaimed he is the right person to review JS :)
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/e08666b410ad
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: