WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
88980
[Forms] Move search field related code to RenderSearchField from RenderTextControlSingleLine
https://bugs.webkit.org/show_bug.cgi?id=88980
Summary
[Forms] Move search field related code to RenderSearchField from RenderTextCo...
yosin
Reported
2012-06-13 03:08:05 PDT
This is part of set RenderTextControlSingleLine free from HTMLInputElement.
Attachments
Patch 1
(46.78 KB, patch)
2012-06-14 02:38 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 2
(47.40 KB, patch)
2012-06-14 03:30 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 3
(66.22 KB, patch)
2012-06-14 21:15 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 4
(71.96 KB, patch)
2012-06-14 22:09 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 5
(67.18 KB, patch)
2012-06-14 22:31 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 6
(67.88 KB, patch)
2012-06-14 23:15 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 7
(74.43 KB, patch)
2012-06-15 01:10 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 8
(47.73 KB, patch)
2012-06-17 21:39 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 9
(74.64 KB, patch)
2012-06-17 21:52 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 10
(74.63 KB, patch)
2012-06-17 21:54 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
yosin
Comment 1
2012-06-14 02:38:15 PDT
Created
attachment 147533
[details]
Patch 1
Build Bot
Comment 2
2012-06-14 03:18:06 PDT
Comment on
attachment 147533
[details]
Patch 1
Attachment 147533
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12950664
yosin
Comment 3
2012-06-14 03:30:07 PDT
Created
attachment 147545
[details]
Patch 2
yosin
Comment 4
2012-06-14 05:31:08 PDT
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 5
2012-06-14 05:34:41 PDT
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
Comment 6
2012-06-14 18:45:03 PDT
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.
yosin
Comment 7
2012-06-14 21:15:45 PDT
Created
attachment 147722
[details]
Patch 3
yosin
Comment 8
2012-06-14 21:47:08 PDT
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 9
2012-06-14 21:53:17 PDT
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.
yosin
Comment 10
2012-06-14 22:09:58 PDT
Created
attachment 147730
[details]
Patch 4
yosin
Comment 11
2012-06-14 22:11:30 PDT
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
yosin
Comment 12
2012-06-14 22:16:02 PDT
(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 13
2012-06-14 22:19:51 PDT
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.
yosin
Comment 14
2012-06-14 22:31:17 PDT
Created
attachment 147732
[details]
Patch 5
yosin
Comment 15
2012-06-14 22:32:12 PDT
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 16
2012-06-14 22:49:59 PDT
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.
yosin
Comment 17
2012-06-14 23:15:30 PDT
Created
attachment 147741
[details]
Patch 6
yosin
Comment 18
2012-06-14 23:17:23 PDT
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 19
2012-06-14 23:32:53 PDT
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.
yosin
Comment 20
2012-06-15 01:10:05 PDT
Created
attachment 147771
[details]
Patch 7
yosin
Comment 21
2012-06-15 02:05:22 PDT
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 22
2012-06-15 02:15:05 PDT
Comment on
attachment 147771
[details]
Patch 7 ok
WebKit Review Bot
Comment 23
2012-06-15 02:53:42 PDT
Comment on
attachment 147771
[details]
Patch 7 Clearing flags on attachment: 147771 Committed
r120432
: <
http://trac.webkit.org/changeset/120432
>
WebKit Review Bot
Comment 24
2012-06-15 02:53:48 PDT
All reviewed patches have been landed. Closing bug.
yosin
Comment 25
2012-06-17 19:06:53 PDT
Reverted
r120432
for reason: Failed to copy merge history to RenderSerachField.{cpp,h} Committed
r120557
: <
http://trac.webkit.org/changeset/120557
>
yosin
Comment 26
2012-06-17 21:39:25 PDT
Created
attachment 148048
[details]
Patch 8
yosin
Comment 27
2012-06-17 21:52:06 PDT
Created
attachment 148050
[details]
Patch 9
yosin
Comment 28
2012-06-17 21:54:45 PDT
Created
attachment 148051
[details]
Patch 10
yosin
Comment 29
2012-06-17 21:57:14 PDT
Comment on
attachment 148051
[details]
Patch 10 Clearing flags on attachment: 148051 Committed
r120569
: <
http://trac.webkit.org/changeset/120569
>
yosin
Comment 30
2012-06-17 21:57:22 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug