Bug 86058 – [Forms] Move ValidityState methods implementation to another place

• Component: Forms
• Status: RESOLVED FIXED

This is part of re-factoring of HTMLInputElement class for Steppable input type.

The objective of this change is
 1. Limit scope of Steppable input type related methods, maximumString, minimumString and stepString.
 2. Dispatching by virtual method rather than type checking.

Created attachment 141119 [details]
Patch 1

Created attachment 141122 [details]
Patch 2

Created attachment 141124 [details]
Patch 3

Created attachment 141125 [details]
Patch 4

Created attachment 141126 [details]
Patch 5

Created attachment 141127 [details]
Patch 6

Created attachment 141310 [details]
Patch 7

Comment on attachment 141310 [details]
Patch 7

It seems cr-linux is too busy. Could you review changes for style and design?
Thanks in advance.

[Kent Tamura]

Comment on attachment 141310 [details]
Patch 7

View in context: https://bugs.webkit.org/attachment.cgi?id=141310&action=review

> Source/WebCore/ChangeLog:91
> +        * html/FormAssociatedElement.cpp:
> +        (WebCore::FormAssociatedElement::formAttributeChanged):
> +        (WebCore::FormAssociatedElement::customError):
> +        (WebCore):
> +        (WebCore::FormAssociatedElement::patternMismatch):
> +        (WebCore::FormAssociatedElement::rangeOverflow):
> +        (WebCore::FormAssociatedElement::rangeUnderflow):
> +        (WebCore::FormAssociatedElement::stepMismatch):
> +        (WebCore::FormAssociatedElement::tooLong):
> +        (WebCore::FormAssociatedElement::typeMismatch):
> +        (WebCore::FormAssociatedElement::valid):
> +        (WebCore::FormAssociatedElement::valueMissing):
> +        (WebCore::FormAssociatedElement::customValidationMessage):
> +        (WebCore::FormAssociatedElement::validationMessage):
> +        (WebCore::FormAssociatedElement::setCustomValidity):
> +        * html/FormAssociatedElement.h:
> +        (FormAssociatedElement):
> +        * html/HTMLFormControlElement.cpp:
> +        (WebCore::HTMLFormControlElement::setCustomValidity):
> +        * html/HTMLFormControlElement.h:
> +        (HTMLFormControlElement):
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::isValidValue):
> +        (WebCore::HTMLInputElement::tooLong):
> +        (WebCore):
> +        (WebCore::HTMLInputElement::typeMismatch):
> +        (WebCore::HTMLInputElement::valueMissing):
> +        (WebCore::HTMLInputElement::patternMismatch):
> +        (WebCore::HTMLInputElement::rangeUnderflow):
> +        (WebCore::HTMLInputElement::rangeOverflow):
> +        (WebCore::HTMLInputElement::validationMessage):
> +        (WebCore::HTMLInputElement::minimum):
> +        (WebCore::HTMLInputElement::maximum):
> +        (WebCore::HTMLInputElement::stepMismatch):
> +        (WebCore::HTMLInputElement::isInRange):
> +        (WebCore::HTMLInputElement::isOutOfRange):
> +        * html/HTMLInputElement.h:
> +        (HTMLInputElement):
> +        * html/HTMLObjectElement.h:
> +        * html/HTMLSelectElement.cpp:
> +        (WebCore::HTMLSelectElement::validationMessage):
> +        (WebCore):
> +        (WebCore::HTMLSelectElement::valueMissing):
> +        * html/HTMLSelectElement.h:
> +        (HTMLSelectElement):
> +        * html/HTMLTextAreaElement.cpp:
> +        (WebCore::HTMLTextAreaElement::validationMessage):
> +        (WebCore):
> +        (WebCore::HTMLTextAreaElement::valueMissing):
> +        (WebCore::HTMLTextAreaElement::tooLong):
> +        * html/HTMLTextAreaElement.h:
> +        (HTMLTextAreaElement):
> +        * html/InputType.cpp:
> +        (WebCore::InputType::stepMismatch):
> +        (WebCore):
> +        (WebCore::InputType::alignValueForStep):
> +        (WebCore::InputType::stepUpFromRenderer):
> +        (WebCore::InputType::validationMessage):
> +        * html/InputType.h:
> +        (InputType):
> +        * html/ValidityState.cpp:
> +        (WebCore::ValidityState::validationMessage):
> +        (WebCore::ValidityState::valueMissing):
> +        (WebCore::ValidityState::typeMismatch):
> +        (WebCore::ValidityState::patternMismatch):
> +        (WebCore::ValidityState::tooLong):
> +        (WebCore::ValidityState::rangeUnderflow):
> +        (WebCore::ValidityState::rangeOverflow):
> +        (WebCore::ValidityState::stepMismatch):
> +        (WebCore::ValidityState::customError):
> +        (WebCore::ValidityState::valid):
> +        * html/ValidityState.h:
> +        (ValidityState):

You had better write what you changed for each of functions.

> Source/WebCore/html/HTMLInputElement.h:51
>      bool tooLong(const String&, NeedsToCheckDirtyFlag) const;

I think we can make this private.

> Source/WebCore/html/HTMLInputElement.h:60
> +    // valueMissing() ignores the specified string value for CHECKBOX and RADIO.
> +    virtual bool valueMissing() const OVERRIDE;

This comment is stale.  We should remove it.

> Source/WebCore/html/HTMLTextAreaElement.h:55
>      bool valueMissing(const String& value) const { return isRequiredFormControl() && !disabled() && !readOnly() && value.isEmpty(); }
>      bool tooLong(const String&, NeedsToCheckDirtyFlag) const;

We should make them private.

Created attachment 141337 [details]
Patch 8

Created attachment 141339 [details]
Patch 9

Created attachment 141343 [details]
Patch 10

Comment on attachment 141343 [details]
Patch 10

Thanks for review. I updated this patch as you suggested.

Failure of EWS-WIN doesn't relate to my patch, I copy/paste error message at botton.

Thanks in advance.

..\WebProcess\WebCoreSupport\win\WebPopupMenuWin.cpp(122) : error C2664: 'WebCore::TextRun::TextRun(const UChar *,int,float,float,WebCore::TextRun::ExpansionBehavior,WebCore::TextDirection,bool,bool,WebCore::TextRun::RoundingHacks)' : cannot convert parameter 6 from 'WebCore::TextRun::ExpansionBehaviorFlags' to 'WebCore::TextDirection'
7>        Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or function-style cast)

[Kent Tamura]

Comment on attachment 141343 [details]
Patch 10

View in context: https://bugs.webkit.org/attachment.cgi?id=141343&action=review

> Source/WebCore/html/InputType.h:160
> +    virtual bool stepMismatch(const String&) const;

We don't need to make this virtual, right?

[Kent Tamura]

Comment on attachment 141343 [details]
Patch 10

View in context: https://bugs.webkit.org/attachment.cgi?id=141343&action=review

> Source/WebCore/html/FormAssociatedElement.h:64
> +    virtual bool customError() const;

We don't need to make this virtual.

> Source/WebCore/html/FormAssociatedElement.h:70
> +    virtual bool patternMismatch() const;
> +    virtual bool rangeOverflow() const;
> +    virtual bool rangeUnderflow() const;
> +    virtual bool stepMismatch() const;
> +    virtual bool tooLong() const;
> +    virtual bool typeMismatch() const;

We should add a comment that an override function must call willValidate().

> Source/WebCore/html/FormAssociatedElement.h:71
> +    virtual bool valid() const;

We don't need to make this virtual.

> Source/WebCore/html/FormAssociatedElement.h:72
> +    virtual bool valueMissing() const;

We should add a comment that an override function must call willValidate().

Created attachment 141366 [details]
Patch 11

Comment on attachment 141366 [details]
Patch 11

Thanks for review.
I updated comments in FormAssociateElement.h as you suggested.

Local build was succeeded and changes at this time is comment only. So, EWS build should be passed.

Please review this patch.
Thanks in advance!

[Kent Tamura]

Comment on attachment 141366 [details]
Patch 11

ok

[WebKit Review Bot]

Comment on attachment 141366 [details]
Patch 11

Clearing flags on attachment: 141366

Committed r116752: <http://trac.webkit.org/changeset/116752>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86201

[Kent Tamura]

(In reply to comment #19)
> (From update of attachment 141366 [details])
> Clearing flags on attachment: 141366
> 
> Committed r116752: <http://trac.webkit.org/changeset/116752>

roll out because of Chromium-mac build errors.


In file included from third_party/WebKit/Source/WebCore/html/HTMLFormControlElementWithState.h:27:
third_party/WebKit/Source/WebCore/html/HTMLFormControlElement.h:98:12:error: 'WebCore::HTMLFormControlElement::validationMessage' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
    String validationMessage();
           ^
third_party/WebKit/Source/WebCore/html/FormAssociatedElement.h:75:20: note: hidden overloaded virtual function 'WebCore::FormAssociatedElement::validationMessage' declared here
    virtual String validationMessage() const;
                   ^

Created attachment 141627 [details]
Patch 12 - Fix virtual/override issues

Comment on attachment 141627 [details]
Patch 12 - Fix virtual/override issues

Build locally with Chromium-Mac/clang to make sure virtual/override declaration correct.

Changes are in HTMLFormControlElement.{cpp,h}, HTMLObjectElement.h.

Please review again.
Thanks in advance.

[WebKit Review Bot]

Comment on attachment 141627 [details]
Patch 12 - Fix virtual/override issues

Clearing flags on attachment: 141627

Committed r116915: <http://trac.webkit.org/changeset/116915>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.