[webkit-reviews] review granted: [Bug 61415] Avoid custom layout code of RenderTextControlSingleLine : [Attachment 97570] Patch 5

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 61415: Avoid custom layout code of RenderTextControlSingleLine
https://bugs.webkit.org/show_bug.cgi?id=61415

Attachment 97570: Patch 5
https://bugs.webkit.org/attachment.cgi?id=97570&action=review

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

This looks like a great incremental clean-up!

> Source/WebCore/html/HTMLInputElement.cpp:135
> +HTMLElement* HTMLInputElement::containerElement() const

All these shadow DOM accessors on HTMLInputElement are going away eventually,
right?

> Source/WebCore/html/TextFieldInputType.cpp:151
> +    bool shouldHaveSpinButton =
RenderTheme::themeForPage(document->page())->shouldHaveSpinButton(element());

This is super-weird. Why create a new theme here, rather than just querying
document->page()->theme()?

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:278
> +    innerTextRenderer->layoutIfNeeded();

This seems weird too... Are you going to eventually remove all this code? If
you don't intend to, I am afraid we are not better off here.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:312
>  bool RenderTextControlSingleLine::nodeAtPoint(const HitTestRequest& request,
HitTestResult& result, const IntPoint& pointInContainer, const IntPoint&
accumulatedOffset, HitTestAction hitTestAction)

nodeAtPoint looks completely unnecessary after shadow DOM conversion. All
elements should correctly react to the hit testing. Are you planning to remove
this altogether?

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:-437
> -    if (!event->isMouseEvent()) {
> -	   RenderTextControl::forwardEvent(event);
> -	   return;
> -    }
> -
> -#if ENABLE(INPUT_SPEECH)
> -    if (RenderBox* speechBox = speechButtonElement() ?
speechButtonElement()->renderBox() : 0) {
> -	   RenderBox* parent = innerTextRenderer ? innerTextRenderer : this;
> -	   FloatPoint pointInTextControlCoords =
parent->absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(),
false, true);
> -	   if
(speechBox->frameRect().contains(roundedIntPoint(pointInTextControlCoords))) {
> -	       speechButtonElement()->defaultEventHandler(event);
> -	       return;
> -	   }
> -    }
> -#endif
> -
> -    FloatPoint localPoint =
innerTextRenderer->absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLoc
ation(), false, true);
> -    int textRight = innerTextRenderer->borderBoxRect().maxX();
> -
> -    HTMLElement* resultsButton = resultsButtonElement();
> -    HTMLElement* cancelButton = cancelButtonElement();
> -    if (resultsButton && localPoint.x() <
innerTextRenderer->borderBoxRect().x())
> -	   resultsButton->defaultEventHandler(event);
> -    else if (cancelButton && localPoint.x() > textRight)
> -	   cancelButton->defaultEventHandler(event);
> -    else
> -	   RenderTextControl::forwardEvent(event);

Wooo weee! :)