Bug 115250

Summary: Get rid of [Callback] IDL extended attribute for parameters
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Tools / TestsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, apavlov, buildbot, commit-queue, eric.carlson, esprehn+autocc, gtk-ews, haraken, hta, jer.noble, jsbell, laszlo.gombos, macpherson, menard, philn, rniwa, tommyw, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 115247    
Bug Blocks:    
Attachments:
Description Flags
Patch
gtk-ews: commit-queue-
Patch
gtk-ews: commit-queue-
Patch
none
Patch
none
Patch
gtk-ews: commit-queue-
Patch
none
Patch none

Chris Dumez
Reported 2013-04-26 05:27:00 PDT
The [Callback] IDL extended attribute is not part of the Web IDL specification and should not be needed. The bindings generator should be smart enough to know by itself if a parameter's type is a callback interface. This will results in simpler IDL files and closer to the standard.
Attachments
Patch (47.52 KB, patch)
2013-04-26 13:07 PDT, Chris Dumez 		gtk-ews: commit-queue-
DetailsFormatted DiffDiff
Patch (49.83 KB, patch)
2013-04-26 13:59 PDT, Chris Dumez 		gtk-ews: commit-queue-
DetailsFormatted DiffDiff
Patch (50.75 KB, patch)
2013-04-26 14:52 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (50.76 KB, patch)
2013-04-26 14:56 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (50.99 KB, patch)
2013-04-26 15:07 PDT, Chris Dumez 		gtk-ews: commit-queue-
DetailsFormatted DiffDiff
Patch (51.87 KB, patch)
2013-04-26 16:36 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (104.11 KB, patch)
2013-04-27 06:17 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2013-04-26 13:07:53 PDT
Created attachment 199853 [details] Patch
kov's GTK+ EWS bot
Comment 2 2013-04-26 13:26:22 PDT
Comment on attachment 199853 [details] Patch Attachment 199853 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/133502
Build Bot
Comment 3 2013-04-26 13:29:05 PDT
Comment on attachment 199853 [details] Patch Attachment 199853 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/133498
Build Bot
Comment 4 2013-04-26 13:32:04 PDT
Comment on attachment 199853 [details] Patch Attachment 199853 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/188022
Build Bot
Comment 5 2013-04-26 13:47:02 PDT
Comment on attachment 199853 [details] Patch Attachment 199853 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/133499
Chris Dumez
Comment 6 2013-04-26 13:59:27 PDT
Created attachment 199856 [details] Patch
kov's GTK+ EWS bot
Comment 7 2013-04-26 14:08:05 PDT
Comment on attachment 199856 [details] Patch Attachment 199856 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/14513
Chris Dumez
Comment 8 2013-04-26 14:52:05 PDT
Created attachment 199863 [details] Patch
Chris Dumez
Comment 9 2013-04-26 14:56:39 PDT
Created attachment 199865 [details] Patch
Chris Dumez
Comment 10 2013-04-26 15:07:19 PDT
Created attachment 199866 [details] Patch
kov's GTK+ EWS bot
Comment 11 2013-04-26 15:14:32 PDT
Comment on attachment 199866 [details] Patch Attachment 199866 [details] did not pass gtk-wk2-ews (gtk-wk2): Output: http://webkit-queues.appspot.com/results/43916
kov's GTK+ EWS bot
Comment 12 2013-04-26 15:15:27 PDT
Comment on attachment 199866 [details] Patch Attachment 199866 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/13432
Chris Dumez
Comment 13 2013-04-26 16:36:00 PDT
Created attachment 199873 [details] Patch Should fix GTK ews.
Kentaro Hara
Comment 14 2013-04-26 22:20:57 PDT
Comment on attachment 199873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199873&action=review > Source/WebCore/Modules/webdatabase/SQLTransaction.idl:37 > + in [Optional] SQLStatementCallback callback, Maybe you want to replace [Optional] with 'optional' when you have time. It's already done in Blink :) > Source/WebCore/bindings/scripts/CodeGenerator.pm:431 > + return 0 if $type eq "Array"; This wouldn't be needed since it is already checked in GetArrayType(). > Source/WebCore/bindings/scripts/CodeGenerator.pm:679 > + return 0 unless $object->IsRefPtrType($type); > + return 0 if $type eq "SerializedScriptValue"; Why do you want to write SerializedScriptValue here instead of in IsRefPtrType(). Actually I'm not sure if we should use IsRefPtrType() here. Maybe we should introduce IsWrapperType() for this use, just like V8 does.
Chris Dumez
Comment 15 2013-04-26 23:52:25 PDT
Comment on attachment 199873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199873&action=review >> Source/WebCore/Modules/webdatabase/SQLTransaction.idl:37 >> + in [Optional] SQLStatementCallback callback, > > Maybe you want to replace [Optional] with 'optional' when you have time. It's already done in Blink :) On my TODO list :) >> Source/WebCore/bindings/scripts/CodeGenerator.pm:431 >> + return 0 if $type eq "Array"; > > This wouldn't be needed since it is already checked in GetArrayType(). ok >> Source/WebCore/bindings/scripts/CodeGenerator.pm:679 >> + return 0 if $type eq "SerializedScriptValue"; > > Why do you want to write SerializedScriptValue here instead of in IsRefPtrType(). > > Actually I'm not sure if we should use IsRefPtrType() here. Maybe we should introduce IsWrapperType() for this use, just like V8 does. Well, SerializedScriptValue is a RefPtr type so I cannot move it to IsRefPtrType(). Anyway, I think you are right, introducing an IsWrapperType() would probably be clearer and we would avoid editing existing functions.
Chris Dumez
Comment 16 2013-04-27 05:42:31 PDT
Comment on attachment 199873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199873&action=review >>> Source/WebCore/bindings/scripts/CodeGenerator.pm:431 >>> + return 0 if $type eq "Array"; >> >> This wouldn't be needed since it is already checked in GetArrayType(). > > ok Actually GetArrayType() return the array subtype: return $1 if $type =~ /^([\w\d_\s]+)\[\]/; Therefore, it is not applicable to the "Array" type.
Chris Dumez
Comment 17 2013-04-27 06:17:35 PDT
Created attachment 199902 [details] Patch Introduce IsWrapperType subroutine.
Kentaro Hara
Comment 18 2013-04-28 01:50:08 PDT
Comment on attachment 199902 [details] Patch Looks good.
WebKit Commit Bot
Comment 19 2013-04-28 02:36:28 PDT
The commit-queue encountered the following flaky tests while processing attachment 199902 [details]: fast/frames/crash-remove-iframe-during-object-beforeload.html bug 115322 (author: zalan@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 20 2013-04-28 02:37:27 PDT
Comment on attachment 199902 [details] Patch Clearing flags on attachment: 199902 Committed r149257: <http://trac.webkit.org/changeset/149257>
WebKit Commit Bot
Comment 21 2013-04-28 02:37:32 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 22 2013-04-30 01:08:18 PDT
Updated WebKitIDL wiki accordingly: http://trac.webkit.org/wiki/WebKitIDL?action=diff&version=90&old_version=89
Note You need to log in before you can comment on or make changes to this bug.