Bug 96001

Summary:

[BlackBerry] Implement a color picker

Product:

WebKit

Reporter:

Crystal Zhang <haizhang>

Component:

WebKit BlackBerry

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, gyuyoung.kim, keishi, mifenton, rakuco, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	rwlbuis: review-
updated patch	rwlbuis: review-
updated patch	none

Crystal Zhang

Reported 2012-09-06 11:10:06 PDT

Implement a HTML color picker.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Crystal Zhang

Comment 1 2012-09-06 11:25:29 PDT

Created attachment 162544 [details] patch

Antonio Gomes

Comment 2 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

Chris Dumez

Comment 3 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.

Rob Buis

Comment 4 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.

Crystal Zhang

Comment 5 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().

Crystal Zhang

Comment 6 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

Crystal Zhang

Comment 7 2012-09-06 12:54:13 PDT

Created attachment 162560 [details] updated patch

Chris Dumez

Comment 8 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.

Rob Buis

Comment 9 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.

Crystal Zhang

Comment 10 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.

Crystal Zhang

Comment 11 2012-09-06 13:28:33 PDT

Created attachment 162567 [details] updated patch

Rob Buis

Comment 12 2012-09-06 13:32:31 PDT

Comment on attachment 162567 [details] updated patch Looks good.

WebKit Review Bot

Comment 13 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>

WebKit Review Bot

Comment 14 2012-09-06 21:18:34 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.