Bug 70951 – IDL constants can't use [Reflect]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Anna Cavender]

For an example, see WebCore/html/TextTrack.idl.  The [Reflect] is not honored.

[Anna Cavender]

Created attachment 112605 [details]
patch for V8

[Adam Barth]

Comment on attachment 112605 [details]
patch for V8

Can you add an attribute to a test IDL file to exercise the patch?

[Adam Barth]

Comment on attachment 112605 [details]
patch for V8

TestObj.idl in Source/WebCore/bindings/test.  You can run the tests with run-bindings-tests.

[Anna Cavender]

Created attachment 112698 [details]
Patch

Sure thing!  Thanks Adam.

[WebKit Review Bot]

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.

[Eric Carlson]

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?

[Anna Cavender]

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* ?

[Adam Barth]

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

[Adam Barth]

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.

[Anna Cavender]

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 ?

[Adam Barth]

> 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.

[Anna Cavender]

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

[Adam Barth]

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.

[Anna Cavender]

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

[Adam Barth]

> 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;

[Anna Cavender]

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

[Anna Cavender]

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

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Anna Cavender]

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

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Anna Cavender]

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.

[Darin Adler]

(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.

[Anna Cavender]

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

[WebKit Review Bot]

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.

[Darin Adler]

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?

[Anna Cavender]

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

[Darin Adler]

(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.

[Anna Cavender]

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

[Anna Cavender]

Created attachment 113264 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113264 [details]
Patch for landing

Clearing flags on attachment: 113264

Committed r99027: <http://trac.webkit.org/changeset/99027>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.