Bug 91406 – MutationCallback should be a WebIDL 'callback', not a [Callback] interface

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

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.

[Adam Klein]

Created attachment 191872 [details]
WIP

[Early Warning System Bot]

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 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]

Created attachment 191877 [details]
WIP

[Build Bot]

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 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 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]

Created attachment 191879 [details]
WIP: try to fix JSC build

[Early Warning System Bot]

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 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 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 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 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 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]

Created attachment 192081 [details]
Patch

[Adam Klein]

Now works on JSC as well as V8, ready for a look.

[Adam Barth]

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]

+ggaren for JSC questions

[Adam Klein]

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 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 on attachment 192081 [details]
Patch

Clearing flags on attachment: 192081

Committed r145379: <http://trac.webkit.org/changeset/145379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.