Bug 41993 – WebKit API for WebInspector-Appcache integration for chrome.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Kavita Kanetkar]

Related to : Bug 24529.

[Michael Nordman]

Created attachment 61568 [details]
WebKitAPI

[Darin Fisher (:fishd, Google)]

Comment on attachment 61568 [details]
WebKitAPI

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.

[Michael Nordman]

Thnx for looking.

(In reply to comment #2)
> (From update of attachment 61568 [details])
> 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?

CacheInfo it is!

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

will do

> 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.

hmmm... then i suppose i should change all the others as well (yes?)

> 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.

ok, will do

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

duh

> 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.

Because the action will be to delete everything related to the manifestURL. In appcache speak, that's an ApplicationCacheGroup. I'd be ok with using deleteAssociatedCache shorthand here, but really the addition of "Group" is more descriptive.

[Michael Nordman]

Created attachment 61586 [details]
WebKitAPI with tweeks

[Michael Nordman]

cc'ing joepeck too

[Michael Nordman]

... and fishd

[Kavita Kanetkar]

public/WebApplicationCacheHost.h 111
ResourceInfo() : size(0), isMaster(false), isManifest(false), isExplicit(false), isForeign(false), isFallback(false) { }

^^^
Can we add url to this ?

[Kavita Kanetkar]

Sorry, I meant can we add c'tor that assigns to members? Not a big deal.

[WebKit Commit Bot]

Comment on attachment 61586 [details]
WebKitAPI with tweeks

Clearing flags on attachment: 61586

Committed r63440: <http://trac.webkit.org/changeset/63440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

> Sorry, I meant can we add c'tor that assigns to members? Not a big deal.

I think we generally don't like to put code in these .h file, just the interfaces, i was 'iffy' on adding the default ctor already. Anyway that's why i didn't put another ctor there.