[webkit-reviews] review denied: [Bug 95681] Move PagePopupClient to WebKit layer : [Attachment 161946] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 95681: Move PagePopupClient to WebKit layer
https://bugs.webkit.org/show_bug.cgi?id=95681

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.