[webkit-reviews] review denied: [Bug 56415] ApplicationCacheGroup is not obsolete after being deleted via ApplicationCacheStorage::deleteEntriesForOrigin : [Attachment 86086] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Anton D'Auria
<adauria at apple.com>'s request for review:
Bug 56415: ApplicationCacheGroup is not obsolete after being deleted via
ApplicationCacheStorage::deleteEntriesForOrigin
https://bugs.webkit.org/show_bug.cgi?id=56415

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86086&action=review

Much nicer! I have some nits below, but there are some things that I think
would prevent
landing. r- for these:

  • This test will fail on ports that have not implemented
layoutTestController.clearApplicationCacheForOrigin.
    It would be good to add the test to those ports Skipped list, with bugzilla
bug + comment.

  • This appears to update
LayoutTests/http/tests/appcache/resources/origin-usage-iframe-1.html but
    the file does not show up in the patch. It also might be worth creating a
new resource, so that
    if a test changes it doesn't accidentally break the other test.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:32
>  #import <WebCore/ApplicationCacheStorage.h>
> +#import <WebCore/ApplicationCacheGroup.h>

Nit: alphabetically swap these two. (Might have been my fault).

> Tools/DumpRenderTree/LayoutTestController.h:53
>      void clearAllApplicationCaches();
> +    void clearApplicationCacheForOrigin(JSStringRef name);
>      void clearAllDatabases();

Nit: probably best to be alphabetical here too, but no big deal.

> LayoutTests/ChangeLog:9
> +	   These tests create appcache for the DRT origin, delete appcache for
the origin,
> +	   and verify that the appcache has obsolete status.

Nit: "These test create" => "This test creates". I also tend to prefix
Application Cache with "the" or "an", but that may be personal preference.

> LayoutTests/http/tests/appcache/origin-delete.html:37
> +	   if (event.data == "PASS - cached iframe-1") {

Nit: With JavaScript, almost always, ==/!= can be ===/!==. It makes it clearer
as well. There are a couple cases here.

> LayoutTests/http/tests/appcache/origin-delete.html:42
> +	       var getStatus = "appcache_status";
> +	       frame.contentWindow.postMessage(getStatus, "*");

Nit: No need for the temp variable. It doesn't add anything extra here.