Bug 90911 – [BlackBerry] Implement Date/Time picker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Crystal Zhang]

Implement HTML Date/Time picker.

[Crystal Zhang]

Created attachment 151535 [details]
patch

[Rob Buis]

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?

[Crystal Zhang]

(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.

[Rob Buis]

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.

[Crystal Zhang]

Created attachment 151739 [details]
updated patch

[Rob Buis]

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.

[Crystal Zhang]

Created attachment 151748 [details]
updated patch

[Rob Buis]

Comment on attachment 151748 [details]
updated patch

LGTM.

[WebKit Review Bot]

Comment on attachment 151748 [details]
updated patch

Clearing flags on attachment: 151748

Committed r122364: <http://trac.webkit.org/changeset/122364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.