Bug 85670 - [BlackBerry] Implement a popup client for HTML controls
Summary: [BlackBerry] Implement a popup client for HTML controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-04 14:39 PDT by Crystal Zhang
Modified: 2012-05-08 16:00 PDT (History)
5 users (show)

See Also:


Attachments
patch (14.98 KB, patch)
2012-05-04 15:48 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
fix style (14.97 KB, patch)
2012-05-04 16:08 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
updated patch (15.19 KB, patch)
2012-05-07 14:12 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
updated patch (7.42 KB, patch)
2012-05-08 11:45 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
updated patch (15.70 KB, patch)
2012-05-08 11:56 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
updated patch (15.59 KB, patch)
2012-05-08 14:14 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
updated patch (15.64 KB, patch)
2012-05-08 14:45 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
updated patch (15.64 KB, patch)
2012-05-08 14:53 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Crystal Zhang 2012-05-04 14:39:00 PDT
popup client.
Comment 1 Crystal Zhang 2012-05-04 15:48:45 PDT
Created attachment 140346 [details]
patch
Comment 2 WebKit Review Bot 2012-05-04 15:52:47 PDT
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.
Comment 3 Crystal Zhang 2012-05-04 16:08:49 PDT
Created attachment 140351 [details]
fix style
Comment 4 Rob Buis 2012-05-06 16:40:28 PDT
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.
Comment 5 Crystal Zhang 2012-05-07 14:12:36 PDT
Created attachment 140589 [details]
updated patch
Comment 6 Rob Buis 2012-05-07 14:25:33 PDT
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.
Comment 7 Crystal Zhang 2012-05-08 11:45:15 PDT
Created attachment 140754 [details]
updated patch
Comment 8 Crystal Zhang 2012-05-08 11:46:28 PDT
Comment on attachment 140754 [details]
updated patch

wrong patch
Comment 9 Crystal Zhang 2012-05-08 11:56:28 PDT
Created attachment 140758 [details]
updated patch
Comment 10 Rob Buis 2012-05-08 12:12:12 PDT
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->
....
Comment 11 Yong Li 2012-05-08 12:29:37 PDT
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.
Comment 12 Crystal Zhang 2012-05-08 14:14:38 PDT
Created attachment 140781 [details]
updated patch
Comment 13 Yong Li 2012-05-08 14:22:38 PDT
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.
Comment 14 Crystal Zhang 2012-05-08 14:45:24 PDT
Created attachment 140790 [details]
updated patch
Comment 15 WebKit Review Bot 2012-05-08 14:47:44 PDT
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.
Comment 16 Crystal Zhang 2012-05-08 14:53:50 PDT
Created attachment 140791 [details]
updated patch
Comment 17 Eric Seidel (no email) 2012-05-08 15:08:10 PDT
Comment on attachment 140790 [details]
updated patch

You should consider using webkit-patch upload, which will automatically obsolete your previous patches for you.
Comment 18 Rob Buis 2012-05-08 15:10:40 PDT
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 19 Crystal Zhang 2012-05-08 15:18:13 PDT
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 20 WebKit Review Bot 2012-05-08 16:00:26 PDT
Comment on attachment 140791 [details]
updated patch

Clearing flags on attachment: 140791

Committed r116462: <http://trac.webkit.org/changeset/116462>
Comment 21 WebKit Review Bot 2012-05-08 16:00:33 PDT
All reviewed patches have been landed.  Closing bug.