Bug 86058 - [Forms] Move ValidityState methods implementation to another place
Summary: [Forms] Move ValidityState methods implementation to another place
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on: 86201
Blocks: 80381 82034
  Show dependency treegraph
 
Reported: 2012-05-09 22:07 PDT by yosin
Modified: 2012-05-28 22:52 PDT (History)
2 users (show)

See Also:


Attachments
Patch 1 (31.57 KB, patch)
2012-05-10 02:43 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (31.15 KB, patch)
2012-05-10 02:51 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (31.18 KB, patch)
2012-05-10 02:56 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (31.08 KB, patch)
2012-05-10 03:00 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 5 (31.09 KB, patch)
2012-05-10 03:06 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 6 (30.82 KB, patch)
2012-05-10 03:14 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 7 (31.33 KB, patch)
2012-05-10 18:47 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 8 (33.57 KB, patch)
2012-05-11 00:02 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 9 (34.24 KB, patch)
2012-05-11 00:16 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 10 (34.22 KB, patch)
2012-05-11 00:26 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 11 (34.36 KB, patch)
2012-05-11 02:50 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 12 - Fix virtual/override issues (35.19 KB, patch)
2012-05-13 19:03 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-05-09 22:07:35 PDT
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.
Comment 1 yosin 2012-05-10 02:43:43 PDT
Created attachment 141119 [details]
Patch 1
Comment 2 yosin 2012-05-10 02:51:13 PDT
Created attachment 141122 [details]
Patch 2
Comment 3 yosin 2012-05-10 02:56:25 PDT
Created attachment 141124 [details]
Patch 3
Comment 4 yosin 2012-05-10 03:00:42 PDT
Created attachment 141125 [details]
Patch 4
Comment 5 yosin 2012-05-10 03:06:19 PDT
Created attachment 141126 [details]
Patch 5
Comment 6 yosin 2012-05-10 03:14:19 PDT
Created attachment 141127 [details]
Patch 6
Comment 7 yosin 2012-05-10 18:47:28 PDT
Created attachment 141310 [details]
Patch 7
Comment 8 yosin 2012-05-10 19:27:27 PDT
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.
Comment 9 Kent Tamura 2012-05-10 23:09:40 PDT
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.
Comment 10 yosin 2012-05-11 00:02:52 PDT
Created attachment 141337 [details]
Patch 8
Comment 11 yosin 2012-05-11 00:16:05 PDT
Created attachment 141339 [details]
Patch 9
Comment 12 yosin 2012-05-11 00:26:51 PDT
Created attachment 141343 [details]
Patch 10
Comment 13 yosin 2012-05-11 01:31:37 PDT
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)
Comment 14 Kent Tamura 2012-05-11 02:29:37 PDT
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?
Comment 15 Kent Tamura 2012-05-11 02:40:36 PDT
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().
Comment 16 yosin 2012-05-11 02:50:31 PDT
Created attachment 141366 [details]
Patch 11
Comment 17 yosin 2012-05-11 02:53:22 PDT
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!
Comment 18 Kent Tamura 2012-05-11 02:59:49 PDT
Comment on attachment 141366 [details]
Patch 11

ok
Comment 19 WebKit Review Bot 2012-05-11 05:39:04 PDT
Comment on attachment 141366 [details]
Patch 11

Clearing flags on attachment: 141366

Committed r116752: <http://trac.webkit.org/changeset/116752>
Comment 20 WebKit Review Bot 2012-05-11 05:39:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2012-05-11 05:48:10 PDT
Re-opened since this is blocked by 86201
Comment 22 Kent Tamura 2012-05-11 05:49:27 PDT
(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;
                   ^
Comment 23 yosin 2012-05-13 19:03:15 PDT
Created attachment 141627 [details]
Patch 12 - Fix virtual/override issues
Comment 24 yosin 2012-05-13 19:19:33 PDT
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.
Comment 25 WebKit Review Bot 2012-05-13 21:22:00 PDT
Comment on attachment 141627 [details]
Patch 12 - Fix virtual/override issues

Clearing flags on attachment: 141627

Committed r116915: <http://trac.webkit.org/changeset/116915>
Comment 26 WebKit Review Bot 2012-05-13 21:22:05 PDT
All reviewed patches have been landed.  Closing bug.