Summary: | MutationCallback should be a WebIDL 'callback', not a [Callback] interface | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||||||||
Component: | DOM | Assignee: | 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
Adam Klein
2012-07-16 11:14:08 PDT
Created attachment 191872 [details]
WIP
Comment on attachment 191872 [details] WIP Attachment 191872 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17059101 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 Created attachment 191877 [details]
WIP
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 Comment on attachment 191877 [details] WIP Attachment 191877 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17070047 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 Created attachment 191879 [details]
WIP: try to fix JSC build
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 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 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 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 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 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 Created attachment 192081 [details]
Patch
Now works on JSC as well as V8, ready for a look. 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. :) +ggaren for JSC questions 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. 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.
Comment on attachment 192081 [details] Patch Clearing flags on attachment: 192081 Committed r145379: <http://trac.webkit.org/changeset/145379> All reviewed patches have been landed. Closing bug. |