We want to move PagePopupClient to WebKit layer where it is more approprate
Created attachment 161910 [details] Patch
Comment on attachment 161910 [details] Patch Attachment 161910 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13734449
Created attachment 161914 [details] Patch
Comment on attachment 161914 [details] Patch Attachment 161914 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13742139
Created attachment 161921 [details] Patch
Comment on attachment 161921 [details] Patch Attachment 161921 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13729878
Comment on attachment 161921 [details] Patch Attachment 161921 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13742165
Created attachment 161945 [details] Patch
Comment on attachment 161945 [details] Patch Attachment 161945 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13754163
Created attachment 161946 [details] Patch
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.
Created attachment 162011 [details] Patch
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.
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&)
Comment on attachment 162011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162011&action=review > Source/WebKit/chromium/ChangeLog:59 > +2012-09-04 Keishi Hattori <keishi@webkit.org> > + > + 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): > + > +2012-09-04 Keishi Hattori <keishi@webkit.org> > + > + 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): > + > +2012-09-04 Keishi Hattori <keishi@webkit.org> 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.
Created attachment 162152 [details] Patch
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.
Created attachment 162157 [details] Patch
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?
Created attachment 162161 [details] Patch
Comment on attachment 162161 [details] Patch ok
Comment on attachment 162161 [details] Patch Clearing flags on attachment: 162161 Committed r127558: <http://trac.webkit.org/changeset/127558>
All reviewed patches have been landed. Closing bug.