Bug 103899

Summary: Vibration API: IDL type doesn't match implementation type
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: WebCore Misc.Assignee: Kihong Kwon <kihong.kwon>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, gyuyoung.kim, haraken, japhet, kihong.kwon, laszlo.gombos, mifenton, rakuco, rwlbuis, tonikitoo, vimff0, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103642, 104914    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Adam Bergkvist 2012-12-03 08:32:16 PST
According to WebIDL, "unsigned long" corresponds to "unsigned" in the platform. If long precision is wanted, the IDL should use "unsigned long long". The right type mapping is necessary when the bindings are correctly generated (will be introduced in http://webkit.org/b/103642).

The patch in b103642 uses a workaround in CodeGeneratorJS.pm (tagged with this bug id) to not break the current behavior of the Vibration API. Please remove that workaround when this bug is fixed.
Comment 1 Kihong Kwon 2012-12-10 03:46:50 PST
Created attachment 178506 [details]
Patch
Comment 2 Kentaro Hara 2012-12-10 03:53:30 PST
Comment on attachment 178506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178506&action=review

Looks reasonable.

> Source/WebCore/ChangeLog:27
> +        * bindings/scripts/CodeGeneratorJS.pm:
> +        Remove workaround codes for the Vibration API which is mapped from unsigned long to unsigned long.
> +        It should be mapped from unsigned long to unsigned by WebIDL spec.

Would you check that this change won't affect other code?

No change in CodeGeneratorV8.pm? (why?)
Comment 3 Adam Bergkvist 2012-12-10 04:25:30 PST
Comment on attachment 178506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178506&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3078
> +    return "unsigned" if $arrayOrSequenceType eq "unsigned long";

This line is not needed since "unsigned log" is handled by the nativeType hash below.
Comment 4 Kihong Kwon 2012-12-10 20:24:57 PST
(In reply to comment #2)
> (From update of attachment 178506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178506&action=review
> 
> Looks reasonable.
> 
> > Source/WebCore/ChangeLog:27
> > +        * bindings/scripts/CodeGeneratorJS.pm:
> > +        Remove workaround codes for the Vibration API which is mapped from unsigned long to unsigned long.
> > +        It should be mapped from unsigned long to unsigned by WebIDL spec.
> 
> Would you check that this change won't affect other code?

I checked that, this change affect only sequence<unsigned long> in the WebIDL, but there is no area using this without Vibration API
> 
> No change in CodeGeneratorV8.pm? (why?)

GetNativeType in the CodeGeneratorV8.pm can handle this already.
Comment 5 Kihong Kwon 2012-12-10 20:25:20 PST
(In reply to comment #3)
> (From update of attachment 178506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178506&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3078
> > +    return "unsigned" if $arrayOrSequenceType eq "unsigned long";
> 
> This line is not needed since "unsigned log" is handled by the nativeType hash below.

You are right. I will remove that.
Comment 6 Kentaro Hara 2012-12-10 20:26:09 PST
Comment on attachment 178506 [details]
Patch

Looks good. Please address Adam's comment before landing.
Comment 7 Kihong Kwon 2012-12-10 20:30:09 PST
Created attachment 178698 [details]
Patch
Comment 8 Kihong Kwon 2012-12-10 20:54:50 PST
Kentaro, could you review this one more time please?
I added some changes to the JSTestObj.cpp for run_binding_tests.
Comment 9 WebKit Review Bot 2012-12-11 21:26:12 PST
Comment on attachment 178698 [details]
Patch

Clearing flags on attachment: 178698

Committed r137410: <http://trac.webkit.org/changeset/137410>
Comment 10 WebKit Review Bot 2012-12-11 21:26:17 PST
All reviewed patches have been landed.  Closing bug.