convert mailnews/news/content/ to MailServices.js

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Networking: NNTP
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 14.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.53 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #602734 - Flags: review?(Pidgeot18)
Attachment #602734 - Flags: feedback?(iann_bugzilla)

Comment 2

5 years ago
(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 3

5 years ago
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...

Updated

5 years ago
Attachment #602734 - Flags: feedback?(iann_bugzilla) → feedback+
(Assignee)

Comment 6

5 years ago
(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?
http://beaufour.dk/jst-review/ and probably standard8
(Assignee)

Comment 8

5 years ago
Created attachment 602975 [details] [diff] [review]
patch v2

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)
(Assignee)

Comment 10

5 years ago
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
Last Resolved: 5 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.