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

[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 118944: Patch for notification support. V8.
https://bugs.webkit.org/attachment.cgi?id=118944&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.