Bug 90911

Summary: [BlackBerry] Implement Date/Time picker
Product: WebKit Reporter: Crystal Zhang <haizhang>
Component: WebKit BlackBerryAssignee: 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 Flags
patch
rwlbuis: review-
updated patch
rwlbuis: review-
updated patch none

Description Crystal Zhang 2012-07-10 14:45:12 PDT
Implement HTML Date/Time picker.
Comment 1 Crystal Zhang 2012-07-10 15:03:34 PDT
Created attachment 151535 [details]
patch
Comment 2 Rob Buis 2012-07-10 15:16:26 PDT
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?
Comment 3 Crystal Zhang 2012-07-10 15:21:02 PDT
(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 4 Rob Buis 2012-07-10 15:29:36 PDT
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.
Comment 5 Crystal Zhang 2012-07-11 11:30:16 PDT
Created attachment 151739 [details]
updated patch
Comment 6 Rob Buis 2012-07-11 11:52:22 PDT
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.
Comment 7 Crystal Zhang 2012-07-11 12:20:03 PDT
Created attachment 151748 [details]
updated patch
Comment 8 Rob Buis 2012-07-11 12:24:46 PDT
Comment on attachment 151748 [details]
updated patch

LGTM.
Comment 9 WebKit Review Bot 2012-07-11 13:21:13 PDT
Comment on attachment 151748 [details]
updated patch

Clearing flags on attachment: 151748

Committed r122364: <http://trac.webkit.org/changeset/122364>
Comment 10 WebKit Review Bot 2012-07-11 13:21:18 PDT
All reviewed patches have been landed.  Closing bug.