Bug 96001 - [BlackBerry] Implement a color picker
Summary: [BlackBerry] Implement a color picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-06 11:10 PDT by Crystal Zhang
Modified: 2012-09-06 21:30 PDT (History)
7 users (show)

See Also:


Attachments
patch (10.99 KB, patch)
2012-09-06 11:25 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
updated patch (10.93 KB, patch)
2012-09-06 12:54 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
updated patch (10.92 KB, patch)
2012-09-06 13:28 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Crystal Zhang 2012-09-06 11:10:06 PDT
Implement a HTML color picker.
Comment 1 Crystal Zhang 2012-09-06 11:25:29 PDT
Created attachment 162544 [details]
patch
Comment 2 Antonio Gomes 2012-09-06 12:08:49 PDT
Comment on attachment 162544 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162544&action=review

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:47
> +ColorPickerClient::~ColorPickerClient()
> +{
> +}

lets omit this?

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:79
> +IntSize ColorPickerClient::contentSize()

const

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:81
> +    // Fixme: will generate content size dynamically

FIXME*

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:85
> +String ColorPickerClient::htmlSource()

const
Comment 3 Chris Dumez 2012-09-06 12:22:09 PDT
Comment on attachment 162544 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162544&action=review

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:52
> +    source.append("<style>\n");

You should use appendLiteral() here, it is more efficient.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:56
> +    source.append("</style>\n<style>");

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:59
> +    source.append("</style></head><body>\n");

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:60
> +    source.append("<script>\n");

Ditto...

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:109
> +    writer.addData(m_source.utf8().data(), m_source.utf8().length());

You should probably store m_source.utf8() in a local variable to avoid copying the string twice.
Comment 4 Rob Buis 2012-09-06 12:23:23 PDT
Comment on attachment 162544 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162544&action=review

Looks good, but please address Antonio and my comments.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.h:48
> +    void closePopup();

It seems many of these can be private.
Comment 5 Crystal Zhang 2012-09-06 12:33:51 PDT
(In reply to comment #3)
> (From update of attachment 162544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162544&action=review
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:52
> > +    source.append("<style>\n");
> 
> You should use appendLiteral() here, it is more efficient.
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:56
> > +    source.append("</style>\n<style>");
> 
> Ditto.
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:59
> > +    source.append("</style></head><body>\n");
> 
> Ditto.
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:60
> > +    source.append("<script>\n");
> 
> Ditto...
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:109
> > +    writer.addData(m_source.utf8().data(), m_source.utf8().length());
> 
> You should probably store m_source.utf8() in a local variable to avoid copying the string twice.
 We don't have appendLiteral() in blackberry port, so have to keep append().
Comment 6 Crystal Zhang 2012-09-06 12:39:20 PDT
(In reply to comment #2)
> (From update of attachment 162544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162544&action=review
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:47
> > +ColorPickerClient::~ColorPickerClient()
> > +{
> > +}
> 
> lets omit this?
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:79
> > +IntSize ColorPickerClient::contentSize()
> 
> const
> 
Can't add const here as the function comes from parent class.

> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:81
> > +    // Fixme: will generate content size dynamically
> 
> FIXME*
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:85
> > +String ColorPickerClient::htmlSource()
> 
> const
Comment 7 Crystal Zhang 2012-09-06 12:54:13 PDT
Created attachment 162560 [details]
updated patch
Comment 8 Chris Dumez 2012-09-06 13:04:04 PDT
Comment on attachment 162560 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162560&action=review

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:105
> +    writer.addData(m_source.utf8().data(), m_source.utf8().length());

My comment about caching m_source.utf8() in a local variable was still valid I believe. Any reason you did not fix it?
Your copying the string twice here.
Comment 9 Rob Buis 2012-09-06 13:13:54 PDT
Comment on attachment 162560 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162560&action=review

The comment needs to be fixed, and Cristophe's concern too.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:88
> +    // Return -1 if user cancel the selection.

Comment is a bit wierd.

> Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:91
> +    static const char* cancelValue = "-1";

I dont like using -1 in a string. Or keeping a static var for it. You also still need to convert it to a String each time. Maybe an empty string can be used to cancel? But this does not need to be fixed with this patch.
Comment 10 Crystal Zhang 2012-09-06 13:17:06 PDT
(In reply to comment #8)
> (From update of attachment 162560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162560&action=review
> 
> > Source/WebKit/blackberry/WebCoreSupport/ColorPickerClient.cpp:105
> > +    writer.addData(m_source.utf8().data(), m_source.utf8().length());
> 
> My comment about caching m_source.utf8() in a local variable was still valid I believe. Any reason you did not fix it?
> Your copying the string twice here.

ok, will add it.
Comment 11 Crystal Zhang 2012-09-06 13:28:33 PDT
Created attachment 162567 [details]
updated patch
Comment 12 Rob Buis 2012-09-06 13:32:31 PDT
Comment on attachment 162567 [details]
updated patch

Looks good.
Comment 13 WebKit Review Bot 2012-09-06 21:18:30 PDT
Comment on attachment 162567 [details]
updated patch

Clearing flags on attachment: 162567

Committed r127817: <http://trac.webkit.org/changeset/127817>
Comment 14 WebKit Review Bot 2012-09-06 21:18:34 PDT
All reviewed patches have been landed.  Closing bug.