[webkit-reviews] review granted: [Bug 224174] Assert failure in isCloneInShadowTreeOfSVGUseElement : [Attachment 425138] Fixed change logs

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 224174: Assert failure in isCloneInShadowTreeOfSVGUseElement
https://bugs.webkit.org/show_bug.cgi?id=224174

Attachment 425138: Fixed change logs

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 425138
  --> https://bugs.webkit.org/attachment.cgi?id=425138
Fixed change logs

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

> Source/WebCore/dom/ScopedEventQueue.cpp:59
> +    if (event.event->eventInterface() == MutationEventInterfaceType &&
event.target->isInShadowTree())

All shadow trees? The other half of this patch is specific to user agent shadow
trees. Do we have tests to verify these events don’t get dispatched for the
various types of shadow trees? Should we? Is this just an optimization or an
important correctness fix?

This seems to have an effect beyond what is mentioned in the change log.

> Source/WebCore/svg/SVGElement.cpp:255
> +	   if (downcast<ShadowRoot>(oldParentOfRemovedTree).mode() ==
ShadowRootMode::UserAgent)

I would have used just one longer if statement instead of two nested if
statements.

Separately, a helper function named isUserAgenrShadowRoot would make this
easier to read and would be consistent with functions like
isInUserAgentShadowTree.