446288 - (minor) Clean up <messengerdnd.js>

Status: ASSIGNED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

While working on bug 370904,
I noticed some clean up to do / look into.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

I'm using SM, but this file is shared with TB.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) |var ...| cleanup ++

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

*Remove lots of superfluous/useless vars.
*Add a few.
*Do a few rewrite.

NB: This cleans up the way for (2+) following patches.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 330686 [details] [diff] [review]
(Av1) |var ...| cleanup ++

>-    var targetUri = targetResource.Value;
>     var targetFolder = targetResource.QueryInterface(Components.interfaces.nsIMsgFolder);
>     var targetServer = targetFolder.server;
>-    var sourceServer;
>-    var sourceResource;
>+    var sourceServer = null;
>+    var sourceResource = null;
>+    var sourceFolder;
>    
>-    for (var i = 0; i < dragSession.numDropItems; i++)
>+    const numDropItems = dragSession.numDropItems;
>+    var dataFlavor, dataObj, len, sourceUri;

I see no advantage to pulling these out of the loop.

>+    for (var i = 0; i < numDropItems; ++i)
>     {
>         dragSession.getData (trans, i);
>-        var dataObj = new Object();
>-        var dataFlavor = new Object();
>-        var len = new Object();
>+        len = dataObj = dataFlavor = {};

These are not equivalent.

>-        if (dataFlavor.value == "text/x-moz-message")
>+        dataFlavor = dataFlavor.value;
>+        if (dataFlavor == "text/x-moz-message")
>         {

I don't see any advantage to doing this replacement. dataFlavor is a javascript object, accessing it is not going to take lots of time.

>-            sourceResource = null;

So now you've taken this outside the loop, if it gets used once round the loop the first time, and not the second time, then it won't be null the second time. I'm not sure if we'd ever get to that case, but it could lead to errors from future modifications because its no longer being set to null.


>+          sourceFolder = sourceResource.QueryInterface(Components.interfaces.nsIMsgFolder);
> 
>-          // don't allow dragging news folder (newsgroup) to server folder
>-          if (targetFolder.isServer)
>+          // don't allow dragging news folder (newsgroup) to other account
>+          // or server folder.
>+          if (targetFolder.rootFolder != sourceFolder.rootFolder ||
>+              targetFolder.isServer)
>+          {
>             return false;
>+          }

Please don't put braces around one line statements.

>           // don't allow dragging news folder (newsgroup) to before/after itself
>           index += orientation;
>           if (index < folderTree.view.rowCount) {	
>-            targetResource = GetFolderResource(folderTree, index);
>-
>-            if (targetResource == sourceResource)	
>+            if (GetFolderResource(folderTree, index) == sourceResource)
>               return false;

I don't see why you're not using the targetResource that's set up at the start.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> (From update of attachment 330686 [details] [diff] [review])
> >+    var dataFlavor, dataObj, len, sourceUri;
> 
> I see no advantage to pulling these out of the loop.

I assumed it might save some "|var| processing".
Does it not ?

> >-        var dataObj = new Object();
> >-        var dataFlavor = new Object();
> >-        var len = new Object();
> >+        len = dataObj = dataFlavor = {};
> 
> These are not equivalent.

I found both values when I did an MXR search,
and assumed the latter was "equivalent but simpler".
Is it not ?

> >+        dataFlavor = dataFlavor.value;
> 
> I don't see any advantage to doing this replacement. dataFlavor is a javascript
> object, accessing it is not going to take lots of time.

Second thought ... agreed.

> >-            sourceResource = null;
> 
> So now you've taken this outside the loop, if it gets used once round the loop
> the first time, and not the second time, then it won't be null the second time.

(line removal part)
Actually, in the current code, this assignment is useless.
Anyway, this case can't loop, as there is a |break;| at the end of its block.

> I'm not sure if we'd ever get to that case, but it could lead to errors from
> future modifications because its no longer being set to null.

(assignment move part)
And my next patch will need the var to be initialized.

> >+          if (targetFolder.rootFolder != sourceFolder.rootFolder ||
> >+              targetFolder.isServer)
> >+          {
> >             return false;
> >+          }
> 
> Please don't put braces around one line statements.

It was only to make it clearer after the multi-line if, but as you wish.

> >           index += orientation;
> >           if (index < folderTree.view.rowCount) {	
> >+            if (GetFolderResource(folderTree, index) == sourceResource)
> >               return false;
> 
> I don't see why you're not using the targetResource that's set up at the start.

Oh, I too wondered about this code the first time I read it:
the answer is that |index| has just got a new value ;->

Comment 5

[Serge Gautherie (:sgautherie)]

Let's complete the cleanup, before looking into the exception bugs.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 330686 [details] [diff] [review]
(Av1) |var ...| cleanup ++

Reminder: Bug 356295 bitrotted the |BeginDragTree()| part...

Comment 7

[Wayne Mery (:wsmwk)]

Serge, possible to drive this and bug 446292 in for TB3?

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)

Mark, can you answer my comment 4 questions?


> > >-        var dataObj = new Object();
> > >-        var dataFlavor = new Object();
> > >-        var len = new Object();
> > >+        len = dataObj = dataFlavor = {};
> > 
> > These are not equivalent.
> 
> I found both values when I did an MXR search,
> and assumed the latter was "equivalent but simpler".
> Is it not ?

Ah, I guess I can use |{}| but I still need 3 distinct initializations, right?

Comment 9

[Mark Banner (:standard8)]

(In reply to Serge Gautherie (:sgautherie) from comment #8)
> (In reply to comment #4)
> 
> Mark, can you answer my comment 4 questions?
> 
> 
> > > >-        var dataObj = new Object();
> > > >-        var dataFlavor = new Object();
> > > >-        var len = new Object();
> > > >+        len = dataObj = dataFlavor = {};
> > > 
> > > These are not equivalent.
> > 
> > I found both values when I did an MXR search,
> > and assumed the latter was "equivalent but simpler".
> > Is it not ?
> 
> Ah, I guess I can use |{}| but I still need 3 distinct initializations,
> right?

Yes, because otherwise you'd end up with all of them trying to use the same object.