Bug 91406

Summary: MutationCallback should be a WebIDL 'callback', not a [Callback] interface
Product: WebKit Reporter: Adam Klein <adamk>
Component: DOMAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, esprehn+autocc, esprehn, ggaren, haraken, japhet, ojan.autocc, ojan, philn, rafaelw, rego+ews, rniwa, sam, syoichi, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68729    
Attachments:
Description Flags
WIP
none
WIP
none
WIP: try to fix JSC build
none
Patch none

Adam Klein
Reported 2012-07-16 11:14:08 PDT
Per the DOM4 spec, http://www.w3.org/TR/dom/#mutationcallback, MutationCallback should be a 'callback'. The main differences between a callback and a [Callback] interface: It must be a function; an object with a 'handleEvent' property that's a function is not acceptable (and should throw a TypeError). See http://www.w3.org/TR/WebIDL/#es-callback-function. This form of callback is not currently supported by CodeGenerator.pm, but it seems like it would be a good thing to add.
Attachments
WIP (10.29 KB, patch)
2013-03-06 17:27 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
WIP (18.45 KB, patch)
2013-03-06 17:58 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
WIP: try to fix JSC build (18.43 KB, patch)
2013-03-06 18:07 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
Patch (19.28 KB, patch)
2013-03-07 13:48 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Adam Klein
Comment 1 2013-03-06 17:27:20 PST
Created attachment 191872 [details] WIP
Early Warning System Bot
Comment 2 2013-03-06 17:43:15 PST
Comment on attachment 191872 [details] WIP Attachment 191872 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17059101
Early Warning System Bot
Comment 3 2013-03-06 17:49:33 PST
Comment on attachment 191872 [details] WIP Attachment 191872 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17086110
Adam Klein
Comment 4 2013-03-06 17:58:24 PST
Created attachment 191877 [details] WIP
Build Bot
Comment 5 2013-03-06 18:02:29 PST
Comment on attachment 191877 [details] WIP Attachment 191877 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17023063
Early Warning System Bot
Comment 6 2013-03-06 18:03:54 PST
Comment on attachment 191877 [details] WIP Attachment 191877 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17070047
Early Warning System Bot
Comment 7 2013-03-06 18:04:26 PST
Comment on attachment 191877 [details] WIP Attachment 191877 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17070048
Adam Klein
Comment 8 2013-03-06 18:07:35 PST
Created attachment 191879 [details] WIP: try to fix JSC build
Early Warning System Bot
Comment 9 2013-03-06 18:10:56 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17070053
Early Warning System Bot
Comment 10 2013-03-06 18:12:22 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17010044
Build Bot
Comment 11 2013-03-06 18:25:10 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17010049
Build Bot
Comment 12 2013-03-06 18:27:22 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17050270
EFL EWS Bot
Comment 13 2013-03-06 19:23:52 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17023098
Build Bot
Comment 14 2013-03-07 12:44:06 PST
Comment on attachment 191879 [details] WIP: try to fix JSC build Attachment 191879 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17064304
Adam Klein
Comment 15 2013-03-07 13:48:23 PST
Created attachment 192081 [details] Patch
Adam Klein
Comment 16 2013-03-08 15:13:08 PST
Now works on JSC as well as V8, ready for a look.
Adam Barth
Comment 17 2013-03-08 15:17:35 PST
Comment on attachment 192081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192081&action=review > Source/WebCore/bindings/js/JSMutationCallback.cpp:69 > + ASSERT_NOT_REACHED(); > + return; Do we need to handle this case if it can't be reached? > Source/WebCore/bindings/js/JSMutationCallback.cpp:95 > + globalObject->globalData().timeoutChecker.start(); > + InspectorInstrumentationCookie cookie = JSMainThreadExecState::instrumentFunctionCall(context, callType, callData); > + > + JSMainThreadExecState::call(exec, callback, callType, callData, jsObserver, args); > + > + InspectorInstrumentation::didCallFunction(cookie); > + globalObject->globalData().timeoutChecker.stop(); > + > + if (exec->hadException()) > + reportCurrentException(exec); Should we wrap this in a helper function so that we don't need to copy/paste this code for every callback? > Source/WebCore/bindings/v8/V8MutationCallback.cpp:78 > + ScriptController::callFunctionWithInstrumentation(scriptExecutionContext(), callback, thisObject, 2, argv); ScriptController::callFunctionWithInstrumentation <--- Looks like we have the wrapper function for V8 already. :)
Adam Klein
Comment 18 2013-03-08 15:24:00 PST
+ggaren for JSC questions
Adam Klein
Comment 19 2013-03-08 15:24:17 PST
Comment on attachment 192081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192081&action=review >> Source/WebCore/bindings/js/JSMutationCallback.cpp:69 >> + return; > > Do we need to handle this case if it can't be reached? I don't know enough about JSC API to answer that question well. It seems like I might be able to avoid this by storing a JSC::JSFunction* instead of a JSC::JSObject*, but it doesn't seem like that's at all common in JSC code. Basically, I think it's a good idea to handle this case for now, since it's trying to enforce an invariant from another class (in this case, the constructor of JSMutationObserver in JSMutationObserverCustom.cpp). >> Source/WebCore/bindings/js/JSMutationCallback.cpp:95 >> + reportCurrentException(exec); > > Should we wrap this in a helper function so that we don't need to copy/paste this code for every callback? Seems like that'd be a good question for some JSC folks, I'm not sure they want yet-another method on JSMainThreadExecState.
Adam Klein
Comment 20 2013-03-11 12:06:33 PDT
Comment on attachment 192081 [details] Patch Neither of the outstanding questions affect the correctness of this code, so I'm sending this to the cq. Those questions can be dealt with as a post-submit cleanup as necessary.
WebKit Review Bot
Comment 21 2013-03-11 12:18:12 PDT
Comment on attachment 192081 [details] Patch Clearing flags on attachment: 192081 Committed r145379: <http://trac.webkit.org/changeset/145379>
WebKit Review Bot
Comment 22 2013-03-11 12:18:18 PDT
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.