308283 - stateFlags is a bit field and console.views.source2.progressListener.onStateChange should use & instead of == to check for something like STOP

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, enhancement)

Description

[timeless]

the code currently compares explicitly == against 0xc0010 (state_is_window | 
state_is_network | state_stop), i think it should probably be using & instead 
of ==.

Comment 1

[timeless]

Attachment: use &

Comment 2

[James Ross]

Comment on attachment 196061 [details] [diff] [review]
use &

r=silver

(not sure I can review Venkman, but it looks good!)

It might be nice to actually use the hex version of the number to make it
clearer, though.

Comment 3

[:Gijs (he/him)]

Timeless, could you check this in?

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 196061 [details] [diff] [review]
use &

>-    else if (stateFlags == 786448)
>+    else if (stateFlags & 786448)

Um, can we not use a magic number and use the actual constants on nsIWebProgressListener?

That number looks like (STATE_STOP | STATE_IS_NETWORK | STATE_IS_WINDOW)

Comment 5

[:Gijs (he/him)]

Attachment: Patch

Like this?

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 296138 [details] [diff] [review]
Patch

>+    const WINDOW = nsIWebProgressListener.STATE_IS_WINDOW;
>+    const NETWORK = nsIWebProgressListener.STATE_IS_NETWORK;

Nit, can you reverse these so they remain in the order they appear in the idl file?


>-    else if (stateFlags == 786448)
>+    else if (stateFlags & (STOP | WINDOW | NETWORK))


I don't think this does what you expect when stateFlags has one or two of the bits set, but not all three.  You probably want to add a bitmask variable, and make sure that the result of the bitwise AND operation is equal to the mask.  Sort of like http://mxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsAppShellService.cpp#313

Comment 7

[:Gijs (he/him)]

Attachment: Better Patch

Like this? :-)

Comment 8

[:Gijs (he/him)]

Checking in mozilla/extensions/venkman/resources/content/venkman-views.js;
/cvsroot/mozilla/extensions/venkman/resources/content/venkman-views.js,v  <--  venkman-views.js
new revision: 1.42; previous revision: 1.41
done