Bug 103746 – [Chromium] Create an enum for the kind of date input and use that on WebDateTimeChooserParams instead of a bare WebString

• Component: New Bugs
• Status: RESOLVED FIXED

[Miguel Garcia]

Create an enum for the kind of date input and use that on WebDateTimeChooserParams[H[D[C[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[B[D[B

[Miguel Garcia]

Created attachment 176964 [details]
Patch

[Miguel Garcia]

Created attachment 176965 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 176965 [details]
Patch

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

> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:137
> +WebDateTimeInputType ExternalDateTimeChooser::webType(const WebString& source)

nit: toWebDateTimeInputType(...)

> Source/WebKit/chromium/src/ExternalDateTimeChooser.h:56
> +    static WebDateTimeInputType webType(const WebString&);

nit: put private static methods that do not depend on any other static members at file scope in the .cpp file.

[Miguel Garcia]

Created attachment 177101 [details]
Patch

[Miguel Garcia]

Comment on attachment 176965 [details]
Patch

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

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:137
>> +WebDateTimeInputType ExternalDateTimeChooser::webType(const WebString& source)
> 
> nit: toWebDateTimeInputType(...)

Done

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.h:56
>> +    static WebDateTimeInputType webType(const WebString&);
> 
> nit: put private static methods that do not depend on any other static members at file scope in the .cpp file.

Done

[Miguel Garcia]

Thanks a lot for the quick review Darin, comments addressed.

[Miguel Garcia]

Created attachment 177102 [details]
Patch

[Kent Tamura]

Comment on attachment 177101 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:4
> +

nit: We usually add no blank line between the summary and the bug URL.

> Source/WebKit/chromium/public/WebDateTimeChooserParams.h:30
> +

nit: Blank line is not needed.

> Source/WebKit/chromium/public/WebTextInputType.h:52
> +    // TODO(miguelg): Remove these types once Date like types are not

TODO(name) is Chromium-style.  It should be FIXME:.

> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:36
>  
> +#include "platform/WebString.h"

Blank line before #include is not needed.

> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:85
> +static WebDateTimeInputType toWebDateTimeInputType(const WebString& source)

parameters.type is AtomicString.  The "source" argument should be const AtomicString&.

> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:87
> +    if (source.equals(WebString::fromUTF8("date")))

You can compare "source" with an AtomicSting object provided by Source/WebCore/html/InputTypeNames.h

if (source == InputTypeNames::date())
  ...

> Source/WebKit/chromium/src/ExternalDateTimeChooser.h:31
> +#include "WebDateTimeInputType.h"

This inclusion looks unnecessary.

[Miguel Garcia]

Created attachment 177114 [details]
Patch

[Miguel Garcia]

Comment on attachment 177101 [details]
Patch

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

>> Source/WebKit/chromium/public/WebDateTimeChooserParams.h:30
>> +
> 
> nit: Blank line is not needed.

Done

>> Source/WebKit/chromium/public/WebTextInputType.h:52
>> +    // TODO(miguelg): Remove these types once Date like types are not
> 
> TODO(name) is Chromium-style.  It should be FIXME:.

Done

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:36
>> +#include "platform/WebString.h"
> 
> Blank line before #include is not needed.

Done

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:85
>> +static WebDateTimeInputType toWebDateTimeInputType(const WebString& source)
> 
> parameters.type is AtomicString.  The "source" argument should be const AtomicString&.

Done

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.cpp:87
>> +    if (source.equals(WebString::fromUTF8("date")))
> 
> You can compare "source" with an AtomicSting object provided by Source/WebCore/html/InputTypeNames.h
> 
> if (source == InputTypeNames::date())
>   ...

Thanks for the pointer! Done

>> Source/WebKit/chromium/src/ExternalDateTimeChooser.h:31
>> +#include "WebDateTimeInputType.h"
> 
> This inclusion looks unnecessary.

True, now that I moved the static method to a static function inside the cpp file this is no longer needed

[Miguel Garcia]

Created attachment 177115 [details]
Patch

[Miguel Garcia]

Comment on attachment 177101 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:4
>> +
> 
> nit: We usually add no blank line between the summary and the bug URL.

Done

[Kent Tamura]

Comment on attachment 177115 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 177115 [details]
Patch

Clearing flags on attachment: 177115

Committed r136343: <http://trac.webkit.org/changeset/136343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.