1140712 - Clean up "SyntaxError: test for equality (==) mistyped as assignment (=)" in messenger/content/addressbook/abResultsPane.js

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

I get 3 warnings like this when opening the main AB window:
Warning: SyntaxError: test for equality (==) mistyped as assignment (=)?
Source File: chrome://messenger/content/addressbook/abResultsPane.js
Line: 91, Column: 61
Source Code:
   if (element = document.getElementById("CardViewOuterBox"))

This code was introduced in bug 170270.

While the code is correct and we may not agree with this warning (it is a false positive), warnings like this are known to break tests (at least xpcshell). So better make the JS engine happy.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[:aceman]

Attachment: patch v2

It looks to me it can be simplified even more. It worked in my tests. Suyash, can you check if this still does what you intended?

Comment 3

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8574295 [details] [diff] [review]
patch v2

Review of attachment 8574295 [details] [diff] [review]:
-----------------------------------------------------------------

Ya, this does what I intend to do :)

Thanks.

::: mailnews/addrbook/content/abResultsPane.js
@@ +82,5 @@
>    UpdateSortIndicators(actualSortColumn, sortDirection);
>  
>    // If the selected address book is LDAP and the search box is empty,
>    // inform the user of the empty results pane.
> +  let elementIDs = ["abResultsTree", "CardViewOuterBox", "blankResultsPaneMessageBox"];

Is it 80chars width? Looks wrong here on splinter.

Comment 4

[:aceman]

Attachment: patch v2.1

OK, fixed long lines.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8574299 [details] [diff] [review]
patch v2.1

Review of attachment 8574299 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry but I think the readability if this is just not good. If you want to get rid of the strict warnings, just assign three variables before the if-else and do the tests later.

Comment 6

[:aceman]

Attachment: patch v3

Yeah, it is a bit over-engineered for only 3 elements :) So do you mean it like this?

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8574436 [details] [diff] [review]
patch v3

Review of attachment 8574436 [details] [diff] [review]:
-----------------------------------------------------------------

Yes! r=mkmelin

Comment 8

[Magnus Melin [:mkmelin]]

Though it could be clearer to call the variables e.g. abResultsTree and CardViewOuterBox instead of box1, box2

Comment 9

[:aceman]

Attachment: patch v3.1

Ok.

Comment 10

[:aceman]

I don't think this needs to go into TB38 if it does not cause any problems there.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/f06d8b2b5f9d

Comment 12

[Kent James (:rkent)]

(In reply to :aceman from comment #10)
> I don't think this needs to go into TB38 if it does not cause any problems
> there.

I would like to eliminate extraneous error messages in TB 38.

Comment 13

[Philip Chee]

Isn't the correct thing to do is to file a bug to remove bogus strict warnings in the jsengine?

Comment 14

[Kent James (:rkent)]

(In reply to Philip Chee from comment #13)
> Isn't the correct thing to do is to file a bug to remove bogus strict
> warnings in the jsengine?

I generally assume, perhaps falsely, that the js people will not make a decision like that based on the needs of Thunderbird or Seamonkey. Even if they did, the chances of getting that into gecko38 are extremely low.

Comment 15

[:aceman]

I am sure Philip questioned the usefulness of the message for all products and the JS core. It is just a question to the coder (even with question mark) but is marked as "TypeError". So the message is maybe too strict, wrongly classified. And if it flags code incorrectly, what can we do? Only change the code to please the checker. Maybe it should have been just at "information" level instead of "warning" level. But I don't think we need to care about it too much for now.

Comment 16

[Kent James (:rkent)]

Comment on attachment 8574450 [details] [diff] [review]
patch v3.1

[Triage Comment]

http://hg.mozilla.org/releases/comm-aurora/rev/6832d62c544f