[webkit-reviews] review denied: [Bug 56722] Add +[WebApplicationCache getOriginsWithCache] : [Attachment 86429] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Anton D'Auria
<adauria at apple.com>'s request for review:
Bug 56722: Add +[WebApplicationCache getOriginsWithCache]
https://bugs.webkit.org/show_bug.cgi?id=56722

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86429&action=review

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:40
> +typedef HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>
SecurityOriginSet;

I now see why you were unsure about adding this to a header. It sure feels
strange to have a SecurityOriginSet definition in ApplicationCacheStorage.h!

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.h:41
> ++ (NSArray *)originsWithCache;

Maybe originsWithCaches? I'm not sure.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:76
> +    NSMutableArray *webOrigins = [[NSMutableArray alloc]
initWithCapacity:coreOrigins.size()];

Please always use RetainPtr.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:80
> +	   RetainPtr<WebSecurityOrigin> webOrigin = [[WebSecurityOrigin alloc]
_initWithWebCoreSecurityOrigin:(*it).get()];

This needs to be RetainPtr<WebSecurityOrigin> webOrigin(AdoptNS, ...).