Bug 149192 – WebIDL: Rename [ReturnNewObject] to [NewObject] and use it more consistently in DOM

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Rename [ReturnNewObject] to [NewObject] and use it more consistently in DOM.

This aligns our IDL extended attribute naming with standard Web IDL:
https://heycam.github.io/webidl/#NewObject

We already have [ReturnedNewObject] in most places that the DOM specification uses [NewObject] but we are missing a few so I'll fix this as well:
https://dom.spec.whatwg.org/#interface-document

[Chris Dumez]

Created attachment 261266 [details]
WIP Patch

[Chris Dumez]

Created attachment 261283 [details]
Patch

[Chris Dumez]

Created attachment 261285 [details]
Patch

[Chris Dumez]

Created attachment 261286 [details]
Patch

[Chris Dumez]

Created attachment 261287 [details]
Patch

[Chris Dumez]

Created attachment 261288 [details]
Patch

[Chris Dumez]

Created attachment 261319 [details]
Patch

[Darin Adler]

Comment on attachment 261319 [details]
Patch

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

I’m a little surprised about what [NewObject] is for in WebIDL. We invented [ReturnNewObject] as a performance optimization for the bindings code; given a guarantee that the result is a new object it can skip the step of looking for an existing DOM wrapper. It was always supposed to be safe to omit it, but more efficient to use it any time the function has an ironclad guaranteed that it won’t return an existing object.

Since [ReturnNewObject] does not have any effect on the semantics of the function, I’m surprised to find a similar WebIDL attribute! This attribute was not intended as part of the interface but just as an annotation for the bindings generator. I wonder if the WebIDL attribute was possibly inspired by [ReturnNewObject] and perhaps a misunderstanding of its purpose? I’m not sure we should be standardizing aspects of IDL that are engine implementation details, although it’s nice to be able to take advantage of them.

Since a lot of this patch is about efficiency, sure would be nice if we had some way to test the performance benefits.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:82
> +static inline JSValue createNewDocumentWrapper(ExecState* exec, JSDOMGlobalObject* globalObject, Document& document)

Whenever possible the new functions should use references for “guaranteed non-null” arguments such as exec and globalObject. Similarly, we like to use “state” rather than “exec” as the name in new code.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:94
> +    if (!document.frame()) {

Just moved code, but: Checking frame for null seems subtly wrong here. I’d like to get a deeper understanding of exactly why this is the right check.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:97
> +        size_t nodeCount = 0;
> +        for (Node* n = &document; n; n = NodeTraversal::next(*n))
> +            ++nodeCount;

Just moved code, but: Isn’t there a tighter way of writing this with the node iterator objects?

> Source/WebCore/bindings/scripts/test/JS/JSTestException.cpp:215
>  #endif
> +JSC::JSValue toJSNewlyCreated(JSC::ExecState*, JSDOMGlobalObject* globalObject, TestException* impl)

Would be nice if the code generator put a blank line here.

[Chris Dumez]

Comment on attachment 261319 [details]
Patch

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

I agree it is a bit surprising that WebIDL has such attribute. However, since it does, I think it makes sense to align the naming. WebIDL specifies that we should return a new Object every time. WebKit specifies that we can bypass the existing wrapper check since we know it is a new Object and there won't be one. The good news is that if we should be able to use [NewObject] everywhere that the specification has it because if the optimization would be invalid, it would likely mean that we don't comply with the specification (i.e. sometimes return the same object). However, we can also use [NewObject] is places where the specification does not have it if we know WebKit always returns a new object and it is hot code. Also note that if someone tries to use [NewObject] for an API where we don't always return a new Object, then we will hit an assertion in createNewWrapper():
ASSERT(!getCachedWrapper(globalObject->world(), domObject));

>> Source/WebCore/bindings/js/JSDocumentCustom.cpp:82
>> +static inline JSValue createNewDocumentWrapper(ExecState* exec, JSDOMGlobalObject* globalObject, Document& document)
> 
> Whenever possible the new functions should use references for “guaranteed non-null” arguments such as exec and globalObject. Similarly, we like to use “state” rather than “exec” as the name in new code.

OK

>> Source/WebCore/bindings/js/JSDocumentCustom.cpp:94
>> +    if (!document.frame()) {
> 
> Just moved code, but: Checking frame for null seems subtly wrong here. I’d like to get a deeper understanding of exactly why this is the right check.

Documents in the PageCache do not have a frame so this check will at least match those. Based on the comment above, it looks like the intention was Page-Cache related.

>> Source/WebCore/bindings/js/JSDocumentCustom.cpp:97
>> +            ++nodeCount;
> 
> Just moved code, but: Isn’t there a tighter way of writing this with the node iterator objects?

Do you mean the DOM NodeIterator type? I haven't found any Native code using it internally.
We do have a lot of other fancy DOM traversal iterator types but those work with Elements, not Nodes.

>> Source/WebCore/bindings/scripts/test/JS/JSTestException.cpp:215
>> +JSC::JSValue toJSNewlyCreated(JSC::ExecState*, JSDOMGlobalObject* globalObject, TestException* impl)
> 
> Would be nice if the code generator put a blank line here.

Ok.

[Chris Dumez]

Created attachment 261324 [details]
Patch

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 261319 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261319&action=review
> 
> I’m a little surprised about what [NewObject] is for in WebIDL. We invented
> [ReturnNewObject] as a performance optimization for the bindings code; given
> a guarantee that the result is a new object it can skip the step of looking
> for an existing DOM wrapper. It was always supposed to be safe to omit it,
> but more efficient to use it any time the function has an ironclad
> guaranteed that it won’t return an existing object.
> 
> Since [ReturnNewObject] does not have any effect on the semantics of the
> function, I’m surprised to find a similar WebIDL attribute! This attribute
> was not intended as part of the interface but just as an annotation for the
> bindings generator. I wonder if the WebIDL attribute was possibly inspired
> by [ReturnNewObject] and perhaps a misunderstanding of its purpose? I’m not
> sure we should be standardizing aspects of IDL that are engine
> implementation details, although it’s nice to be able to take advantage of
> them.
> 
> Since a lot of this patch is about efficiency, sure would be nice if we had
> some way to test the performance benefits.

I ran Speedometer and Dromaeo. Mostly no impact but there seems to be a nice ~5.7% progression on Dromaeo's jslib-event-prototype.html. This test is using prototype's Element.fire [1] which uses Document.createEvent() internally. Note that Document.createEvent() is one of the operations for which I added [NewObject].

[1] http://api.prototypejs.org/dom/Element/fire/

[WebKit Commit Bot]

Comment on attachment 261324 [details]
Patch

Clearing flags on attachment: 261324

Committed r189881: <http://trac.webkit.org/changeset/189881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.