Bug 103899 - Vibration API: IDL type doesn't match implementation type
Summary: Vibration API: IDL type doesn't match implementation type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kihong Kwon
URL:
Keywords:
Depends on: 103642 104914
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-03 08:32 PST by Adam Bergkvist
Modified: 2012-12-13 07:10 PST (History)
13 users (show)

See Also:


Attachments
Patch (20.45 KB, patch)
2012-12-10 03:46 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (22.20 KB, patch)
2012-12-10 20:30 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.