RESOLVED FIXED Bug 96001
[BlackBerry] Implement a color picker 
https://bugs.webkit.org/show_bug.cgi?id=96001
Summary [BlackBerry] Implement a color picker
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-
DetailsFormatted DiffDiff
updated patch (10.93 KB, patch)
2012-09-06 12:54 PDT, Crystal Zhang 		rwlbuis: review-
DetailsFormatted DiffDiff
updated patch (10.92 KB, patch)
2012-09-06 13:28 PDT, Crystal Zhang 		no flags
DetailsFormatted DiffDiff
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.