Bug 54514 – Need API to view origins which have items in the Application Cache and delete items for those origins

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Need API to view origins which have items in the Application Cache and delete items for those origins

In radar as <rdar://problem/8762042>

[Brady Eidson]

Created attachment 83344 [details]
Patch v1

[Brady Eidson]

Stubbing the WebCore methods for now - just want to get all the huge API gloop in place first.

[WebKit Review Bot]

Attachment 83344 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:54:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/WebApplicationCacheManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Comment on attachment 83344 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=83344&action=review

Make sure that at least the code that calls into WebCore is guarded with #if ENABLE(OFFLINE_WEB_APPLICATIONS), I don't think all of this code needs to be guarded, as I don't think it hurts to expose the API either way, but another reviewer might want to weigh in on it.

Patch looks really good overall, I really like the generic callback function you added.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1293
> +    // FIXME <rdar://problem/8762042> and https://bugs.webkit.org/show_bug.cgi?id=54514 - Implement.

Nit: Our style has FIXME: ...

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1299
> +    // FIXME <rdar://problem/8762042> and https://bugs.webkit.org/show_bug.cgi?id=54514 - Implement.

Ditto about FIXME:

> Source/WebKit2/ChangeLog:9
> +        it's sorting was wrong due to a last minute name change.

Thanks!

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:41
> +    toImpl(ApplicationCacheManagerRef)->getApplicationCacheOrigins(ArrayCallback::create(context, callback));

The variable name should be applicationCacheManagerRef (or just applicationCacheManager, like your header file).

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:46
> +    toImpl(ApplicationCacheManagerRef)->deleteEntriesForOrigin(toImpl(originRef));

Ditto.

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:51
> +    toImpl(ApplicationCacheManagerRef)->deleteAllEntries();

Ditto.

> Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.cpp:93
> +    cacheStorage().vacuumDatabaseFile();

Should these two function calls be their own function? (Like ApplicationCache::deleteAllEntries)? Seems strange for WebKit to know about vacuuming database files.

[Brady Eidson]

Created attachment 83346 [details]
Patch v2 (With critical ENABLE(OFFLINE_WEB_APPLICATIONS) protectors in places that might break the build for some platforms)

Brian reminded me we need #if ENABLE(OFFLINE_WEB_APPLICATIONS) flags here.

I don't know how feasible it is to properly #if ENABLE(OFFLINE_WEB_APPLICATIONS) everything added here, especially with regards to API and *messages.in files.

This patch adds critical #if ENABLE(OFFLINE_WEB_APPLICATIONS) guards to the WebCore callsites that will at least keep everyone building, and will behave as if the app cache is there, but is just always empty.

[WebKit Review Bot]

Attachment 83346 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:54:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/WebApplicationCacheManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 83344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83344&action=review
> 
> Make sure that at least the code that calls into WebCore is guarded with #if ENABLE(OFFLINE_WEB_APPLICATIONS), I don't think all of this code needs to be guarded, as I don't think it hurts to expose the API either way, but another reviewer might want to weigh in on it.
> 
> Patch looks really good overall, I really like the generic callback function you added.
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1293
> > +    // FIXME <rdar://problem/8762042> and https://bugs.webkit.org/show_bug.cgi?id=54514 - Implement.
> 
> Nit: Our style has FIXME: ...
> 
> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1299
> > +    // FIXME <rdar://problem/8762042> and https://bugs.webkit.org/show_bug.cgi?id=54514 - Implement.
> 
> Ditto about FIXME:

Done.

> 
> > Source/WebKit2/ChangeLog:9
> > +        it's sorting was wrong due to a last minute name change.
> 
> Thanks!

YVW

> 
> > Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:41
> > +    toImpl(ApplicationCacheManagerRef)->getApplicationCacheOrigins(ArrayCallback::create(context, callback));
> 
> The variable name should be applicationCacheManagerRef (or just applicationCacheManager, like your header file).
> 
> > Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:46
> > +    toImpl(ApplicationCacheManagerRef)->deleteEntriesForOrigin(toImpl(originRef));
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:51
> > +    toImpl(ApplicationCacheManagerRef)->deleteAllEntries();
> 
> Ditto.

Actually, what I've done follows the same idiom we follow in a vast majority of our WK* api implementations.  :)

> 
> > Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.cpp:93
> > +    cacheStorage().vacuumDatabaseFile();
> 
> Should these two function calls be their own function? (Like ApplicationCache::deleteAllEntries)? Seems strange for WebKit to know about vacuuming database files.

Fair enough.  WebKit1 knows about it, but it shouldn't either.

I'm gonna do the WebCore patch, that includes that change, and a WebKit1 change to use it, then I'll come back to the WK2 patch.

[Brady Eidson]

Created attachment 83348 [details]
WebCore-only patch (with WK1 change)

[Brian Weinstein]

Comment on attachment 83348 [details]
WebCore-only patch (with WK1 change)

This looks good. Unofficial r=me

[Brady Eidson]

Landed the WebCore/WK1 patch in 79344

New WK2 patch coming up.

[Brady Eidson]

Created attachment 83354 [details]
Patch v3 (no more pending WebCore changes, addresses all of Brian's v1 comments)

[WebKit Review Bot]

Attachment 83354 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebApplicationCacheManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Comment on attachment 83354 [details]
Patch v3 (no more pending WebCore changes, addresses all of Brian's v1 comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=83354&action=review

Make sure to add your new directory to WebKit2Common.vsprops, and to add WKApplicationCacheManager.h to WebKit2Generated.make (in Source/WebKit2/win).

Looks good!

> Source/WebKit2/ChangeLog:8
> +        I couldn't help myself - I also re-alphabatized WebResourceCacheManager in many places where

I think it's re-alphabetized.

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:39
> +void WKApplicationCacheManagerGetApplicationCacheOrigins(WKApplicationCacheManagerRef ApplicationCacheManagerRef, void* context, WKApplicationCacheManagerGetApplicationCacheOriginsFunction callback)

Variable name (ApplicationCacheManagerRef) should be lower case here.

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:44
> +void WKApplicationCacheManagerDeleteEntriesForOrigin(WKApplicationCacheManagerRef ApplicationCacheManagerRef, WKSecurityOriginRef originRef)

Ditto.

> Source/WebKit2/UIProcess/API/C/WKApplicationCacheManager.cpp:49
> +void WKApplicationCacheManagerDeleteAllEntries(WKApplicationCacheManagerRef ApplicationCacheManagerRef)

Ditto.

[Build Bot]

Attachment 83346 [details] did not build on win:
Build output: http://queues.webkit.org/results/7946319

[Brady Eidson]

Created attachment 83356 [details]
Patch v4 - With Brian's latest feedback

[WebKit Review Bot]

Attachment 83356 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebApplicationCacheManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 83354 [details] did not build on win:
Build output: http://queues.webkit.org/results/7944354

[Build Bot]

Attachment 83356 [details] did not build on win:
Build output: http://queues.webkit.org/results/7949187

[Brady Eidson]

Created attachment 83368 [details]
Patch v5 - Fix Windows build - that header isn't needed here anymore, anyways!

[WebKit Review Bot]

Attachment 83368 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebApplicationCacheManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Landed in r79364

Now to implement the new functionality in WebCore

[Anders Carlsson]

Committed r79366: <http://trac.webkit.org/changeset/79366>

[Brady Eidson]

Created attachment 83407 [details]
Final patch v1 - Implement the WebCore stubs

[Brady Eidson]

Fixed in r79368

[Csaba Osztrogonác]

Qt buildfix landed in http://trac.webkit.org/changeset/79370