popup client.
Created attachment 140346 [details] patch
Attachment 140346 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1 Source/WebKit/ChangeLog:8: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 140351 [details] fix style
Comment on attachment 140351 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=140351&action=review Can be cleaned up some more. > Source/WebKit/ChangeLog:7 > + It is best to add at least a short description. > Source/WebKit/PlatformBlackBerry.cmake:6 > + Better not add empty lines here. > Source/WebKit/PlatformBlackBerry.cmake:81 > + Ditto. > Source/WebKit/blackberry/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). It is best to add at least a short description. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:43 > +#include "WindowFeatures.h" Are all includes needed? > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:48 > +using namespace BlackBerry::WebKit; Add a newline here. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:51 > +class PagePopupChromeClient: public ChromeClientBlackBerry { please add a space character before the ':' > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:56 > + 0, 0, 0, 0) Check if you need to initialize m_rect that way, it may default to 0,0,0,0 anyway. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:81 > + Remove empty line. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:97 > + new EmptyFrameLoaderClient; I don't think you have to wrap here, can be on one line. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:105 > + new EmptyContextMenuClient; Ditto. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:122 > + emptyFrameLoaderClient); Ditto. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:129 > + frame->loader()->activeDocumentLoader()->writer(); Ditto. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:138 > + return false; Better do early return style: if (!webpage) return false; ..... return true; > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.h:31 > +#include <JavaScriptCore/JSValueRef.h> Please make sure to only include the ones you need, for example JSC includes can go to the .cpp.
Created attachment 140589 [details] updated patch
Comment on attachment 140589 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=140589&action=review Looks good, can still be cleaned up a bit. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:50 > + Can remove the empty line. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:133 > + Can remov the empty line here. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:233 > + case WebCore::PlatformEvent::MouseReleased: Can remove all WebCore:: prefixes in this method. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:252 > + if (m_page) { Can early return here if !page. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:258 > + m_page.clear(); Better use m_page->clear(); > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.h:24 > + Can remove empty line here. > Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.h:54 > + Ditto.
Created attachment 140754 [details] updated patch
Comment on attachment 140754 [details] updated patch wrong patch
Created attachment 140758 [details] updated patch
Comment on attachment 140758 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=140758&action=review Looks good, can be cleaned up a bit more. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:212 > + JSC::UString name("popUp"); Yong can review this better than me. I do wonder if popUp is a good name. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:233 > + event); Combine the two lines above into one line. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:258 > +} I think early return is preferred here: if (!m_page) { m_page->clear(); return; } m_page-> ....
View in context: https://bugs.webkit.org/attachment.cgi?id=140758&action=review It is an interesting way to do it. I'm always worried about JS re-entrancy. So please make sure it cannot be triggered by JS code. Also, where are the other parts? I cannot finish reviewing without seeing how PagePopupBlackBerry is used. Can I assume it is already working well? > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:43 > +#define urlbarHeight 70 1. all letters should in capital case for macros. 2. I would use enum/const in a namespace/class scope. Also why 70? Where is the number determined? Should we query it from webpageclient dynamically? > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:62 > + WebPagePrivate* webPage() > + { this method can be const > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:73 > +PagePopupBlackBerry::PagePopupBlackBerry(BlackBerry::WebKit::WebPagePrivate* webPage, > + PagePopupClient* client, const IntRect& rect) : > + m_webPagePrivate(webPage), m_client(client) Is the style right? I would write it like this: PagePopupBlackBerry::PagePopupBlackBerry(BlackBerry::WebKit::WebPagePrivate* webPage, PagePopupClient* client, const IntRect& rect) : m_webPagePrivate(webPage) , m_client(client) > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:76 > + m_rect = IntRect(rect.x(), rect.y() - urlbarHeight, > + client->contentSize().width(), client->contentSize().height()); one line should be enough. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:89 > +bool PagePopupBlackBerry::init(WebPage* webpage) > +{ I would use initialize > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:91 > + if (!webpage) > + return false; is it necessary? > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:144 > + char* strArgs = new char[sizeUTF8]; I would use WTF::Vector<char> > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:244 > + delete m_client; > + m_client = 0; I think the client should delete itself in didClosePopup. Or we can use a OwnPtr, and get PassOwnPtr<PopupClient> passed in through contructor.
Created attachment 140781 [details] updated patch
Comment on attachment 140781 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=140781&action=review Other than the "strArgs" issue, the patch looks good to me > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:143 > + WTF::Vector<char> strArgs; > + JSStringGetUTF8CString(string, strArgs.data(), sizeUTF8); We have to allocate enough buffer for strArgs. Otherwise strArgs.data() is a null pointer. The reason I suggested to use Vector<char> is I hate "delete". OwnArrayPtr<char> should also work. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:152 > + client->setValueAndClosePopup(0, strArgs.data()); Are we sure the string data has a null terminator included? Otherwise, we have to pass the string length.
Created attachment 140790 [details] updated patch
Attachment 140790 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1 Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.cpp:143: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 140791 [details] updated patch
Comment on attachment 140790 [details] updated patch You should consider using webkit-patch upload, which will automatically obsolete your previous patches for you.
Comment on attachment 140791 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=140791&action=review Looks good. > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.h:49 > + void setRect(); Can this be removed?
Comment on attachment 140791 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=140791&action=review >> Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerry.h:49 >> + void setRect(); > > Can this be removed? I'd rather keep this method, because it might get call to change the rect in the runtime.
Comment on attachment 140791 [details] updated patch Clearing flags on attachment: 140791 Committed r116462: <http://trac.webkit.org/changeset/116462>
All reviewed patches have been landed. Closing bug.