Bug 70951 - IDL constants can't use [Reflect]
Summary: IDL constants can't use [Reflect]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-26 11:41 PDT by Anna Cavender
Modified: 2011-11-01 18:52 PDT (History)
7 users (show)

See Also:


Attachments
patch for V8 (1.46 KB, patch)
2011-10-26 15:26 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2011-10-27 10:10 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
changes added to JSC + new layout test (37.19 KB, patch)
2011-10-27 22:55 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
function for getting reflect name + swapping order (14.93 KB, patch)
2011-10-28 08:48 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
only modify the constant with the reflected name for implemenmtation value (17.31 KB, patch)
2011-11-01 10:21 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
fixing = typo and adding better description in changelog (17.73 KB, patch)
2011-11-01 14:33 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff
Patch for landing (20.60 KB, patch)
2011-11-01 17:29 PDT, Anna Cavender
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-10-26 11:41:38 PDT
For an example, see WebCore/html/TextTrack.idl.  The [Reflect] is not honored.
Comment 1 Anna Cavender 2011-10-26 15:26:23 PDT
Created attachment 112605 [details]
patch for V8
Comment 2 Adam Barth 2011-10-26 16:28:51 PDT
Comment on attachment 112605 [details]
patch for V8

Can you add an attribute to a test IDL file to exercise the patch?
Comment 3 Adam Barth 2011-10-26 16:32:41 PDT
Comment on attachment 112605 [details]
patch for V8

TestObj.idl in Source/WebCore/bindings/test.  You can run the tests with run-bindings-tests.
Comment 4 Anna Cavender 2011-10-27 10:10:15 PDT
Created attachment 112698 [details]
Patch

Sure thing!  Thanks Adam.
Comment 5 WebKit Review Bot 2011-10-27 10:14:09 PDT
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 6 Eric Carlson 2011-10-27 11:04:03 PDT
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 7 Anna Cavender 2011-10-27 11:30:00 PDT
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* ?
Comment 8 Adam Barth 2011-10-27 11:36:29 PDT
run-bindings-tests keeps separate results for each code generator.
Comment 9 Adam Barth 2011-10-27 11:41:42 PDT
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 10 Anna Cavender 2011-10-27 11:50:16 PDT
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 ?
Comment 11 Adam Barth 2011-10-27 12:18:21 PDT
> 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.
Comment 12 Anna Cavender 2011-10-27 22:55:54 PDT
Created attachment 112817 [details]
changes added to JSC + new layout test
Comment 13 Adam Barth 2011-10-27 22:59:35 PDT
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 14 Anna Cavender 2011-10-27 23:13:49 PDT
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
Comment 15 Adam Barth 2011-10-27 23:22:00 PDT
> 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;
Comment 16 Anna Cavender 2011-10-27 23:43:07 PDT
Ah!  I understand better how this attribute is to be used.  Thanks Adam!
Comment 17 Anna Cavender 2011-10-28 08:48:22 PDT
Created attachment 112869 [details]
function for getting reflect name + swapping order
Comment 18 WebKit Review Bot 2011-10-28 08:50:20 PDT
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 19 Adam Barth 2011-10-28 11:11:08 PDT
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.
Comment 20 Anna Cavender 2011-11-01 10:21:05 PDT
Created attachment 113188 [details]
only modify the constant with the reflected name for implemenmtation value
Comment 21 WebKit Review Bot 2011-11-01 10:25:05 PDT
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 22 Darin Adler 2011-11-01 10:39:27 PDT
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.
Comment 23 Anna Cavender 2011-11-01 11:00:07 PDT
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.
Comment 24 Darin Adler 2011-11-01 11:54:29 PDT
(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.
Comment 25 Anna Cavender 2011-11-01 14:33:02 PDT
Created attachment 113223 [details]
fixing = typo and adding better description in changelog
Comment 26 WebKit Review Bot 2011-11-01 14:36:07 PDT
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 27 Darin Adler 2011-11-01 15:20:27 PDT
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?
Comment 28 Anna Cavender 2011-11-01 16:01:45 PDT
I just figured it was a good opportunity to better conform to other WebKit enums that do not use all caps.
Comment 29 Darin Adler 2011-11-01 16:39:59 PDT
(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.
Comment 30 Anna Cavender 2011-11-01 17:06:10 PDT
OK, that's fine.  I'll change TextTrack.idl to only use [Reflect] for ERROR.
Comment 31 Anna Cavender 2011-11-01 17:29:53 PDT
Created attachment 113264 [details]
Patch for landing
Comment 32 WebKit Review Bot 2011-11-01 18:52:52 PDT
Comment on attachment 113264 [details]
Patch for landing

Clearing flags on attachment: 113264

Committed r99027: <http://trac.webkit.org/changeset/99027>
Comment 33 WebKit Review Bot 2011-11-01 18:52:58 PDT
All reviewed patches have been landed.  Closing bug.