that makes the following IDL return -1 (overflow) instead of the actual result. ] interface NodeFilter { ... // Constants for whatToShow const unsigned long SHOW_ALL = 0xFFFFFFFF; ... } Generated code from this IDL looks like: JSValue jsNodeFilterSHOW_ALL(ExecState* exec, JSValue, PropertyName) { UNUSED_PARAM(exec); return jsNumber(static_cast<int>(0xFFFFFFFF)); <--- OVERFLOW } Firefox, for instance, returns 4294967295 as expected.
We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good. Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed. I suggest we make tests for all these enum values. They would be very easy to write: shouldBe("NodeFilter.SHOW_ALL", "0xFFFFFFFF");
(In reply to comment #1) > We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good. It might be important to static_cast it, so the proper jsNumber constructor can be called? > Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed. We already here this information: - push(@implContent, " return JSValue::encode(jsNumber(static_cast<int>(" . $constant->value . ")));\n"); + push(@implContent, " return JSValue::encode(jsNumber(static_cast<" . $constant->type . ">(" . $constant->value . ")));\n"); > I suggest we make tests for all these enum values. They would be very easy to write: > > shouldBe("NodeFilter.SHOW_ALL", "0xFFFFFFFF"); Sounds good. There are actually compile-times tests dumped into the generated C++ code, but an IDL might have a "DoNotCheckConstants" extended attribute entry, that bypasses it.
(In reply to comment #2) > (In reply to comment #1) > > We should try just removing "static_cast<int>" from the generated code. I don’t think that cast is doing us any good. > > It might be important to static_cast it, so the proper jsNumber constructor can be called? No, I don’t believe it is important. Generally speaking, the jsNumber constructor should correctly convert a number passed in of most any C++ type. It’s overloaded for the various types. But of course we need to test to make sure it works. > > Actually parsing the type of the constant (“unsigned long”) would be OK, but likely is not needed. > > We already here this information: > > - push(@implContent, " return JSValue::encode(jsNumber(static_cast<int>(" . $constant->value . ")));\n"); > + push(@implContent, " return JSValue::encode(jsNumber(static_cast<" . $constant->type . ">(" . $constant->value . ")));\n"); If $constant->type is going to be “unsigned long” then the code above is going to be incorrect. The type “unsigned long” in IDL needs to turn into “unsigned” in C++. That’s one of the reasons I made my suggestion above. Casting to unsigned long will convert to 64-bit on a 64-bit system, which is something we don’t want to do. > There are actually compile-times tests dumped into the generated C++ code, but an IDL might have a "DoNotCheckConstants" extended attribute entry, that bypasses it. Tests in C++ code would not be sufficient. We need to test this in JavaScript, since it’s a JavaScript binding we are testing.
Created attachment 227885 [details] patch - for EWS
Comment on attachment 227885 [details] patch - for EWS Attachment 227885 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6078183057129472 New failing tests: fast/dom/constants.html
Created attachment 227893 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 227885 [details] patch - for EWS Attachment 227885 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5987782585483264 New failing tests: fast/dom/constants.html
Created attachment 227897 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 227885 [details] patch - for EWS Attachment 227885 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4716566755147776 New failing tests: fast/dom/constants.html
Created attachment 227899 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 227885 [details] patch - for EWS Attachment 227885 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4573937333698560 New failing tests: fast/dom/constants.html
Created attachment 227901 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 227918 [details] patch - for review
Comment on attachment 227918 [details] patch - for review Attachment 227918 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5823585985757184 New failing tests: media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html
Created attachment 227927 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Darin, could you review the patch?
Comment on attachment 227918 [details] patch - for review View in context: https://bugs.webkit.org/attachment.cgi?id=227918&action=review Fix is good, but LayoutTests part of patch is wrong. > LayoutTests/fast/dom/constants-expected.txt:30 > -PASS: nodeFilter.SHOW_ALL should be -1 and is. > +FAIL: nodeFilter.SHOW_ALL should be -1 but instead is 4294967295. This is wrong. We should fix the fast/dom/constants.html test itself, which we now believe was expecting the wrong thing. We should not instead update the expected results to show failure! > Source/WebCore/ChangeLog:16 > + NodeFilter.SHOW_ALL (0xFFFFFF). Should have 8 Fs there, not 6.
Created attachment 227998 [details] patch - as per Darin's review
Created attachment 227999 [details] (actual) patch - as per Darin's review
Comment on attachment 227999 [details] (actual) patch - as per Darin's review View in context: https://bugs.webkit.org/attachment.cgi?id=227999&action=review > LayoutTests/fast/dom/constants.html:83 > + shouldBe("nodeFilter.SHOW_ALL", 4294967295); I would have written this as 0xFFFFFFFF. > LayoutTests/fast/dom/constants.html:100 > + shouldBe("window.NodeFilter.SHOW_ALL", 4294967295); Ditto.
(In reply to comment #20) > (From update of attachment 227999 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227999&action=review > > > LayoutTests/fast/dom/constants.html:83 > > + shouldBe("nodeFilter.SHOW_ALL", 4294967295); > > I would have written this as 0xFFFFFFFF. > > > LayoutTests/fast/dom/constants.html:100 > > + shouldBe("window.NodeFilter.SHOW_ALL", 4294967295); > > Ditto. Done: <https://trac.webkit.org/r166413>