Summary: | [Forms] Move search field related code to RenderSearchField from RenderTextControlSingleLine | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | adele, eric, japhet, jochen, joepeck, jonlee, rakuco, tkent, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 89155 | ||||||||||||||||||||||||
Bug Blocks: | 88979 | ||||||||||||||||||||||||
Attachments: |
|
Description
yosin
2012-06-13 03:08:05 PDT
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> All reviewed patches have been landed. Closing bug. |