Bug 162996 – Upgrading and constructing element should always report exception instead of rethrowing

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

When upgrading or constructing an element synchronously inside the HTML parser and document.createElement,
we must report the exception instead of rethrowing the exception.

[Ryosuke Niwa]

Created attachment 290782 [details]
Fixes the bug

[Radar WebKit Bug Importer]

<rdar://problem/28647657>

[Darin Adler]

Comment on attachment 290782 [details]
Fixes the bug

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

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:72
> +    return element.get();

The get() here means we get reference count churn. Can we compile a version that transfers ownership instead? Maybe WTFMove(element) works? If not, then we should fix the Ref class so it does.

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:91
> +    RefPtr<Element> element = constructCustomElementSynchronously(document, vm, state, m_constructor.get(), localName);

Can we use auto here instead of RefPtr?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:200
> +        Ref<Element> newElement = elementInterface.constructElementWithFallback(*document(), constructionData->name);

Can we use auto here?

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #3)
> Comment on attachment 290782 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290782&action=review
> 
> > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:72
> > +    return element.get();
> 
> The get() here means we get reference count churn. Can we compile a version
> that transfers ownership instead? Maybe WTFMove(element) works? If not, then
> we should fix the Ref class so it does.

WTFMove(element) fails because Ref<HTMLUnknownElement>&& can be adopted by Ref<Element>&& :(  I agree we should fix that, not in this patch though.

> 
> > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:91
> > +    RefPtr<Element> element = constructCustomElementSynchronously(document, vm, state, m_constructor.get(), localName);
> 
> Can we use auto here instead of RefPtr?

Sure.

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:200
> > +        Ref<Element> newElement = elementInterface.constructElementWithFallback(*document(), constructionData->name);
> 
> Can we use auto here?

Sure.

[Ryosuke Niwa]

Created attachment 290872 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290872 [details]
Patch for landing

Clearing flags on attachment: 290872

Committed r206890: <http://trac.webkit.org/changeset/206890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.