[webkit-reviews] review granted: [Bug 108196] Notification.requestPermission callback should be optional : [Attachment 190382] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has granted Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 108196: Notification.requestPermission callback should be optional
https://bugs.webkit.org/show_bug.cgi?id=108196

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190382&action=review


The change looks good. A couple of comments on the test.

>
LayoutTests/fast/notifications/notifications-request-permission-optional.html:9

> +function log(message)
> +{
> +    document.getElementById("result").innerHTML += message + "<br>";
> +}

Nit: Remove this.

>
LayoutTests/fast/notifications/notifications-request-permission-optional.html:1
4
> +    if (window.testRunner)
> +	   testRunner.dumpAsText();
> +

Nit: This is not needed.

>
LayoutTests/fast/notifications/notifications-request-permission-optional.html:1
6
> +    if (!window.webkitNotifications)
> +	   log("FAIL: No webkitNotification interface!");

You can write: shouldNotBe('!window.webkitNotifications');

>
LayoutTests/fast/notifications/notifications-request-permission-optional.html:2
5
> +runTests();

Nit: Now the test is just two lines. You can directly write the test here.

>
LayoutTests/fast/notifications/notifications-request-permission-optional.html:2
7
> +</body>

Please add <script src="../js/resources/js-test-post.js"></script>