Last Comment Bug 732811 - convert mailnews/news/content/ to MailServices.js
: convert mailnews/news/content/ to MailServices.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: NNTP (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 720358
  Show dependency treegraph
 
Reported: 2012-03-04 08:53 PST by :aceman
Modified: 2012-03-24 14:58 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.74 KB, patch)
2012-03-04 09:13 PST, :aceman
Pidgeot18: review+
iann_bugzilla: feedback+
Details | Diff | Splinter Review
patch v2 (5.53 KB, patch)
2012-03-05 11:10 PST, :aceman
no flags Details | Diff | Splinter Review

Description :aceman 2012-03-04 08:53:03 PST
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.
Comment 1 :aceman 2012-03-04 09:13:16 PST
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.
Comment 2 Ian Neal 2012-03-04 14:17:21 PST
(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 Ian Neal 2012-03-04 14:29:25 PST
Comment on attachment 602734 [details] [diff] [review]
patch

I would say on the right track, but the JS in this file needs a rework.
Comment 4 Joshua Cranmer [:jcranmer] 2012-03-04 14:45:24 PST
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
Comment 5 Joshua Cranmer [:jcranmer] 2012-03-04 14:51:50 PST
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...
Comment 6 :aceman 2012-03-04 23:59:10 PST
(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 Ludovic Hirlimann [:Usul] 2012-03-05 02:34:51 PST
http://beaufour.dk/jst-review/ and probably standard8
Comment 8 :aceman 2012-03-05 11:10:55 PST
Created attachment 602975 [details] [diff] [review]
patch v2

Sorry, I can't rework the logical problems in this bug.
Comment 9 Mark Banner (:standard8) 2012-03-22 15:48:00 PDT
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.
Comment 10 :aceman 2012-03-23 01:00:18 PDT
Well, I understood him he specifically disclaimed he is the right person to review JS :)
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-03-24 14:58:07 PDT
http://hg.mozilla.org/comm-central/rev/e08666b410ad

Note You need to log in before you can comment on or make changes to this bug.