Related to : Bug 24529.
Created attachment 61568 [details] WebKitAPI
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.
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.
Created attachment 61586 [details] WebKitAPI with tweeks
cc'ing joepeck too
... and fishd
public/WebApplicationCacheHost.h 111 ResourceInfo() : size(0), isMaster(false), isManifest(false), isExplicit(false), isForeign(false), isFallback(false) { } ^^^ Can we add url to this ?
Sorry, I meant can we add c'tor that assigns to members? Not a big deal.
Comment on attachment 61586 [details] WebKitAPI with tweeks Clearing flags on attachment: 61586 Committed r63440: <http://trac.webkit.org/changeset/63440>
All reviewed patches have been landed. Closing bug.
> 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.