Bug 88980 – [Forms] Move search field related code to RenderSearchField from RenderTextControlSingleLine

• Component: Forms
• Status: RESOLVED FIXED

This is part of set RenderTextControlSingleLine free from HTMLInputElement.

Created attachment 147533 [details]
Patch 1

[Build Bot]

Comment on attachment 147533 [details]
Patch 1

Attachment 147533 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12950664

Created attachment 147545 [details]
Patch 2

Comment on attachment 147545 [details]
Patch 2

Could you review this patch?
Thanks in advance.

* Build locally on CR-Linux and Mac
* Move around codes and small changes for new class RenderSearchFiled.

[Kent Tamura]

Comment on attachment 147545 [details]
Patch 2

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

> Source/WebCore/html/HTMLInputElement.cpp:53
> +#include "RenderSearchField.h"

Using RenderSearchField in HTMLInputElement.cpp is not good.
We should move such code to SearchInputType.

> Source/WebCore/html/TextFieldInputType.cpp:203
>  RenderObject* TextFieldInputType::createRenderer(RenderArena* arena, RenderStyle*) const
>  {
> -    return new (arena) RenderTextControlSingleLine(element());
> +    return isSearchField() ? new (arena) RenderSearchField(element()) : new (arena) RenderTextControlSingleLine(element());

Should introduce SearchInputType::createRenderer().

[Kent Tamura]

We need to make sure copied code has correct revision history. So you need to
* Work with SVN checkout, or
* Make another patch only for copying RenderTextControlSingleLine.{cpp,h} to RenderSearchField.{cpp,h} before this.

Created attachment 147722 [details]
Patch 3

Comment on attachment 147722 [details]
Patch 3

Could you review this patch?
Thanks in advance.

* Local build on CR-Linux and Mac.
* Changed since last patch:
** Changes of RenderSearchField are diffed from copied RenderTextControlSingleLine
** Add InputType::addSearchResult
** Add SearchInputType::createRenderer

[Kent Tamura]

Comment on attachment 147722 [details]
Patch 3

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

> Source/WebCore/html/InputType.cpp:529
> +void InputType::addSearchResult()
> +{
> +    ASSERT_NOT_REACHED();
> +}

I prefer removing ASSERT_NO_REACHED() here and isSearchField() check in loader/FormSubmission.cpp because we can reduce two virtual calls to one.

Created attachment 147730 [details]
Patch 4

Comment on attachment 147730 [details]
Patch 4

Could you review this patch?
Thanks in advance.

== Changes from last review ==
* InputType::addSearchResult: Remove ASSERT_NOT_REACHED
* FormSubmission::create: Call HTMLInputElement::addSearchResult w/o isSearchField gurad

(In reply to comment #9)
> (From update of attachment 147722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147722&action=review
> 
> > Source/WebCore/html/InputType.cpp:529
> > +void InputType::addSearchResult()
> > +{
> > +    ASSERT_NOT_REACHED();
> > +}
> 
> I prefer removing ASSERT_NO_REACHED() here and isSearchField() check in loader/FormSubmission.cpp because we can reduce two virtual calls to one.

For loader/FormSubmission.cpp, create method, I would like to introduce
HTMLInputElement::appendToFormValue and replace below fragment:

            HTMLInputElement* input = static_cast<HTMLInputElement*>(control);
            if (input->isTextField()) {
                formValues.append(pair<String, String>(input->name().string(), input->value()));
                input->addSearchResult();
            }

New methods are

HTMLInputElement::appendFormValue(Vector<pair<String, String> > formValues)
{
    m_inputType->appendFormValue(formValues);
}


InputType::appendFormValue(Vector<pair<String, String> > formValues)
{
    // nothing to do
}

TextFieldInputType::appendFormValue(Vector<pair<String, String> > formValues)
{
    formValues.append(pair<String, String>(element()->name().string(), element()->value()));
}

SearchInptType::appendFormValue(Vector<pair<String, String> > formValues)
{
    TextFieldInputType::appendFormValue(formValues);
    addSearchResult();
}

In this way, I would like to introduce FormValues class, Vector<pair<String, String>> is little bit long.

[Kent Tamura]

Comment on attachment 147730 [details]
Patch 4

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

> Source/WebCore/ChangeLog:77
> +        * rendering/RenderingAllInOne.cpp: Added RenderSearchField.cpp
> +
> [phone]  Yoshifumi Inoue  <[e-mail]>
> +

There are two ChangeLog entries.

Created attachment 147732 [details]
Patch 5

Comment on attachment 147732 [details]
Patch 5

Oops, sorry... m(_ _)m

Could you review this patch?
Thanks in advance.

== Changes from last review ==
* Remove duplicated ChangeLog entry.

[Kent Tamura]

Comment on attachment 147732 [details]
Patch 5

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

> Source/WebCore/rendering/RenderSearchField.cpp:-459
> -
> -    if (m_searchPopupIsVisible)
> -        m_searchPopup->popupMenu()->updateFromElement();
> -}

RenderSearchField should have this code.

Created attachment 147741 [details]
Patch 6

Comment on attachment 147741 [details]
Patch 6

Could you review this patch?
Thanks in advance.

== Changes from last review ==
* Recover RenderSearchField::updateFromElement and changed for RenderSearchField.
* Add updateFromElement into RenderSearchField.h

[Kent Tamura]

Comment on attachment 147741 [details]
Patch 6

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

> Source/WebCore/rendering/RenderSearchField.cpp:-448
> -LayoutUnit RenderTextControlSingleLine::computeControlHeight(LayoutUnit lineHeight, LayoutUnit nonContentHeight) const
> -{
> -    HTMLElement* resultsButton = resultsButtonElement();
> -    if (RenderBox* resultsRenderer = resultsButton ? resultsButton->renderBox() : 0) {
> -        resultsRenderer->computeLogicalHeight();
> -        nonContentHeight = max(nonContentHeight, resultsRenderer->borderAndPaddingHeight() + resultsRenderer->marginHeight());
> -        lineHeight = max(lineHeight, resultsRenderer->height());
> -    }
> -    HTMLElement* cancelButton = cancelButtonElement();
> -    if (RenderBox* cancelRenderer = cancelButton ? cancelButton->renderBox() : 0) {
> -        cancelRenderer->computeLogicalHeight();
> -        nonContentHeight = max(nonContentHeight, cancelRenderer->borderAndPaddingHeight() + cancelRenderer->marginHeight());
> -        lineHeight = max(lineHeight, cancelRenderer->height());
> -    }
> -
> -    return lineHeight + nonContentHeight;
> -}

resultsButtonElement() and cancelButtonElement() are search-specific elements.  This function should not be removed.

> Source/WebCore/rendering/RenderSearchField.cpp:-518
> -void RenderTextControlSingleLine::updateCancelButtonVisibility() const
> -{
> -    RenderObject* cancelButtonRenderer = cancelButtonElement()->renderer();
> -    if (!cancelButtonRenderer)
> -        return;
> -
> -    const RenderStyle* curStyle = cancelButtonRenderer->style();
> -    EVisibility buttonVisibility = visibilityForCancelButton();
> -    if (curStyle->visibility() == buttonVisibility)
> -        return;
> -
> -    RefPtr<RenderStyle> cancelButtonStyle = RenderStyle::clone(curStyle);
> -    cancelButtonStyle->setVisibility(buttonVisibility);
> -    cancelButtonRenderer->setStyle(cancelButtonStyle);
> -}
> -
> -EVisibility RenderTextControlSingleLine::visibilityForCancelButton() const
> -{
> -    return (style()->visibility() == HIDDEN || inputElement()->value().isEmpty()) ? HIDDEN : VISIBLE;
> -}

cancelButtonElement() is search-specific.  This should not be removed.

> Source/WebCore/rendering/RenderSearchField.cpp:-529
> -const AtomicString& RenderTextControlSingleLine::autosaveName() const
> -{
> -    return static_cast<Element*>(node())->getAttribute(autosaveAttr);
> -}

autosaveName() is search-specific.  This should not be removed.

Created attachment 147771 [details]
Patch 7

Comment on attachment 147771 [details]
Patch 7

Could you review this patch?
Thanks in advance and sorry for many patches.

== Changes since last review ==
* Move cancel and results button related codes to RenderSearchField
* Move search field related code to SearchInputType from HTMLInputElement
** now HTMLInputElement free from search field.

[Kent Tamura]

Comment on attachment 147771 [details]
Patch 7

ok

[WebKit Review Bot]

Comment on attachment 147771 [details]
Patch 7

Clearing flags on attachment: 147771

Committed r120432: <http://trac.webkit.org/changeset/120432>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

Reverted r120432 for reason:

Failed to copy merge history to RenderSerachField.{cpp,h}

Committed r120557: <http://trac.webkit.org/changeset/120557>

Created attachment 148048 [details]
Patch 8

Created attachment 148050 [details]
Patch 9

Created attachment 148051 [details]
Patch 10

Comment on attachment 148051 [details]
Patch 10

Clearing flags on attachment: 148051

Committed r120569: <http://trac.webkit.org/changeset/120569>

All reviewed patches have been landed.  Closing bug.