[webkit-reviews] review granted: [Bug 78586] Event dispatching should use the composed shadow DOM tree instead of original DOM tree. : [Attachment 142219] ready for reviews. Refine a changelog.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 78586: Event dispatching should use the composed shadow DOM tree instead of
original DOM tree.
https://bugs.webkit.org/show_bug.cgi?id=78586

Attachment 142219: ready for reviews. Refine a changelog.
https://bugs.webkit.org/attachment.cgi?id=142219&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142219&action=review


Maybe it's because I spent a lot of time on this part of the spec, but the new
code reads _a lot_ better than the old one to me. Good work! A few nits -- up
to you to fix before or after landing.

> Source/WebCore/ChangeLog:90
> +	   I've also conducted a micro benchmark using both
> +	   Shadow-Free-DOM-Tree and DOM-Tree-With-Shadow-Host.
> +	   See https://bugs.webkit.org/show_bug.cgi?id=78586#c40 for the
results.
> +	   It seems that the new implementation has more capabilities, but
> +	   doesn't sacrifice a performance of event dispatching in either
cases.

You really didn't pull any punches on this ChangeLog, did you :)

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:212
> +    if (node->isShadowRoot()) {
> +	   const ShadowRoot* shadowRoot = toShadowRoot(node);

This would probably look a bit better as early return.

> Source/WebCore/dom/EventContext.cpp:50
> +static MouseEvent* toMouseEvent(Event* event)
> +{
> +    ASSERT(event && event->isMouseEvent());
> +    return static_cast<MouseEvent*>(event);
> +}

This seems like it belongs in MouseEvent.h?