Bug 164206 – Move Element, NamedNodeMap, and DOMStringMap from ExceptionCode to Exception

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

[Darin Adler]

Move Element, NamedNodeMap, and DOMStringMap from ExceptionCode to Exception

[Darin Adler]

Created attachment 293351 [details]
Patch

[Chris Dumez]

Comment on attachment 293351 [details]
Patch

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

r=me but looks like GTK / Win are possibly broken.

> Source/WebCore/ChangeLog:12
> +        true any time an exception is raised. Since true tealls the caller to return

"tells"

> Source/WebCore/ChangeLog:26
> +        removeAttributeNode. Changed to use is<Document> and is<Attr> instead of

Why is this better for performance? The common case is likely an Element. For an Element, we used to do a single virtual function call (nodeType()), and now we have 2 branches:
- isDocumentNode() which is non-virtual
- isAttributeNode() which is virtual

It looks to me that only adopting a Document became a bit faster but this is likely not a common case.

> Source/WebCore/dom/DatasetDOMStringMap.cpp:196
> +ExceptionOr<void> DatasetDOMStringMap::setItem(const String& name, const String& value) const

I don't feel strongly but I personally feel making such setters const is a bit confusing. I read your explanation in the Changelog but even so.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:669
> +    auto result = parsedElement.get().setInnerHTML("<span " + text + "></span>");

don't we usually use parsedElement->setInnerHTML() ?

[Darin Adler]

Comment on attachment 293351 [details]
Patch

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

>> Source/WebCore/ChangeLog:26
>> +        removeAttributeNode. Changed to use is<Document> and is<Attr> instead of
> 
> Why is this better for performance? The common case is likely an Element. For an Element, we used to do a single virtual function call (nodeType()), and now we have 2 branches:
> - isDocumentNode() which is non-virtual
> - isAttributeNode() which is virtual
> 
> It looks to me that only adopting a Document became a bit faster but this is likely not a common case.

Good point, I will change it back. I did not realize that isAttributeNode was virtual.

>> Source/WebCore/dom/DatasetDOMStringMap.cpp:196
>> +ExceptionOr<void> DatasetDOMStringMap::setItem(const String& name, const String& value) const
> 
> I don't feel strongly but I personally feel making such setters const is a bit confusing. I read your explanation in the Changelog but even so.

I’m going to leave it this way. I agree there are arguments both ways. I think it’s better for these wrappers to be more true to their status as wrappers and not pretend they actually contain the data, but we could go the other direction too I suppose and try to do "transitive const-ness".

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:669
>> +    auto result = parsedElement.get().setInnerHTML("<span " + text + "></span>");
> 
> don't we usually use parsedElement->setInnerHTML() ?

The advantage of .get(). over -> is that it doesn’t look like a pointer dereference. I’m not sure we have standardized either way. Kling suggested that we prefer .get(). but we didn’t really firm this up.

[Darin Adler]

Comment on attachment 293351 [details]
Patch

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

>>> Source/WebCore/dom/DatasetDOMStringMap.cpp:196
>>> +ExceptionOr<void> DatasetDOMStringMap::setItem(const String& name, const String& value) const
>> 
>> I don't feel strongly but I personally feel making such setters const is a bit confusing. I read your explanation in the Changelog but even so.
> 
> I’m going to leave it this way. I agree there are arguments both ways. I think it’s better for these wrappers to be more true to their status as wrappers and not pretend they actually contain the data, but we could go the other direction too I suppose and try to do "transitive const-ness".

No, changed my mind. I’ll do it the other way.

[Darin Adler]

Created attachment 293363 [details]
Patch

[Darin Adler]

Created attachment 293368 [details]
Patch

[Darin Adler]

Created attachment 293369 [details]
Patch

[Darin Adler]

Created attachment 293370 [details]
Patch

[Darin Adler]

Committed r208135: <http://trac.webkit.org/changeset/208135>