Bug 85670 – [BlackBerry] Implement a popup client for HTML controls

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Crystal Zhang]

popup client.

[Crystal Zhang]

Created attachment 140346 [details]
patch

[WebKit Review Bot]

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.

[Crystal Zhang]

Created attachment 140351 [details]
fix style

[Rob Buis]

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.

[Crystal Zhang]

Created attachment 140589 [details]
updated patch

[Rob Buis]

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.

[Crystal Zhang]

Created attachment 140754 [details]
updated patch

[Crystal Zhang]

Comment on attachment 140754 [details]
updated patch

wrong patch

[Crystal Zhang]

Created attachment 140758 [details]
updated patch

[Rob Buis]

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

[Yong Li]

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.

[Crystal Zhang]

Created attachment 140781 [details]
updated patch

[Yong Li]

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.

[Crystal Zhang]

Created attachment 140790 [details]
updated patch

[WebKit Review Bot]

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.

[Crystal Zhang]

Created attachment 140791 [details]
updated patch

[Eric Seidel (no email)]

Comment on attachment 140790 [details]
updated patch

You should consider using webkit-patch upload, which will automatically obsolete your previous patches for you.

[Rob Buis]

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?

[Crystal Zhang]

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.

[WebKit Review Bot]

Comment on attachment 140791 [details]
updated patch

Clearing flags on attachment: 140791

Committed r116462: <http://trac.webkit.org/changeset/116462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.