Bug 68949 - [MutationObservers] Add stub implementation of WebKitMutationObserver
Summary: [MutationObservers] Add stub implementation of WebKitMutationObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (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: 2011-09-27 16:06 PDT by Adam Klein
Modified: 2011-10-11 10:00 PDT (History)
13 users (show)

See Also:


Attachments
Patch (63.41 KB, patch)
2011-09-27 16:16 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Add custom constructors for WebKitMutationObserver (74.39 KB, patch)
2011-10-03 11:37 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Fixed nits, got rid of MutationRecordArray (76.58 KB, patch)
2011-10-03 15:59 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Added custom code for MutationObserverOptions, removed IDL (76.61 KB, patch)
2011-10-07 17:08 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Merge to r97071, kill SET_OPTION macro (78.61 KB, patch)
2011-10-10 12:47 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Update various build rules (78.66 KB, patch)
2011-10-10 13:25 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Use ASSERT instead of CRASH (78.66 KB, patch)
2011-10-10 13:42 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Simplify JSWebKitMutationObserverCustom.cpp (78.42 KB, patch)
2011-10-10 14:49 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Use notImplemented instead of FIXME (78.45 KB, patch)
2011-10-10 15:16 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (78.56 KB, patch)
2011-10-10 17:19 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 2011-09-27 16:06:56 PDT
[MutationObservers] Add stub implementation of WebKitMutationObserver
Comment 1 Adam Klein 2011-09-27 16:16:23 PDT
Created attachment 108923 [details]
Patch
Comment 2 Ryosuke Niwa 2011-09-27 16:49:56 PDT
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?
Comment 3 Adam Klein 2011-09-27 16:54:18 PDT
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.
Comment 4 Adam Klein 2011-10-03 11:37:55 PDT
Created attachment 109502 [details]
Add custom constructors for WebKitMutationObserver
Comment 5 Ryosuke Niwa 2011-10-03 11:53:03 PDT
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.
Comment 6 Adam Klein 2011-10-03 12:08:37 PDT
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.
Comment 7 Darin Adler 2011-10-03 12:21:23 PDT
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.
Comment 8 Ryosuke Niwa 2011-10-03 12:27:36 PDT
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.
Comment 9 Daniel Bates 2011-10-03 12:34:23 PDT
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?
Comment 10 Sam Weinig 2011-10-03 13:32:58 PDT
If WebKitMutationObserverOptions is really supposed to be a dictionary, I don't think it should be included as it currently is.
Comment 11 Adam Klein 2011-10-03 15:59:09 PDT
Created attachment 109545 [details]
Fixed nits, got rid of MutationRecordArray
Comment 12 Adam Klein 2011-10-03 16:02:37 PDT
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.
Comment 13 Ryosuke Niwa 2011-10-03 16:21:02 PDT
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.
Comment 14 Adam Klein 2011-10-07 17:08:51 PDT
Created attachment 110235 [details]
Added custom code for MutationObserverOptions, removed IDL
Comment 15 Ryosuke Niwa 2011-10-10 11:48:29 PDT
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.
Comment 16 Ryosuke Niwa 2011-10-10 11:49:43 PDT
Can we somehow make EWS green? I'd like to make sure we're not breaking any builds.
Comment 17 Adam Klein 2011-10-10 11:59:38 PDT
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.
Comment 18 Ryosuke Niwa 2011-10-10 12:08:13 PDT
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.
Comment 19 Adam Klein 2011-10-10 12:47:35 PDT
Created attachment 110384 [details]
Merge to r97071, kill SET_OPTION macro
Comment 20 Gyuyoung Kim 2011-10-10 13:17:56 PDT
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
Comment 21 Adam Klein 2011-10-10 13:25:20 PDT
Created attachment 110389 [details]
Update various build rules
Comment 22 Adam Klein 2011-10-10 13:42:51 PDT
Created attachment 110394 [details]
Use ASSERT instead of CRASH
Comment 23 Oliver Hunt 2011-10-10 14:16:09 PDT
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())));
Comment 24 Adam Klein 2011-10-10 14:48:18 PDT
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.
Comment 25 Adam Klein 2011-10-10 14:49:31 PDT
Created attachment 110407 [details]
Simplify JSWebKitMutationObserverCustom.cpp
Comment 26 Ryosuke Niwa 2011-10-10 15:03:41 PDT
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?
Comment 27 Adam Klein 2011-10-10 15:16:06 PDT
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.
Comment 28 Adam Klein 2011-10-10 15:16:59 PDT
Created attachment 110415 [details]
Use notImplemented instead of FIXME
Comment 29 Ryosuke Niwa 2011-10-10 16:29:19 PDT
Non-binding part of the patch looks good to me. Waiting for someone else to review JSC & V8 binding code.
Comment 30 Oliver Hunt 2011-10-10 16:36:27 PDT
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.
Comment 31 Adam Klein 2011-10-10 17:19:13 PDT
Created attachment 110447 [details]
Patch for landing
Comment 32 Sam Weinig 2011-10-10 18:19:42 PDT
Do the options ever have to be returned anywhere?  If not, why do they need to be a refcounted class?
Comment 33 Adam Klein 2011-10-10 18:34:01 PDT
(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).
Comment 34 WebKit Review Bot 2011-10-11 00:53:29 PDT
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
Comment 35 Adam Klein 2011-10-11 09:56:09 PDT
Committed r97159: <http://trac.webkit.org/changeset/97159>