Summary: | Need API to view origins which have items in the Application Cache and delete items for those origins | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | andersca, buildbot, ossy, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2011-02-15 17:39:18 PST
Created attachment 83344 [details]
Patch v1
Stubbing the WebCore methods for now - just want to get all the huge API gloop in place first. 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.
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. 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.
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.
(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. Created attachment 83348 [details]
WebCore-only patch (with WK1 change)
Comment on attachment 83348 [details]
WebCore-only patch (with WK1 change)
This looks good. Unofficial r=me
Landed the WebCore/WK1 patch in 79344 New WK2 patch coming up. Created attachment 83354 [details]
Patch v3 (no more pending WebCore changes, addresses all of Brian's v1 comments)
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.
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. Attachment 83346 [details] did not build on win: Build output: http://queues.webkit.org/results/7946319 Created attachment 83356 [details]
Patch v4 - With Brian's latest feedback
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.
Attachment 83354 [details] did not build on win: Build output: http://queues.webkit.org/results/7944354 Attachment 83356 [details] did not build on win: Build output: http://queues.webkit.org/results/7949187 Created attachment 83368 [details]
Patch v5 - Fix Windows build - that header isn't needed here anymore, anyways!
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.
Landed in r79364 Now to implement the new functionality in WebCore Committed r79366: <http://trac.webkit.org/changeset/79366> Created attachment 83407 [details]
Final patch v1 - Implement the WebCore stubs
Fixed in r79368 Qt buildfix landed in http://trac.webkit.org/changeset/79370 |