Bug 95681 – Move PagePopupClient implementation for input[type=date] to Chromium WebKit layer

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

We want to move PagePopupClient to WebKit layer where it is more approprate

[Keishi Hattori]

Created attachment 161910 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161910 [details]
Patch

Attachment 161910 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13734449

[Keishi Hattori]

Created attachment 161914 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 161914 [details]
Patch

Attachment 161914 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13742139

[Keishi Hattori]

Created attachment 161921 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 161921 [details]
Patch

Attachment 161921 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13729878

[WebKit Review Bot]

Comment on attachment 161921 [details]
Patch

Attachment 161921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13742165

[Keishi Hattori]

Created attachment 161945 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 161945 [details]
Patch

Attachment 161945 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13754163

[Keishi Hattori]

Created attachment 161946 [details]
Patch

[Kent Tamura]

Comment on attachment 161946 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Move PagePopupClient to WebKit layer and introduce ValuePicker that will
> +        be shared with the various pickers in the future.

Would you explain your plan in detail please?
Will you merge ColorChooser interfaces to ValuePicker?
Will you add ChromeClient::openWeekPickerPopup() and openMonthPickerPopup()?

> Source/WebCore/page/Chrome.cpp:462
> +PassOwnPtr<ValuePicker> Chrome::openCalendarPickerPopup(ValuePickerClient* client)
> +{
> +    return m_client->openCalendarPickerPopup(client);
> +}

This function is just calling ChromeClient.  You should remove Chrome::openCalendarPickerPopup(), and use ChromeClient::openCalendarPickerPopup() directly.

> Source/WebCore/platform/ValuePicker.h:40
> +class ValuePicker {

The name should be FooChooser in order to follow existing interfaces.
StringValueChooser, DateTimeChooser, etc.

> Source/WebCore/platform/ValuePickerClient.h:45
> +    virtual IntRect elementRectRelativeToRootView() const = 0;

Using the word 'element' is not good for platform/ code.  I think 'anchorRectInRootView' is better.

> Source/WebCore/platform/ValuePickerClient.h:61
> +    // Returns true if we should show suggestions to the user.
> +    virtual bool shouldShowSuggestions() const = 0;
> +    // Returns the list of suggestion values.
> +    virtual Vector<String> suggestionValues() const = 0;
> +    // Returns the list of suggestion labels.
> +    virtual Vector<String> suggestionLabels() const = 0;
> +    // Returns the minimum pickable value.
> +    virtual double minimumValue() const = 0;
> +    // Returns the maximum pickable value.
> +    virtual double maximumValue() const = 0;
> +    // Returns the value step size.
> +    virtual double valueStep() const = 0;
> +    // Returns if the value is required.
> +    virtual bool valueRequired() const = 0;

ValuePicker needs many arguments, and specifying them by virtual functions is not elegant.
We had better introduce a parameter struct like ValuePickerParameters or something.

[Keishi Hattori]

Created attachment 162011 [details]
Patch

[Kent Tamura]

Comment on attachment 162011 [details]
Patch

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

> Source/WebCore/platform/DateTimeChooserClient.h:53
> +    // Returns the rect for the element that originated the picker.
> +    virtual IntRect anchorRectInRootView() const = 0;
> +    // Returns the current value which should be shown in the color picker.
> +    virtual String currentValue() = 0;
> +    // Returns true if we should show suggestions to the user.
> +    virtual bool shouldShowSuggestions() const = 0;
> +    // Returns the list of suggestion values.
> +    virtual Vector<String> suggestionValues() const = 0;
> +    // Returns the list of suggestion labels.
> +    virtual Vector<String> suggestionLabels() const = 0;

We can move them to DateTimeChooserParameters.

[Kent Tamura]

Comment on attachment 162011 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:232
> +        virtual PassOwnPtr<DateTimeChooser> openCalendarPickerPopup(DateTimeChooserClient*, const DateTimeChooserParameters&) = 0;

We can make it more generic.
e.g. 
 - Adding 'type' member to DateTimeChooserParameters, and 
 - openDateTimeChooser(DateTimeChooserClient*, const DateTimeChooserParameters&)

[Kent Tamura]

Comment on attachment 162011 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:59
> [phone]  Keishi Hattori  <[e-mail]>
> +
> +        Move PagePopupClient to WebKit layer
> +        https://bugs.webkit.org/show_bug.cgi?id=95681
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebKit.gyp:
> +        * src/CalendarPickerPopup.cpp: Added.
> +        (WebKit):
> +        (WebKit::CalendarPickerPopup::CalendarPickerPopup):
> +        (WebKit::CalendarPickerPopup::~CalendarPickerPopup):
> +        (WebKit::CalendarPickerPopup::endChooser):
> +        (WebKit::CalendarPickerPopup::contentSize):
> +        (WebKit::CalendarPickerPopup::writeDocument):
> +        (WebKit::CalendarPickerPopup::setValueAndClosePopup):
> +        (WebKit::CalendarPickerPopup::didClosePopup):
> +        * src/CalendarPickerPopup.h: Copied from Source/WebCore/html/shadow/CalendarPickerElement.h.
> +        (WebCore):
> +        (WebKit):
> +        (CalendarPickerPopup):
> +        * src/ChromeClientImpl.cpp:
> +        (WebKit):
> +        (WebKit::ChromeClientImpl::openCalendarPickerPopup):
> +        * src/ChromeClientImpl.h:
> +        (WebCore):
> +        (ChromeClientImpl):
> +
> [phone]  Keishi Hattori  <[e-mail]>
> +
> +        Move PagePopupClient to WebKit layer
> +        https://bugs.webkit.org/show_bug.cgi?id=95681
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> +
> +        * WebKit.gyp:
> +        * src/CalendarPickerPopup.cpp: Added.
> +        (WebKit):
> +        (WebKit::CalendarPickerPopup::CalendarPickerPopup):
> +        (WebKit::CalendarPickerPopup::~CalendarPickerPopup):
> +        (WebKit::CalendarPickerPopup::endChooser):
> +        (WebKit::CalendarPickerPopup::contentSize):
> +        (WebKit::CalendarPickerPopup::writeDocument):
> +        (WebKit::CalendarPickerPopup::setValueAndClosePopup):
> +        (WebKit::CalendarPickerPopup::didClosePopup):
> +        * src/CalendarPickerPopup.h: Copied from Source/WebCore/html/shadow/CalendarPickerElement.h.
> +        (WebCore):
> +        (WebKit):
> +        (CalendarPickerPopup):
> +        * src/ChromeClientImpl.cpp:
> +        (WebKit):
> +        (WebKit::ChromeClientImpl::openCalendarPickerPopup):
> +        * src/ChromeClientImpl.h:
> +        (WebCore):
> +        (ChromeClientImpl):
> +
> [phone]  Keishi Hattori  <[e-mail]>

Three Change entries

> Source/WebCore/WebCore.exp.in:2574
> -#endif
> +#endif

Unrelated change

> Source/WebCore/html/shadow/CalendarPickerElement.h:71
> +    OwnPtr<DateTimeChooser> m_picker;

m_chooser is a better name.

> Source/WebKit/chromium/src/CalendarPickerPopup.h:48
> +class CalendarPickerPopup : public WebCore::DateTimeChooser, public WebCore::PagePopupClient {

DateTiemChooserImpl might be a better class name.

[Keishi Hattori]

Created attachment 162152 [details]
Patch

[Kent Tamura]

Comment on attachment 162152 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        (WebCore::CalendarPickerElement::hostInput): Made const.

why?

> Source/WebCore/ChangeLog:36
> +        (WebCore::PagePopupClient::addProperty): Adds double value property to JSON.

why?

> Source/WebCore/html/shadow/CalendarPickerElement.cpp:144
> +    parameters.suggestionValues = Vector<String>();
> +    parameters.suggestionLabels = Vector<String>();

nit: You don't need to set empty vectors.
Anyway,  we had better have a FIXME comment here about <datalist> support.

> Source/WebCore/html/shadow/CalendarPickerElement.h:54
> +    // ColorChooserClient implementation.

ColorChooserClient -> DateTimeChooserClient

> Source/WebCore/loader/EmptyClients.cpp:44
> +#if ENABLE(CALENDAR_PICKER)
> +#include "DateTimeChooser.h"
> +#endif

We don't need such ugly #if-#endif If the content of DateTimeChooser.h is wrapped by #if-#endif.

> Source/WebCore/page/ChromeClient.h:85
> +#if ENABLE(CALENDAR_PICKER)    

You don't need #if-#endif here.  Class declarations are harmless.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:44
> +#if ENABLE(CALENDAR_PICKER)
> +#include "DateTimeChooserImpl.h"
> +#endif

#if-#endif is not needed because  the content of DateTiemChooserImpl.h is wrapped with #if-#endif.

[Keishi Hattori]

Created attachment 162157 [details]
Patch

[Kent Tamura]

Comment on attachment 162157 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        * page/PagePopupClient.cpp:
> +        (WebCore::PagePopupClient::addProperty): Adds double value property to JSON.
> +        (WebCore):
> +        * page/PagePopupClient.h:
> +        (PagePopupClient):

out-of-sync.

> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:103
> +    WebCore::PagePopupClient::addProperty("todayLabel", Platform::current()->queryLocalizedString(WebLocalizedString::CalendarToday), writer);
> +    WebCore::PagePopupClient::addProperty("clearLabel", Platform::current()->queryLocalizedString(WebLocalizedString::CalendarClear), writer);

Is WebCore::PagePopupClient:: needed?

[Keishi Hattori]

Created attachment 162161 [details]
Patch

[Kent Tamura]

Comment on attachment 162161 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 162161 [details]
Patch

Clearing flags on attachment: 162161

Committed r127558: <http://trac.webkit.org/changeset/127558>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.