Bug 68949 – [MutationObservers] Add stub implementation of WebKitMutationObserver

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

[MutationObservers] Add stub implementation of WebKitMutationObserver

[Adam Klein]

Created attachment 108923 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 108923 [details]
Patch

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

> LayoutTests/platform/qt/Skipped:149
> +# ENABLE(MUTATION_OBSERVERS) is not yet enabled. http://webkit.org/b/68729

Nit: say "MutationObservers is not yet enabled" to match the rest?

> Source/WebCore/dom/MutationCallback.idl:38
> +    ] MutationCallback {
> +        boolean handleEvent(in MutationRecordArray mutations,
> +                            in WebKitMutationObserver observer);
> +    };

When is this interface used?  Is WebKitMutationObserver going to take this object instead of just a function?

[Adam Klein]

Comment on attachment 108923 [details]
Patch

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

>> LayoutTests/platform/qt/Skipped:149
>> +# ENABLE(MUTATION_OBSERVERS) is not yet enabled. http://webkit.org/b/68729
> 
> Nit: say "MutationObservers is not yet enabled" to match the rest?

Eh, I was trying to match the style elsewhere in this file.  If you feel strongly, I can change it.

>> Source/WebCore/dom/MutationCallback.idl:38
>> +    };
> 
> When is this interface used?  Is WebKitMutationObserver going to take this object instead of just a function?

Ah, looks like I'm missing a bit of plumbing here.  I need to make the WebKitMutationObserverConstructor take a MutationCallback.  I could use a pointer to an example of constructors...do I need to write some custom code here?

Also, note that the [Callback] annotation means a function can be substituted for an object and it'll be called with the arguments to handleEvent.

[Adam Klein]

Created attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

[Ryosuke Niwa]

Comment on attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

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

I need some other reviewer looking at JSC/V8 bindings.

> Source/WebCore/dom/MutationCallback.idl:37
> +                            in WebKitMutationObserver observer);

Nit: wrong indentation. it should only be indented by 4 spaces as in:
boolean handleEvent(in MutationRecordArray mutations,
    in WebKitMutationObserver observer);

> Source/WebCore/dom/MutationRecordArray.h:62
> +        m_records.swap(records);

Really? doesn't swap modify records? I don't think that's a very intuitive behavior.

> Source/WebCore/dom/WebKitMutationObserver.idl:40
> +                     in WebKitMutationObserverOptions options);

Nit: wrong indentation.

[Adam Klein]

Comment on attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

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

>> Source/WebCore/dom/MutationCallback.idl:37
>> +                            in WebKitMutationObserver observer);
> 
> Nit: wrong indentation. it should only be indented by 4 spaces as in:
> boolean handleEvent(in MutationRecordArray mutations,
>     in WebKitMutationObserver observer);

Is this documented somewhere? Most examples I've seen in WebCore IDL have used this style (which imho is more readable).  Examples: *Event.idl, Document.idl.  A quick grep doesn't find any with 12 leading spaces followed by 'in'.

>> Source/WebCore/dom/MutationRecordArray.h:62
>> +        m_records.swap(records);
> 
> Really? doesn't swap modify records? I don't think that's a very intuitive behavior.

Thus the factory method's "adopt" name.  I took this convention from StaticNodeList.  The point is to avoid doing an unnecessary copy, since the caller is expected to not have any use for the vector after passing it in.  Would a comment on adopt() help?  I could make this a PassOwnPtr, but that constrains the caller a bit more than necessary.

>> Source/WebCore/dom/WebKitMutationObserver.idl:40
>> +                     in WebKitMutationObserverOptions options);
> 
> Nit: wrong indentation.

See above.

[Darin Adler]

You’re right, many of the IDL files currently line up subsequent lines, but this does go against WebKit style guidelines. The subsequent lines should not be lined up.

[Ryosuke Niwa]

Comment on attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

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

>>> Source/WebCore/dom/MutationRecordArray.h:62
>>> +        m_records.swap(records);
>> 
>> Really? doesn't swap modify records? I don't think that's a very intuitive behavior.
> 
> Thus the factory method's "adopt" name.  I took this convention from StaticNodeList.  The point is to avoid doing an unnecessary copy, since the caller is expected to not have any use for the vector after passing it in.  Would a comment on adopt() help?  I could make this a PassOwnPtr, but that constrains the caller a bit more than necessary.

I think the problem comes from the fact the swap is done in the constructor. Can we do it in adopt() instead so that the context is clear? I don't think there's any need for a comment once you do that.

[Daniel Bates]

Comment on attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

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

I have some minor nits. None of these are show stoppers and I didn't check this patch for correctness.

> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:57
> +        JSObject* object = exec->argument(0).getObject();
> +        if (object)

Nit: I would inline this definition into the if-statement condition since this variable is only referenced within the block "if (object)".

> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY

Nit: This license block differs from the license block used in all the other new files added by this patch. Was this intended?

> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:43
> +        return throwError("DOM object constructor cannot be called as a function.", V8Proxy::TypeError);

Nit: This is the only error message in this patch that uses a complete sentence that ends in a period. I suggest using a consistent style for error messages.

> Source/WebCore/dom/WebKitMutationObserver.cpp:53
> +WebKitMutationObserver::~WebKitMutationObserver()
> +{
> +}

Nit: Is it necessary to explicitly define an empty destructor? Doesn't the compiler generate such a default destructor when a destructor isn't declared? Are we worried about inlining of such a default destructor?

[Sam Weinig]

If WebKitMutationObserverOptions is really supposed to be a dictionary, I don't think it should be included as it currently is.

[Adam Klein]

Created attachment 109545 [details]
Fixed nits, got rid of MutationRecordArray

[Adam Klein]

Comment on attachment 109502 [details]
Add custom constructors for WebKitMutationObserver

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

>> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:57
>> +        if (object)
> 
> Nit: I would inline this definition into the if-statement condition since this variable is only referenced within the block "if (object)".

Done.

>> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:13
>> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND ANY
> 
> Nit: This license block differs from the license block used in all the other new files added by this patch. Was this intended?

Oops, need to set up a better script to autogen my source files.  Fixed, and fixed the others I noticed (should all be new Google Inc versions).

>> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:43
>> +        return throwError("DOM object constructor cannot be called as a function.", V8Proxy::TypeError);
> 
> Nit: This is the only error message in this patch that uses a complete sentence that ends in a period. I suggest using a consistent style for error messages.

This is boilerplate from elsewhere (V8Proxy.h, other custom constructors).  Would rather leave as-is for consistency across files.

>>> Source/WebCore/dom/MutationCallback.idl:37
>>> +                            in WebKitMutationObserver observer);
>> 
>> Nit: wrong indentation. it should only be indented by 4 spaces as in:
>> boolean handleEvent(in MutationRecordArray mutations,
>>     in WebKitMutationObserver observer);
> 
> Is this documented somewhere? Most examples I've seen in WebCore IDL have used this style (which imho is more readable).  Examples: *Event.idl, Document.idl.  A quick grep doesn't find any with 12 leading spaces followed by 'in'.

Moved onto a single line (since that seems to best-match WebKit style).

>>>> Source/WebCore/dom/MutationRecordArray.h:62
>>>> +        m_records.swap(records);
>>> 
>>> Really? doesn't swap modify records? I don't think that's a very intuitive behavior.
>> 
>> Thus the factory method's "adopt" name.  I took this convention from StaticNodeList.  The point is to avoid doing an unnecessary copy, since the caller is expected to not have any use for the vector after passing it in.  Would a comment on adopt() help?  I could make this a PassOwnPtr, but that constrains the caller a bit more than necessary.
> 
> I think the problem comes from the fact the swap is done in the constructor. Can we do it in adopt() instead so that the context is clear? I don't think there's any need for a comment once you do that.

This file is now gone: I wrote custom handleEvent methods to get around this issue, and pass an array in.

>> Source/WebCore/dom/WebKitMutationObserver.cpp:53
>> +}
> 
> Nit: Is it necessary to explicitly define an empty destructor? Doesn't the compiler generate such a default destructor when a destructor isn't declared? Are we worried about inlining of such a default destructor?

This is necessary to avoid having the header file include MutationCallback.h, since the class has a RefPtr member which derefs on destruction and thus needs to know MutationCallback's declaration.

>>> Source/WebCore/dom/WebKitMutationObserver.idl:40
>>> +                     in WebKitMutationObserverOptions options);
>> 
>> Nit: wrong indentation.
> 
> See above.

Same as above, moved onto a single line.

[Ryosuke Niwa]

Comment on attachment 109545 [details]
Fixed nits, got rid of MutationRecordArray

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

r- given Sam's comment.

> Source/WebCore/dom/WebKitMutationObserver.idl:39
> +        void observe(in Node target, in WebKitMutationObserverOptions options);

It seems like Sam want you to replace WebKitMutationObserverOptions by Object and properly handle dictionary.

[Adam Klein]

Created attachment 110235 [details]
Added custom code for MutationObserverOptions, removed IDL

[Ryosuke Niwa]

Comment on attachment 110235 [details]
Added custom code for MutationObserverOptions, removed IDL

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

> LayoutTests/fast/mutation/mutation-observer-constructor.html:4
> +<meta charset="utf-8">

Why do you need to specify charset?

> Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:46
> +bool JSMutationCallback::handleEvent(MutationRecordArray* mutations, WebKitMutationObserver* observer)

This function should probably take const MutationRecordArray&.

> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:68
> +    RefPtr<WebKitMutationObserver> observer = WebKitMutationObserver::create(callback.release());
> +
> +    return JSValue::encode(asObject(toJS(exec, jsConstructor->globalObject(), observer.release())));

Nit: This can be done in one line.

> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:92
> +        return jsUndefined();

When do we execute this line?

> Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:50
> +bool V8MutationCallback::handleEvent(MutationRecordArray* mutations, WebKitMutationObserver* observer)

This function should probably take const MutationRecordArray&.

> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:95
> +#define SET_OPTION(option, setter) \
> +    do { \
> +        bool option; \
> +        if (optionsObject.getKeyValue(#option, option)) \
> +            options->setter(option); \
> +    } while (0)

Is this a common practice?

> Source/WebCore/dom/MutationCallback.h:51
> +    virtual bool handleEvent(MutationRecordArray*, WebKitMutationObserver*) = 0;

This function should probably take const MutationRecordArray&.

> Source/WebCore/dom/MutationCallback.idl:36
> +        [Custom] boolean handleEvent(in MutationRecordArray mutations, in WebKitMutationObserver observer);

Now that we have a custom binding, mutations should be of type Array.

[Ryosuke Niwa]

Can we somehow make EWS green? I'd like to make sure we're not breaking any builds.

[Adam Klein]

Comment on attachment 110235 [details]
Added custom code for MutationObserverOptions, removed IDL

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

>> LayoutTests/fast/mutation/mutation-observer-constructor.html:4
>> +<meta charset="utf-8">
> 
> Why do you need to specify charset?

This is boilerplate created by Tools/Scripts/make-new-script-test.  It's certainly not going to hurt anything, but I can remove it if you like.

>> Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:46
>> +bool JSMutationCallback::handleEvent(MutationRecordArray* mutations, WebKitMutationObserver* observer)
> 
> This function should probably take const MutationRecordArray&.

This signature is generated by CodeGeneratorJS.pm, so it's not really up to me.

>> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:68
>> +    return JSValue::encode(asObject(toJS(exec, jsConstructor->globalObject(), observer.release())));
> 
> Nit: This can be done in one line.

Done locally, currently working on merging, will be reflected in next patch.

>> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:92
>> +        return jsUndefined();
> 
> When do we execute this line?

From my reading of JSDictionary, it's if the options object has a getter that throws an exception.

>> Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:50
>> +bool V8MutationCallback::handleEvent(MutationRecordArray* mutations, WebKitMutationObserver* observer)
> 
> This function should probably take const MutationRecordArray&.

Again, CodeGeneratorV8 is what makes this signature (when it sees [Callback]).

>> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:95
>> +    } while (0)
> 
> Is this a common practice?

It's something of a C-ism, but yeah, it's fairly common.  Given that we need to define a local variable to pass as a reference to getKeyValue, it should be scoped to a block, and using a do{}while forces the usage to end each invocation with a semicolon.

>> Source/WebCore/dom/MutationCallback.h:51
>> +    virtual bool handleEvent(MutationRecordArray*, WebKitMutationObserver*) = 0;
> 
> This function should probably take const MutationRecordArray&.

Again, not up to me.

>> Source/WebCore/dom/MutationCallback.idl:36
>> +        [Custom] boolean handleEvent(in MutationRecordArray mutations, in WebKitMutationObserver observer);
> 
> Now that we have a custom binding, mutations should be of type Array.

This has to name a type, since it will be transcribed into C++ code by the code generators.

[Ryosuke Niwa]

Comment on attachment 110235 [details]
Added custom code for MutationObserverOptions, removed IDL

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

>>> Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:95

>> 
>> Is this a common practice?
> 
> It's something of a C-ism, but yeah, it's fairly common.  Given that we need to define a local variable to pass as a reference to getKeyValue, it should be scoped to a block, and using a do{}while forces the usage to end each invocation with a semicolon.

I don't quite get the use of defining option inside a block. Since we're checking the return value of getKeyValue and we aren't initializing option, it seems like we can reuse the same option for all calls to getKeyValue and setter. I think we should avoid using macros as much as possible.

[Adam Klein]

Created attachment 110384 [details]
Merge to r97071, kill SET_OPTION macro

[Gyuyoung Kim]

Comment on attachment 110384 [details]
Merge to r97071, kill SET_OPTION macro

Attachment 110384 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10015797

[Adam Klein]

Created attachment 110389 [details]
Update various build rules

[Adam Klein]

Created attachment 110394 [details]
Use ASSERT instead of CRASH

[Oliver Hunt]

Comment on attachment 110394 [details]
Use ASSERT instead of CRASH

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

r- due to the noted issues in the JSC bindings sorry :-/

> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:53
> +    if (!jsConstructor)
> +        return throwVMError(exec, createReferenceError(exec, "WebKitMutationObserver constructor callee is unavailable"));

This should be an ASSERT(jsConstructor);  and also a ASSERT(jsConstructor->inherits(&JSWebKitMutationObserverConstructor::s_info));

Have you seen a case where you have a null callee?

> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:60
> +    if (!exec->argument(0).isUndefinedOrNull()) {
> +        if (JSObject* object = exec->argument(0).getObject())

You should be able to simplify this to

JSObject* object = exec->argument(0).getObject();
if (!object)
    return throwVMError(exec, createTypeError(exec, "Argument is not an Object"));
RefPtr<MutationCallback> callback = JSMutationCallback::create(object, jsConstructor->globalObject());
return JSValue::encode(toJS(exec, jsConstructor->globalObject(), WebKitMutationObserver::create(callback.release())));

[Adam Klein]

Comment on attachment 110394 [details]
Use ASSERT instead of CRASH

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

>> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:53
>> +        return throwVMError(exec, createReferenceError(exec, "WebKitMutationObserver constructor callee is unavailable"));
> 
> This should be an ASSERT(jsConstructor);  and also a ASSERT(jsConstructor->inherits(&JSWebKitMutationObserverConstructor::s_info));
> 
> Have you seen a case where you have a null callee?

This was taken from JSAudioContextCustom.cpp (chosen as a random other custom constructor), but it doesn't seem like there's any reason to expect this to ever be null.  Perhaps the asserts aren't even necessary?  I don't see them elsewhere in JS*Custom.cpp.

I'm leaving the ASSERTs out for now (this is called only from generated code anyway).  Let me know if you'd prefer I keep them in.

>> Source/WebCore/bindings/js/JSWebKitMutationObserverCustom.cpp:60
>> +        if (JSObject* object = exec->argument(0).getObject())
> 
> You should be able to simplify this to
> 
> JSObject* object = exec->argument(0).getObject();
> if (!object)
>     return throwVMError(exec, createTypeError(exec, "Argument is not an Object"));
> RefPtr<MutationCallback> callback = JSMutationCallback::create(object, jsConstructor->globalObject());
> return JSValue::encode(toJS(exec, jsConstructor->globalObject(), WebKitMutationObserver::create(callback.release())));

Nice, done.

[Adam Klein]

Created attachment 110407 [details]
Simplify JSWebKitMutationObserverCustom.cpp

[Ryosuke Niwa]

Comment on attachment 110407 [details]
Simplify JSWebKitMutationObserverCustom.cpp

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

> LayoutTests/fast/mutation/observe-exceptions.html:21
> +    shouldThrow('observer.observe(document.body, undefined)');

Should we add a case where we pass an unknown option?

> Source/WebCore/dom/WebKitMutationObserver.cpp:57
> +    // FIXME: implement

Maybe use call notImplemented() instead?

[Adam Klein]

Comment on attachment 110407 [details]
Simplify JSWebKitMutationObserverCustom.cpp

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

>> LayoutTests/fast/mutation/observe-exceptions.html:21
>> +    shouldThrow('observer.observe(document.body, undefined)');
> 
> Should we add a case where we pass an unknown option?

The spec doesn't say anything about passing an object with unknown options (and I don't think it should).  But note that none of that parsing code is testable from JS yet (that'll come in the next patch when we start actually implementing observation).

>> Source/WebCore/dom/WebKitMutationObserver.cpp:57
>> +    // FIXME: implement
> 
> Maybe use call notImplemented() instead?

Done.

[Adam Klein]

Created attachment 110415 [details]
Use notImplemented instead of FIXME

[Ryosuke Niwa]

Non-binding part of the patch looks good to me. Waiting for someone else to review JSC & V8 binding code.

[Oliver Hunt]

Comment on attachment 110415 [details]
Use notImplemented instead of FIXME

r=me on the JSC bindings, setting r+ as apparently i'm the last person who still needed to review this.

[Adam Klein]

Created attachment 110447 [details]
Patch for landing

[Sam Weinig]

Do the options ever have to be returned anywhere?  If not, why do they need to be a refcounted class?

[Adam Klein]

(In reply to comment #32)
> Do the options ever have to be returned anywhere?  If not, why do they need to be a refcounted class?

They probably won't end up being refcounted, I just forgot to remove that when I got rid of the IDL file.  Can take of that detail in a followup (which may get rid of the class altogether in preference for a bitfield).

[WebKit Review Bot]

Comment on attachment 110447 [details]
Patch for landing

Rejecting attachment 110447 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
257ad45105c2fa20af02a38876a60a293353f719
r97130 = fd6dff2430dc9524fa845952a01f2085b59fb355
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10026487

[Adam Klein]

Committed r97159: <http://trac.webkit.org/changeset/97159>