Bug 81712 - "this" argument for MutationCallbacks should be the MutationObserver
Summary: "this" argument for MutationCallbacks should be the MutationObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-03-20 16:08 PDT by Adam Klein
Modified: 2012-03-21 15:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.61 KB, patch)
2012-03-21 12:40 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (12.37 KB, patch)
2012-03-21 14:48 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-03-20 16:08:05 PDT
"this" argument for MutationCallbacks should be the MutationObserver
Comment 1 Adam Klein 2012-03-21 12:40:09 PDT
Created attachment 133097 [details]
Patch
Comment 2 Adam Klein 2012-03-21 12:41:04 PDT
Add some bindings experts.
Comment 3 Adam Barth 2012-03-21 13:45:37 PDT
Comment on attachment 133097 [details]
Patch

Do other callbacks have this problem?  Why is this code custom and not autogenerated.
Comment 4 Adam Klein 2012-03-21 13:54:26 PDT
(In reply to comment #3)
> (From update of attachment 133097 [details])
> Do other callbacks have this problem?  Why is this code custom and not autogenerated.

It's specified to be the observer by DOM4 (see step 4):

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-mo-invoke

Note that WebIDL lets specifications define this on a per-callback basis:

http://dev.w3.org/2006/webapi/WebIDL/#dfn-callback-this-value

I think there probably should be some codegen perl here, but I'm not sure exactly what it should look like, so I'd like to land this for now and wait to see if we have other users of this WebIDL feature.
Comment 5 Erik Arvidsson 2012-03-21 14:22:50 PDT
Comment on attachment 133097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133097&action=review

LGTM

It is really silly to pass the mutation observer as the second argument to the callback but that is a topic for the spec and not for this patch.

> LayoutTests/fast/mutation/callback-this-argument.html:4
> +if (window.layoutTestController) {

Can you use js-test-pre?

> LayoutTests/fast/mutation/callback-this-argument.html:9
> +function mutationCallback(mutations) {

Should we also test arguments[1]?
Comment 6 Adam Klein 2012-03-21 14:35:21 PDT
Comment on attachment 133097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133097&action=review

>> LayoutTests/fast/mutation/callback-this-argument.html:9
>> +function mutationCallback(mutations) {
> 
> Should we also test arguments[1]?

Will combine with callback-second-argument.html and use js-test-pre.
Comment 7 Adam Klein 2012-03-21 14:48:25 PDT
Created attachment 133115 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-21 15:34:46 PDT
Comment on attachment 133115 [details]
Patch for landing

Clearing flags on attachment: 133115

Committed r111611: <http://trac.webkit.org/changeset/111611>
Comment 9 WebKit Review Bot 2012-03-21 15:34:51 PDT
All reviewed patches have been landed.  Closing bug.