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

Adam Bergkvist
Reported 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.
Attachments
Patch (20.45 KB, patch)
2012-12-10 03:46 PST, Kihong Kwon 		no flags
DetailsFormatted DiffDiff
Patch (22.20 KB, patch)
2012-12-10 20:30 PST, Kihong Kwon 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kihong Kwon
Comment 1 2012-12-10 03:46:50 PST
Created attachment 178506 [details] Patch
Kentaro Hara
Comment 2 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?)
Adam Bergkvist
Comment 3 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.
Kihong Kwon
Comment 4 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.
Kihong Kwon
Comment 5 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.
Kentaro Hara
Comment 6 2012-12-10 20:26:09 PST
Comment on attachment 178506 [details] Patch Looks good. Please address Adam's comment before landing.
Kihong Kwon
Comment 7 2012-12-10 20:30:09 PST
Created attachment 178698 [details] Patch
Kihong Kwon
Comment 8 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.
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2012-12-11 21:26:17 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.