[webkit-reviews] review granted: [Bug 227189] Skip shadow-root creation for input element if it is not necessary : [Attachment 431811] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 227189: Skip shadow-root creation for input element if it is not necessary
https://bugs.webkit.org/show_bug.cgi?id=227189

Attachment 431811: Patch

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




--- Comment #3 from Maciej Stachowiak <mjs at apple.com> ---
Comment on attachment 431811
  --> https://bugs.webkit.org/attachment.cgi?id=431811
Patch

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

Many nits but generally seems ok. r=me but please consider the individual
comments.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:309
> +    ASSERT(needsShadowSubtree());

There seem to be a lot of asserts of needsShadowSubtree() all over the place
and I'm not sure what they are there for. needsShadowSubtree() is fixed based
on input type, so there can't be an inconsistency at runtime unless someone
forgot to add a type to the list of types. I could see having a compile-time
assert somewhere maybe, but why have this in every implementation of
createShadowSubtreeAndUpdateInnerTextElementEditability and every input type
constructor?

> Source/WebCore/html/HTMLInputElement.cpp:87
> -class ListAttributeTargetObserver : IdTargetObserver {
> +class ListAttributeTargetObserver final : public IdTargetObserver {

This seems unrelated to the rest of the change and isn't documented in the
ChangeLog.

> Source/WebCore/html/HTMLInputElement.cpp:138
> +	   ASSERT(m_inputType->needsShadowSubtree());

Extra weird to assert here considering we would have asserted in the
constructor (and nothing seems to depend on this condition).

> Source/WebCore/html/HTMLInputElement.cpp:164
> +void HTMLInputElement::createShadowSubtree()

Why this rename? Other elements have didAddUserAgentShadowRoot and not
createShadowSubtree. didAddUserAgentShadowRoot is called automatically from
addShadowRoot, so with this method at the original name, the explicit calls to
createShadowSubtree should be mostly not necessary (except in cases where it's
known there is already a shadow root; is the goal here to simplicy that logic?

> Source/WebCore/html/HTMLInputElement.cpp:586
> -   
m_inputType->createShadowSubtreeAndUpdateInnerTextElementEditability(m_parsingI
nProgress ? ChildChange::Source::Parser : ChildChange::Source::API,
isInnerTextElementEditable());
> +    if (m_inputType->needsShadowSubtree()) {
> +	   ensureUserAgentShadowRoot();
> +	   createShadowSubtree();
> +    }

I guess if tests pass it must be ok, but the old function name suggest there's
a need to do something related to inner text element editability, but I don't
think the new code will do that.

> Source/WebCore/html/HTMLInputElement.cpp:1279
> +    // Some input types only need shadow roots to hide any children that may
> +    // have been appended by script. For such types, shadow roots are lazily
> +    // created when children are added for the first time.
> +    ensureUserAgentShadowRoot();

I'm not sure it's necessary to have a shadow root to hide children and it might
not be the most efficient way to do so. form controls managed to hide their
children before there was such a thing as shadow DOM. I wonder how we did it
before. I guess its of if it actually works.

> Source/WebCore/html/HTMLInputElement.h:139
> -    RenderStyle createInnerTextStyle(const RenderStyle&) override;
> +    RenderStyle createInnerTextStyle(const RenderStyle&) final;

Seems unrelated to the change and isn't documented in ChangeLog.

> Source/WebCore/html/HTMLInputElement.h:272
> -    bool willRespondToMouseClickEvents() override;
> +    bool willRespondToMouseClickEvents() final;

Seems unrelated to the change and isn't documented in ChangeLog.

> Source/WebCore/html/HTMLInputElement.h:351
> -    void defaultEventHandler(Event&) override;
> +    void defaultEventHandler(Event&) final;

Seems unrelated to the change and isn't documented in ChangeLog.

> LayoutTests/fast/forms/checkbox-child-hidden-expected.html:5
> +<input type="checkbox" id="cb" />

The / at the end of the <input> element is unnecessary and is invalid in HTML.
(It would have been both necessary and valid in XHTML).