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: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Crystal Zhang
2012-09-06 11:10:06 PDT
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. |