[webkit-reviews] review denied: [Bug 73194] [BlackBerry] Add notification support for the BlackBerry port : [Attachment 118335] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Robin Qiu
<robin.qiu at torchmobile.com.cn>'s request for review:
Bug 73194: [BlackBerry] Add notification support for the BlackBerry port
https://bugs.webkit.org/show_bug.cgi?id=73194

Attachment 118335: Patch
https://bugs.webkit.org/attachment.cgi?id=118335&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118335&action=review


Looks good to me, still some nits that lead to r-.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:34
> +NotificationPresenterImpl* NotificationPresenterImpl::s_instance = 0;

No need for this, just move the static s_instance declaration into ::instance()
- this way it doesn't need to be present in the header as well.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:36
> +NotificationPresenter* NotificationPresenterImpl::instance()

Why is this named *Impl - we generally try to avoid this idiom in WebCore, but
as this is WebKit it might be fine. Do you plan to add more *Impl.cpp named
files?

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:55
> +    if (NotificationPresenter::PermissionAllowed !=
checkPermission(notification->scriptExecutionContext()))

This reads awkward, I'd swap the order here.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:70
> +	   message = makeString(n->contents().title(), ": ",
n->contents().body());

As I mentioned to Dan before, your old variant was fine as well, you could
revert to it, this is no gain.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:83
> +    if (m_notifications.contains(n)) {
> +	  
m_platformPresenter->cancel(std::string(m_notifications.get(n).utf8().data()));

> +	   m_notifications.remove(n);

This idiom is slow, don't use contains to check, and then remove to query the
hash table again. Instead use m_notifications.find(n) which will return you an
iterator, if it's map.end() you can early exit, otherwhise you can cancel the
presenter and call remove(it);

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:94
> +{

No ASSERT(context) here?

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:103
> +    PermissionRequestMap::iterator iter = m_permissionRequests.begin();

s/iter/it/ - as this is used all over WebCore in this way :-) About the only
tolerated abbreviations that we use.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:104
> +    for (; iter != m_permissionRequests.end(); ++iter) {

If this is hot code, you could cache m_permissionRequests.end(), but I doubt
it.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:105
> +	   if (iter->first->url().host() == domainString) {

Early continue; here if it doesn't match.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:121
> +    return;

Unncessary.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:126
> +    // FIXME: Should store the permission information into file permanently
instead of in m_allowedDomains.

ASSERT(context) ?

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:139
> +    NotificationMap::iterator iter = m_notifications.begin();

s/iter/it/