Implement a HTML color picker.
Created attachment 162544 [details] patch
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 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 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.
(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().
(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
Created attachment 162560 [details] updated patch
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 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.
(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.
Created attachment 162567 [details] updated patch
Comment on attachment 162567 [details] updated patch Looks good.
Comment on attachment 162567 [details] updated patch Clearing flags on attachment: 162567 Committed r127817: <http://trac.webkit.org/changeset/127817>
All reviewed patches have been landed. Closing bug.