Bug 70951

Created attachment 112605 [details] patch for V8

Comment on attachment 112605 [details] patch for V8 Can you add an attribute to a test IDL file to exercise the patch?

Comment on attachment 112605 [details] patch for V8 TestObj.idl in Source/WebCore/bindings/test. You can run the tests with run-bindings-tests.

Created attachment 112698 [details] Patch Sure thing! Thanks Adam.

Attachment 112698 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:179: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1930: jsTestObjCONST_REFLECTED is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:258: jsTestObjCONST_REFLECTED is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1401: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 112698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112698&action=review > Source/WebCore/ChangeLog:13 > + * bindings/scripts/CodeGeneratorV8.pm: > + (GenerateImplementation): check for [Reflect] and assign name accordingly. > + > + * bindings/scripts/test/CPP/WebDOMTestObj.h: Update test file. > + * bindings/scripts/test/JS/JSTestObj.cpp: Update test file. > + (WebCore::jsTestObjCONST_REFLECTED): Won't run-bindings-tests fail for every port that doesn't use V8 unless you also update the JSC code generator?

Comment on attachment 112698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112698&action=review >> Source/WebCore/ChangeLog:13 > > Won't run-bindings-tests fail for every port that doesn't use V8 unless you also update the JSC code generator? Yes. Hmm, does that mean we'll need a change for each CodeGenerator* ?

run-bindings-tests keeps separate results for each code generator.

Comment on attachment 112698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112698&action=review Do we need this feature to work for JSC as well? > Source/WebCore/bindings/scripts/test/TestObj.idl:193 > + const [Reflect=CONST_CUSTOM] unsigned short CONST_REFLECTED = 0; I would have used a number other than zero here, just because 0 is a special number that could come from lots of places. You want to make sure this number makes it to the output file. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1386 > + {"CONST_CUSTOM", static_cast<signed int>(0)}, It looks like you've got this backwards, right? Reading this code CONST_CUSTOM is exposed to JavaScript, but TestObj::CONST_REFLECTED is used on the implementation object. I would have expected the reverse.

Comment on attachment 112698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112698&action=review Adam, yes, this will need to be implemented for JSC too. I'll see what I can do. >> Source/WebCore/bindings/scripts/test/TestObj.idl:193 >> + const [Reflect=CONST_CUSTOM] unsigned short CONST_REFLECTED = 0; > > I would have used a number other than zero here, just because 0 is a special number that could come from lots of places. You want to make sure this number makes it to the output file. OK, will change. >> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1386 >> + {"CONST_CUSTOM", static_cast<signed int>(0)}, > > It looks like you've got this backwards, right? Reading this code CONST_CUSTOM is exposed to JavaScript, but TestObj::CONST_REFLECTED is used on the implementation object. I would have expected the reverse. I was confused by this too. But, I just stole the method used by the other Reflect testers (e.g. attribute [Reflect=customContentStringAttr] DOMString reflectedStringAttr;). I could make them more explicit. How about CONST_JAVASCRIPT and CONST_IMPL ?

> I was confused by this too. But, I just stole the method used by the other Reflect testers (e.g. attribute [Reflect=customContentStringAttr] DOMString reflectedStringAttr;). I could make them more explicit. How about CONST_JAVASCRIPT and CONST_IMPL ? Sure. Another this we can do is to use this feature in a real IDL file and write a LayoutTest that verifies that the right name is exposed as part of the API.

Created attachment 112817 [details] changes added to JSC + new layout test

Comment on attachment 112817 [details] changes added to JSC + new layout test View in context: https://bugs.webkit.org/attachment.cgi?id=112817&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1116 > - my $getter = "js" . $interfaceName . $codeGenerator->WK_ucfirst($constant->name); > + my $reflect = $constant->extendedAttributes->{"Reflect"}; > + my $name = $reflect ? $reflect : $constant->name; > + my $getter = "js" . $interfaceName . $codeGenerator->WK_ucfirst($name); Can we break this copy/paste code out into a function? > Source/WebCore/bindings/scripts/test/TestObj.idl:193 > + const [Reflect=CONST_JAVASCRIPT] unsigned short CONST_IMPL = 15; Isn't this backwards? The attribute should have the implementation name and the main declaration should have the API name that JavaScript sees.

Comment on attachment 112817 [details] changes added to JSC + new layout test View in context: https://bugs.webkit.org/attachment.cgi?id=112817&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1116 >> + my $getter = "js" . $interfaceName . $codeGenerator->WK_ucfirst($name); > > Can we break this copy/paste code out into a function? Yes, that would be better :). >> Source/WebCore/bindings/scripts/test/TestObj.idl:193 >> + const [Reflect=CONST_JAVASCRIPT] unsigned short CONST_IMPL = 15; > > Isn't this backwards? The attribute should have the implementation name and the main declaration should have the API name that JavaScript sees. No. Checkout examples of 'for' and 'default' in HTMLLabelElement.idl and HTMLTrackElement.idl

> No. Checkout examples of 'for' and 'default' in HTMLLabelElement.idl and HTMLTrackElement.idl Those IDL files are wrong. The stuff in the attribute value should be the implementation name and the stuff outside the attribute should be the name exposed to JavaScript. If you remove all our implementation-specific attributs, the IDL files you're left with should match the specs. The spec says: attribute boolean default; not attribute boolean isDefault;

Ah! I understand better how this attribute is to be used. Thanks Adam!

Created attachment 112869 [details] function for getting reflect name + swapping order

Attachment 112869 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1935: jsTestObjCONST_IMPL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:258: jsTestObjCONST_IMPL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 112869 [details] function for getting reflect name + swapping order View in context: https://bugs.webkit.org/attachment.cgi?id=112869&action=review > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1390 > + {"CONST_IMPL", static_cast<signed int>(15)}, The JSC bindings look right, but this one still looks backwards.

Created attachment 113188 [details] only modify the constant with the reflected name for implemenmtation value

Attachment 113188 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1935: jsTestObjCONST_JAVASCRIPT is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:258: jsTestObjCONST_JAVASCRIPT is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 113188 [details] only modify the constant with the reflected name for implemenmtation value View in context: https://bugs.webkit.org/attachment.cgi?id=113188&action=review I don’t understand the need for this [Reflect=XXX] feature for constants. Why can’t we use the ALL CAPS form inside our DOM code too? > Source/WebCore/bindings/scripts/CodeGenerator.pm:572 > + my $name = $reflect ? $reflect : $constant->name; Extra space here after the "=" sign.

Darin, Sorry I didn't include the context/reasoning here. We need the [Reflect] because of some weird windows defines that conflict with the enums defined in TextTrack.idl: (I believe it was TextTrack::ERROR that it didn't like). I tried to hunt down the original error that broke the build, but the old link no longer works. Because we want to use TextTrack::ERROR to match the spec, the Reflect would be a really nice option.

(In reply to comment #23) > We need the [Reflect] because of some weird windows defines that conflict with the enums defined in TextTrack.idl: (I believe it was TextTrack::ERROR that it didn't like). Yes, that should indeed be included in the change log and maybe even a comment in the IDL file.

Created attachment 113223 [details] fixing = typo and adding better description in changelog

Attachment 113223 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1935: jsTestObjCONST_JAVASCRIPT is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:258: jsTestObjCONST_JAVASCRIPT is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 113223 [details] fixing = typo and adding better description in changelog View in context: https://bugs.webkit.org/attachment.cgi?id=113223&action=review > Source/WebCore/html/TextTrack.idl:46 > + const [Reflect=None] unsigned short NONE = 0; > + const [Reflect=Loading] unsigned short LOADING = 1; > + const [Reflect=Loaded] unsigned short LOADED = 2; > + const [Reflect=Error] unsigned short ERROR = 3; > readonly attribute unsigned short readyState; > > - const [Reflect=DISABLED] unsigned short Disabled = 0; > - const [Reflect=HIDDEN] unsigned short Hidden = 1; > - const [Reflect=SHOWING] unsigned short Showing = 2; > + const [Reflect=Disabled] unsigned short DISABLED = 0; > + const [Reflect=Hidden] unsigned short HIDDEN = 1; > + const [Reflect=Showing] unsigned short SHOWING = 2; Given that ERROR is the one that conflicts, do we need to rename all the others?

I just figured it was a good opportunity to better conform to other WebKit enums that do not use all caps.

(In reply to comment #28) > I just figured it was a good opportunity to better conform to other WebKit enums that do not use all caps. I think that’s not a great idea. Generally our DOM bindings use the names from the DOM itself, and don’t try to conform with WebKit coding style. If we did want to do that in general, then we’d probably make the default mapping convert from all uppercase to mixed case rather than doing it on a case by case basis for each DOM enum.

OK, that's fine. I'll change TextTrack.idl to only use [Reflect] for ERROR.

Created attachment 113264 [details] Patch for landing

Comment on attachment 113264 [details] Patch for landing Clearing flags on attachment: 113264 Committed r99027: <http://trac.webkit.org/changeset/99027>

All reviewed patches have been landed. Closing bug.