[webkit-reviews] review denied: [Bug 41993] WebKit API for WebInspector-Appcache integration for chrome. : [Attachment 61568] WebKitAPI

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 41993: WebKit API for WebInspector-Appcache integration for chrome.
https://bugs.webkit.org/show_bug.cgi?id=41993

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebApplicationCacheHost.h:96
 +	struct ApplicationCacheInfo {
this struct is contained in the WebApplicationCacheHost "namespace"
so the ApplicationCache prefix here is a bit redundant.  however,
just calling it Info seems too vague.  maybe CacheInfo is enough
since you use the method name getAssociatedCacheInfo?


WebKit/chromium/public/WebApplicationCacheHost.h:97
 +	    WebURL manifestUrl; // Empty if there is no associated cache.
nit: manifestUrl -> manifestURL

WebKit/chromium/public/WebApplicationCacheHost.h:113
 +	// FIXME: Make these pure virtual after chrome has caught up.
Actually, our convention for WebKit API interfaces that are
implemented by the embedder is that they should not have pure
virtual methods.

WebKit/chromium/public/WebApplicationCacheHostClient.h:43
 +	virtual void didChangeCacheAssociation() { return; } // FIXME: Make
this pure virtual.
this one can be pure virtual now since you can provide a dummy
implementation in ApplicationCacheHostInternal.h.

WebKit/chromium/public/WebApplicationCacheHost.h:114
 +	virtual void getAssociatedCacheInfo(ApplicationCacheInfo*) { return; }
style-nit: no need for the return statements

WebKit/chromium/public/WebApplicationCacheHost.h:116
 +	virtual void deleteAssociatedCacheGroup() { return; }
it is unclear to me why this is called deleteAssociatedCacheGroup
and not deleteAssociatedCache given the method named
getAssociatedCacheInfo.