[Webkit-unassigned] [Bug 85670] [BlackBerry] Implement a popup client for HTML controls
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 8 12:30:33 PDT 2012
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.
More information about the webkit-unassigned
mailing list