This is part of set RenderTextControlSingleLine free from HTMLInputElement.
Created attachment 147533 [details] Patch 1
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.
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().
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
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.
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 > + > +2012-06-14 Yoshifumi Inoue <yosin@chromium.org> > + 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.
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
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.
Comment on attachment 147771 [details] Patch 7 ok
Comment on attachment 147771 [details] Patch 7 Clearing flags on attachment: 147771 Committed r120432: <http://trac.webkit.org/changeset/120432>
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>