[Webkit-unassigned] [Bug 73194] [BlackBerry] Add notification support for the BlackBerry port

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118335|review?                     |review-
               Flag|                            |




--- Comment #13 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-10 00:22:20 PST ---
(From update of attachment 118335)
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/

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