[webkit-reviews] review denied: [Bug 54179] Change text-based <input> types to use the new shadow DOM model : [Attachment 84705] work-in-progress

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 54179: Change text-based <input> types to use the new shadow DOM model
https://bugs.webkit.org/show_bug.cgi?id=54179

Attachment 84705: work-in-progress
https://bugs.webkit.org/attachment.cgi?id=84705&action=review

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

Overall, I understand now why this patch is so large. I think it's fine to go
with this, provided that we have green bubbles on EWS. But I hope this is not
the end of it. Ideally, render objects shouldn't dip into shadow subtree to do
layout calculations. Instead the render objects inside the shadow tree should
provide this information to parent -- that's the way normal layout works.

> Source/WebCore/html/InputType.cpp:369
> +    shadowParent->parserAddChild(shadowChild);

What notification messages?

shadowParent->appendChild(shadowChild)

> Source/WebCore/html/SearchInputType.h:65
> +    RefPtr<TextControlInnerElement> m_innerBlock;
> +    RefPtr<SearchFieldResultsButtonElement> m_resultsButton;
> +    RefPtr<SearchFieldCancelButtonElement> m_cancelButton;

Do these need to be RefPtr?

> Source/WebCore/rendering/RenderTextControl.cpp:490
>  void RenderTextControl::forwardEvent(Event* event)
>  {
>      if (event->type() == eventNames().blurEvent || event->type() ==
eventNames().focusEvent)

Event forwarding should not be necessary now that shadowRoot is set properly.

> Source/WebCore/rendering/RenderTextControlMultiLine.cpp:110
> +/* $$$ remove

Probably didn't mean to leave this in.