[Webkit-unassigned] [Bug 85670] [BlackBerry] Implement a popup client for HTML controls

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85670


Yong Li <yong.li.webkit at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yong.li.webkit at gmail.com




--- Comment #11 from Yong Li <yong.li.webkit at gmail.com>  2012-05-08 12:29:37 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.