[webkit-reviews] review granted: [Bug 206605] Prevent infinite recursion when upgrading custom elements : [Attachment 407380] Implements the new behavior

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 206605: Prevent infinite recursion when upgrading custom elements
https://bugs.webkit.org/show_bug.cgi?id=206605

Attachment 407380: Implements the new behavior

https://bugs.webkit.org/attachment.cgi?id=407380&action=review




--- Comment #2 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 407380
  --> https://bugs.webkit.org/attachment.cgi?id=407380
Implements the new behavior

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

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:203
> +    // Unlike spec, set element's custom element state to "failed" after
enqueueing post-upgrade reactions
> +    // to avoid hitting debug assertions in enqueuePostUpgradeReactions.
> +    element.setIsFailedCustomElementWithoutClearingReactionQueue();

Isn't this confusing? Wouldn't it be better to change the asserts?

> Source/WebCore/dom/Element.cpp:2468
> +#if ASSERT_ENABLED
> +    if (isFailedCustomElement()) {
> +	   auto* queue = elementRareData()->customElementReactionQueue();
> +	   ASSERT(queue);
> +	   ASSERT(queue->isEmpty() || queue->hasJustUpgradeReaction());
> +    } else
> +	   ASSERT(isDefinedCustomElement() ||
isCustomElementUpgradeCandidate());
> +#endif

Alternatively you could factor the validity tests into a boolean returning
function and ASSERT that.