Closed Bug 1140712 Opened 5 years ago Closed 5 years ago

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

Categories

(MailNews Core :: Address Book, defect, trivial)

defect
Not set
trivial

Tracking

(thunderbird38+ fixed, thunderbird39 fixed)

RESOLVED FIXED
Thunderbird 39.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8574283 - Flags: review?(mkmelin+mozilla)
Attached patch patch v2 (obsolete) — Splinter Review
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?
Attachment #8574283 - Attachment is obsolete: true
Attachment #8574283 - Flags: review?(mkmelin+mozilla)
Attachment #8574295 - Flags: review?(mkmelin+mozilla)
Attachment #8574295 - Flags: feedback?(syshagarwal)
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.
Attachment #8574295 - Flags: feedback?(syshagarwal) → feedback+
Attached patch patch v2.1 (obsolete) — Splinter Review
OK, fixed long lines.
Attachment #8574295 - Attachment is obsolete: true
Attachment #8574295 - Flags: review?(mkmelin+mozilla)
Attachment #8574299 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8574299 - Flags: review?(mkmelin+mozilla) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Yeah, it is a bit over-engineered for only 3 elements :) So do you mean it like this?
Attachment #8574299 - Attachment is obsolete: true
Attachment #8574436 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8574436 [details] [diff] [review]
patch v3

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

Yes! r=mkmelin
Attachment #8574436 - Flags: review?(mkmelin+mozilla) → review+
Though it could be clearer to call the variables e.g. abResultsTree and CardViewOuterBox instead of box1, box2
Attached patch patch v3.1Splinter Review
Ok.
Attachment #8574436 - Attachment is obsolete: true
Attachment #8574450 - Flags: review+
I don't think this needs to go into TB38 if it does not cause any problems there.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f06d8b2b5f9d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
(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.
Isn't the correct thing to do is to file a bug to remove bogus strict warnings in the jsengine?
(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.
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.
You need to log in before you can comment on or make changes to this bug.