485407 - Assignment to undeclared variable dragAfter

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Phil Ringnalda (:philor)]

When bug 251627 added tracking whether the window is ltr or rtl to decide which side to drop on, it failed to declare var dragAfter, making it an unintentional global.

Comment 1

[Phil Ringnalda (:philor)]

And despite the microscopic scrutiny it went through, that seems to have made it into the SeaMonkey fork, too.

Comment 2

[Philip Chee]

It's a temporary fork Phil, so we forked their bugs as well. :P

Comment 3

[(no longer active)]

Attachment: No-brainer :-)

Comment 4

[Philip Chee]

Attachment: Patch v1.0: consistent code style.

In other parts of customizeToolbar.js we use this style:

var value = direction == "ltr"? "left" : "right";

This version uses the more compact coding style:

var dragAfter = direction == "ltr" ? aEvent.clientX > dropTargetCenter : aEvent.clientX < dropTargetCenter;

Comment 5

[Philip Chee]

Comment on attachment 369668 [details] [diff] [review]
Patch v1.0: consistent code style.

What do you think Ehsan?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 369668 [details] [diff] [review]
Patch v1.0: consistent code style.

I think this is harder to read and too long of a line :)

Comment 7

[Philip Chee]

Attachment: Patch v1.0-comm comm-central version.

Comment 8

[Philip Chee]

> I think this is harder to read and too long of a line :)
So wrap on multiple lines?

var dragAfter = direction == "ltr" ?
                aEvent.clientX > dropTargetCenter :
                aEvent.clientX < dropTargetCenter;

Comment 9

[[e-mail]]

Comment on attachment 369670 [details] [diff] [review]
Patch v1.0-comm comm-central version.

      var dragAfter = direction == "ltr" ? aEvent.clientX > dropTargetCenter :
                                           aEvent.clientX < dropTargetCenter;

Comment 10

[Phil Ringnalda (:philor)]

Is that your entry in the "Make A Simple JavaScript Assignment Easy To Misunderstand" contest? If so, you should probably sneak in use of the comma operator, that always helps obfuscate.

Comment 11

[Philip Chee]

Attachment: [comm-central checkin-needed] Patch v1.1-comm comm-central version.

Carrying forward r+/sr+ for suite/comm-central

> (From update of attachment 369670 [details] [diff] [review])
>      var dragAfter = direction == "ltr" ? aEvent.clientX > dropTargetCenter :
>                                           aEvent.clientX < dropTargetCenter;
Nit Fixed.

Comment 12

[(no longer active)]

(In reply to comment #5)
> (From update of attachment 369668 [details] [diff] [review])
> What do you think Ehsan?

FWIW, I find the use of the conditional test in non-trivial assignments confusing.  Indentation can be used to compensate for the problem partly, but I don't get why one would do that in the first place.  :-)  (And as far as the coding conventions in the file are concerned, I'd file a bug to fix any other such assignments instead of adding more of them...)

Comment 13

[(no longer active)]

Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/68b8e8a9c0c7>.  Keeping open until it's landed on comm-central as well.

Comment 14

[Philip Chee]

Comment on attachment 369700 [details] [diff] [review]
[comm-central checkin-needed] Patch v1.1-comm comm-central version.

> Keeping open until it's landed on comm-central as well.
I don't have checkin privileges. Setting keywords.

Comment 15

[Frank Wein [:mcsmurf]]

Pushed to comm-central, changeset 266595448dd8.