Summary: | [BlackBerry] Implement Date/Time picker | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Crystal Zhang <haizhang> | ||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mifenton, rakuco, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Crystal Zhang
2012-07-10 14:45:12 PDT
Created attachment 151535 [details]
patch
Comment on attachment 151535 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=151535&action=review Can be improved some more. > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:44 > + void generateHTML(BlackBerry::Platform::BlackBerryInputType, const BlackBerry::WebKit::WebString& value, const BlackBerry::WebKit::WebString& min, const BlackBerry::WebKit::WebString& max, double step); You can probably make some stuff more private, this is probably internal. > Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:504 > + WTF::String value = element->value(); WTF:: prefix can go in this file. > Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:509 > + DatePickerClient* client = new DatePickerClient(type, value, min, max, step, m_webPage, element); Who deletes client? (In reply to comment #2) > (From update of attachment 151535 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151535&action=review > > Can be improved some more. > > > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:44 > > + void generateHTML(BlackBerry::Platform::BlackBerryInputType, const BlackBerry::WebKit::WebString& value, const BlackBerry::WebKit::WebString& min, const BlackBerry::WebKit::WebString& max, double step); > > You can probably make some stuff more private, this is probably internal. The parameters passed from ctor that needed have been made private, others are just for generating HTML. > > > Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:504 > > + WTF::String value = element->value(); > > WTF:: prefix can go in this file. ok. > > > Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:509 > > + DatePickerClient* client = new DatePickerClient(type, value, min, max, step, m_webPage, element); > > Who deletes client? DatePickerClient will be passed to PagePopupBlackBerry as an OwnPtr, and will be deleted automatically. Comment on attachment 151535 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=151535&action=review > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:37 > +class PagePopup; I think you can actually remove the last reference. Created attachment 151739 [details]
updated patch
Comment on attachment 151739 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=151739&action=review Looks good, found some small stuff. > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:22 > +#include "IntSize.h" You can use forward reference. > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:25 > +#include "WebString.h" Ditto. > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.h:55 > + HTMLInputElement* m_element; Can you recheck the private situation again? I think at least the member vars can be made private. Created attachment 151748 [details]
updated patch
Comment on attachment 151748 [details]
updated patch
LGTM.
Comment on attachment 151748 [details] updated patch Clearing flags on attachment: 151748 Committed r122364: <http://trac.webkit.org/changeset/122364> All reviewed patches have been landed. Closing bug. |