Closed
Bug 732811
Opened 13 years ago
Closed 13 years ago
convert mailnews/news/content/ to MailServices.js
Categories
(MailNews Core :: Networking: NNTP, defect)
MailNews Core
Networking: NNTP
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 14.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
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.
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 4•13 years ago
|
||
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+
Comment 5•13 years ago
|
||
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?
Comment 7•13 years ago
|
||
http://beaufour.dk/jst-review/ and probably standard8
Sorry, I can't rework the logical problems in this bug.
Attachment #602734 -
Attachment is obsolete: true
Attachment #602975 -
Flags: review?(mbanner)
Comment 9•13 years ago
|
||
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)
Assignee | ||
Comment 10•13 years ago
|
||
Well, I understood him he specifically disclaimed he is the right person to review JS :)
Keywords: checkin-needed
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 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.
Description
•