Summary: | Add clear button to date/time input types | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, dglazkov, eric, esprehn+autocc, gyuyoung.kim, macpherson, menard, mifenton, ojan.autocc, rakuco, rego+ews, rniwa, roger_fong, tkent, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 111549, 109439 | ||||||||||||||||
Attachments: |
|
Description
Keishi Hattori
2013-03-04 07:04:07 PST
Created attachment 191250 [details]
Patch
Attachment 191250 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-spinbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/resources/common-clearbutton-change-and-input-events.js', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/datetimelocal/datetimelocal-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/datetimelocal/datetimelocal-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-locale-hebrew-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/css/html.css', u'Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp', u'Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.h', u'Source/WebCore/html/DateTimeFieldsState.cpp', u'Source/WebCore/html/DateTimeFieldsState.h', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/InputType.h', u'Source/WebCore/html/shadow/ClearButtonElement.cpp', u'Source/WebCore/html/shadow/ClearButtonElement.h', u'Source/WebCore/html/shadow/DateTimeEditElement.cpp', u'Source/WebCore/html/shadow/DateTimeEditElement.h', u'Source/WebCore/html/shadow/DateTimeFieldElement.cpp', u'Source/WebCore/html/shadow/DateTimeFieldElement.h']" exit_code: 1
LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5]
Source/WebCore/html/DateTimeFieldsState.cpp:113: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/html/DateTimeFieldsState.cpp:114: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 3 in 37 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191250 [details] Patch Attachment 191250 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16952006 New failing tests: fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-clearbutton-change-and-input-events.html fast/forms/month-multiple-fields/month-multiple-fields-clearbutton-change-and-input-events.html fast/forms/week-multiple-fields/week-multiple-fields-clearbutton-change-and-input-events.html fast/forms/date-multiple-fields/date-multiple-fields-clearbutton-change-and-input-events.html Created attachment 191307 [details]
Patch
Attachment 191307 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-spinbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/fast/forms/resources/common-clearbutton-change-and-input-events.js', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-clearbutton-change-and-input-events-expected.txt', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-clearbutton-change-and-input-events.html', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-mouse-events-expected.txt', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-mouse-events.html', u'LayoutTests/fast/forms/week-multiple-fields/week-multiple-fields-spinbutton-change-and-input-events.html', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/date/date-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/datetimelocal/datetimelocal-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/datetimelocal/datetimelocal-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-l10n-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/month/month-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-basic-expected.png', u'LayoutTests/platform/chromium-mac/fast/forms/week/week-appearance-pseudo-elements-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-locale-hebrew-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-rtl-expected.png', u'LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/css/html.css', u'Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp', u'Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.h', u'Source/WebCore/html/DateTimeFieldsState.cpp', u'Source/WebCore/html/DateTimeFieldsState.h', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/InputType.h', u'Source/WebCore/html/shadow/ClearButtonElement.cpp', u'Source/WebCore/html/shadow/ClearButtonElement.h', u'Source/WebCore/html/shadow/DateTimeEditElement.cpp', u'Source/WebCore/html/shadow/DateTimeEditElement.h', u'Source/WebCore/html/shadow/DateTimeFieldElement.cpp', u'Source/WebCore/html/shadow/DateTimeFieldElement.h']" exit_code: 1
LayoutTests/platform/chromium-mac/platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-with-scroll-bar-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5]
Total errors found: 1 in 37 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191307&action=review > Source/WebCore/ChangeLog:8 > + We are going to replace the spin buttons in input types date/week/month/datetime-local with a clear value button. I don't think the replacement is reasonable. Why don't you just add the clear button? The input[type=time] exception is bad for consistency, and we want the clear button for input[type=time] too. > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:499 > + if (element()->isRequired() || m_dateTimeEditElement->valueAsDateTimeFieldsState().isEmpty()) m_dateTimeEditElement->valueAsDateTimeFieldsState().isEmpty() is equivalent to !m_dateTimeEditElement->anyEditableFieldsHaveValues() > Source/WebCore/html/shadow/ClearButtonElement.h:33 > +class ClearButtonElement : public HTMLDivElement { We had better re-use SearchFieldCancelButtonElement. (In reply to comment #6) > (From update of attachment 191307 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191307&action=review > > > Source/WebCore/ChangeLog:8 > > + We are going to replace the spin buttons in input types date/week/month/datetime-local with a clear value button. > > I don't think the replacement is reasonable. Why don't you just add the clear button? > The input[type=time] exception is bad for consistency, and we want the clear button for input[type=time] too. ok I'm adding the clear button to all date/time types. > > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:499 > > + if (element()->isRequired() || m_dateTimeEditElement->valueAsDateTimeFieldsState().isEmpty()) > > m_dateTimeEditElement->valueAsDateTimeFieldsState().isEmpty() is equivalent to !m_dateTimeEditElement->anyEditableFieldsHaveValues() Done. > > Source/WebCore/html/shadow/ClearButtonElement.h:33 > > +class ClearButtonElement : public HTMLDivElement { > > We had better re-use SearchFieldCancelButtonElement. Done. Created attachment 191487 [details]
Patch
I had to remove the image files because the size went over 2MB. Comment on attachment 191487 [details] Patch Attachment 191487 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17055092 Comment on attachment 191487 [details] Patch Attachment 191487 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17053154 Comment on attachment 191487 [details] Patch Attachment 191487 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17046185 Comment on attachment 191487 [details] Patch Attachment 191487 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17071016 Comment on attachment 191487 [details] Patch Attachment 191487 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17013079 New failing tests: platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-locale-hebrew.html fast/forms/time/time-appearance-basic.html fast/forms/time/time-appearance-pseudo-elements.html fast/css/text-input-with-webkit-border-radius.html touchadjustment/search-cancel.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-rtl.html fast/css/text-overflow-input.html fast/repaint/search-field-cancel.html fast/speech/input-appearance-searchandspeech.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-with-scroll-bar.html fast/css/input-search-padding.html fast/replaced/width100percent-searchfield.html Comment on attachment 191487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191487&action=review > Source/WebCore/ChangeLog:60 > + * html/shadow/ClearButtonElement.cpp: Added. Adding ClearButtonElement and updating input[type=search] code should be a separated patch, and I think we can avoid rebaseline for existing type=search tests. > Source/WebCore/css/html.css:458 > -input[type="search"]::-webkit-search-cancel-button { > +input::-webkit-clear-button { > -webkit-appearance: searchfield-cancel-button; > display: block; > -webkit-box-flex: 0; > -webkit-user-modify: read-only !important; > + margin-left: 2px; We don't need to add the margin for input[type=search]. So, we should input::-webkit-clear-button { ... display: inline-block; /* Multiple fields UI requires inline-block. See Bug 110974 */ .... margin-left: 2px; } input[type="search"]::-webkit-clear-button { margin-left: 0; } > Source/WebCore/html/SearchInputType.cpp:200 > + element()->focus(); > + element()->select(); Possible use-after-free for 'this' by focus events. > Source/WebCore/html/SearchInputType.cpp:212 > + element()->setValueForUser(""); > + updateClearButtonVisibility(); > + element()->onSearch(); Possible use-after-free for 'this' by 'change' 'input' events. Created attachment 191694 [details]
Patch
Comment on attachment 191694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191694&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:9576 > - 7EA30F6716DFFE7500257D0B /* JSWebGLCompressedTextureATC.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = JSWebGLCompressedTextureATC.cpp; path = JSWebGLCompressedTextureATC.cpp; sourceTree = "<group>"; }; > - 7EA30F6816DFFE7500257D0B /* JSWebGLCompressedTextureATC.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = JSWebGLCompressedTextureATC.h; path = JSWebGLCompressedTextureATC.h; sourceTree = "<group>"; }; > + 7EA30F6716DFFE7500257D0B /* JSWebGLCompressedTextureATC.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSWebGLCompressedTextureATC.cpp; sourceTree = "<group>"; }; > + 7EA30F6816DFFE7500257D0B /* JSWebGLCompressedTextureATC.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSWebGLCompressedTextureATC.h; sourceTree = "<group>"; }; You don't need to update these lines > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26694 > + C37DF8F016E746710079A0EB /* ClearButtonElement.h in Headers */, Put this line in a sorted position. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:29897 > + C37DF8EF16E746710079A0EB /* ClearButtonElement.cpp in Sources */, Ditto. > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:486 > + element()->setValue("", DispatchInputAndChangeEvent); > + updateClearButtonVisibility(); Possible use-after-free for 'this' because change/input event handlers can change the input type. We had better have HTMLInputElement::updateClearButtonVisibility and InputType::updateClearButtonVisibility, and this function would be something like: RefPtr<HTMLInputElement> input(element()); input->setValue(...); input->updateClearButtonVisibility(); > Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:512 > +void BaseMultipleFieldsDateAndTimeInputType::hideClearButton() > +{ > + if (!m_clearButton) > + return; > + m_clearButton->setInlineStyleProperty(CSSPropertyVisibility, CSSValueHidden); > +} > + > +void BaseMultipleFieldsDateAndTimeInputType::showClearButton() > +{ > + if (!m_clearButton) > + return; > + m_clearButton->removeInlineStyleProperty(CSSPropertyVisibility); > +} These functions are used only by updateClearButtonVisiblity. We can fold them into updateClearButtonVisibility and remove the functions. > Source/WebCore/html/shadow/ClearButtonElement.cpp:1 > +#include "config.h" Need a copyright header. > Source/WebCore/html/shadow/ClearButtonElement.cpp:28 > +const AtomicString& ClearButtonElement::shadowPseudoId() const > +{ > + DEFINE_STATIC_LOCAL(AtomicString, pseudoId, ("-webkit-clear-button", AtomicString::ConstructFromLiteral)); > + return pseudoId; > +} nit: You don't need to override shadowPseudoId nowadays. You can call setPseudo in the constructor. > Source/WebCore/html/shadow/ClearButtonElement.cpp:41 > + if (m_capturing) { We prefer early return. > Source/WebCore/html/shadow/ClearButtonElement.h:1 > +#ifndef ClearButtonElement_h Need a copyright header. > Source/WebCore/html/shadow/ClearButtonElement.h:20 > + virtual void releaseCapture(); virtual is not needed? > Source/WebCore/html/shadow/ClearButtonElement.h:23 > + virtual void defaultEventHandler(Event*); It seems we can make this private. Created attachment 191908 [details]
Patch
Comment on attachment 191908 [details] Patch Attachment 191908 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17000373 New failing tests: platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-locale-hebrew.html fast/forms/time/time-appearance-basic.html fast/forms/time/time-appearance-pseudo-elements.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-rtl.html platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-appearance-with-scroll-bar.html Comment on attachment 191908 [details]
Patch
Looks good. Please address the cr-linux EWS failures.
Created attachment 191946 [details]
Patch
Comment on attachment 191946 [details] Patch Clearing flags on attachment: 191946 Committed r145055: <http://trac.webkit.org/changeset/145055> All reviewed patches have been landed. Closing bug. Hi, immediately after this patch, many of the layout tests on Window started failing on the bots. Namely, it looks like all the dom/html/level2/html/ tests are crashing. I for some reason cannot reproduce locally and am not sure why this is happening. Can we temporarily roll this out just so I can confirm whether or not it's really this patch that's causing the issue? Much appreciated. (In reply to comment #24) > Hi, immediately after this patch, many of the layout tests on Window started failing on the bots. > Namely, it looks like all the dom/html/level2/html/ tests are crashing. > > I for some reason cannot reproduce locally and am not sure why this is happening. > Can we temporarily roll this out just so I can confirm whether or not it's really this patch that's causing the issue? > Much appreciated. Looks like a different JSC patch fixed the issue. nvm! |