415990 - js warning: while(calGoItem = calGoPopupMenu.firstChild) {...

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Daniel Boelzle [:dbo]]

Warning: test for equality (==) mistyped as assignment (=)?
Source File: chrome://lightning/content/messenger-overlay-sidebar.js
Line: 619, Column: 47
Source Code:
    while(calGoItem = calGoPopupMenu.firstChild) {

Comment 1

[Daniel Boelzle [:dbo]]

To me that looks like an endless loop. Maybe I miss some some side effect (that I'd like to see documented then).

Comment 2

[Sven Giermann]

I think this should remove the warning:

-    while(calGoItem = calGoPopupMenu.firstChild) {
+    while((calGoItem = calGoPopupMenu.firstChild) != null) {

Comment 3

[Philipp Kewisch [:Fallen][☁️📆]]

Just to remove the warning, it is sufficient to do:

while((calGoItem = calGoPopupMenu.firstChild))

but we should investigate why this is an endless loop.

Comment 4

[Sven Giermann]

Attachment: Cleanup patch for bug #413620

(In reply to comment #3)
> but we should investigate why this is an endless loop.

No, this is no endless loop - but since already Berend and now Daniel wondered about it, I should add an comment.

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 302114 [details] [diff] [review]
Cleanup patch for bug #413620

>-    while(calGoItem = calGoPopupMenu.firstChild) {
>+    while((calGoItem = calGoPopupMenu.firstChild) != null) {
>+        // appendChild actually moves the items from the cloned Node to Thunderbird's menu
>         tbGoPopupMenu.appendChild(calGoItem);
>     }


I see, interesting behavior but not very intuitive. To make this more intuitive, would something like:

while ((calgoItem = calgoPopupMenu.firstChild)) {
  tbGoPopupMenu.appendChild(calGoPopupMenu.removeChild(calGoItem));
}

make sense?

I'm fine with both though, its up to you.

r=philipp with a decision on which method you prefer.

Comment 6

[Sven Giermann]

(In reply to comment #5)
> while ((calgoItem = calgoPopupMenu.firstChild)) {
>   tbGoPopupMenu.appendChild(calGoPopupMenu.removeChild(calGoItem));
> }

I'm fine with both, too.
Maybe it's a question of runtime, but I don't want to step too deep into the code of 'appendChild' and it's not too dependent for that - so you're probably right: this would be more intuitive understood on the first look.

Since this bug is not assigned to me, I'm unable to set the keyword 'checkin-needed', but please check-in your version with attention to the the spelling:

-    while(calGoItem = calGoPopupMenu.firstChild) {
-        tbGoPopupMenu.appendChild(calGoItem);
+    while((calGoItem = calGoPopupMenu.firstChild)) {
+        tbGoPopupMenu.appendChild(calGoPopupMenu.removeChild(calGoItem));
     }

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED