[webkit-reviews] review denied: [Bug 111319] Add clear button to date/time input types : [Attachment 191694] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 111319: Add clear button to date/time input types
https://bugs.webkit.org/show_bug.cgi?id=111319

Attachment 191694: Patch
https://bugs.webkit.org/attachment.cgi?id=191694&action=review

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
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.