[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 #118944|review?                     |review-
               Flag|                            |




--- Comment #20 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-13 01:03:49 PST ---
(From update of attachment 118944)
View in context: https://bugs.webkit.org/attachment.cgi?id=118944&action=review

Almost there, I still have some style nitpicks, and give r- as the ChangeLog doesn't contain any information. At least one line should be added.

> Source/WebKit/ChangeLog:7
> +

Please add at least one or two lines to the ChangeLog, sth. like "Initial upstreaming, no new tests."

> Source/WebKit/ChangeLog:20
> +        * blackberry/WebCoreSupport/NotificationPresenterImpl.h: Added.

You could explain the file naming here.
I still don't see why we don't name it NotificationPresenterBlackBerry.h, or sth. similar. But it's okay for now.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:23
> +

One newline too much.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:33
> +

One newline too much.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:39
> +        s_instance = new NotificationPresenterImpl();

s/()//

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:61
> +    if (checkPermission(notification->scriptExecutionContext()) != NotificationPresenter::PermissionAllowed)
> +        return false;
> +
> +    RefPtr<Notification> n = notification;
> +    if (m_notifications.contains(n))
> +        return false;

Which case is simpler to compute? I assume the latter, so we should swap these.

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

Even if Dan suggested to add this, I'm suggesting to remove this :-)
There is no benefit of adding this, and it's also leading to compiler warnings under certain circumstances IIRC.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:23
> +

This newline can be removed.

> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:84
> +

This newline can be removed.

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