Bug 122781 – Refactor stored website data APIs

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Hock]

There are a number of APIs in WebKit2 for dealing with stored website data:
WKApplicationCacheManager
WKCookieManager
WKDatabaseManager
WKKeyValueStorageManager
WKMediaCacheManager
WKPluginSiteDataManager
WKResourceCacheManager

We can consolidate these and simplify the implementation.

[Martin Hock]

Created attachment 214200 [details]
Add WebOriginDataManager and friends

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DerivedSources.make', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/OriginDataAcceptPolicy.h', u'Source/WebKit2/UIProcess/API/C/WKAPICast.h', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.messages.in', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.messages.in']" exit_code: 1
Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:47:  The parameter name "mask" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:50:  The parameter name "mask" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:51:  The parameter name "mask" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:76:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/API/C/WKContext.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:147:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:154:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:161:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 14 in 19 files


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

[Tim Horton]

Comment on attachment 214200 [details]
Add WebOriginDataManager and friends

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

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:44
> +    kWKOriginDataMaskApplicationCache = 01,
> +    kWKOriginDataMaskCookie = 02,
> +    kWKOriginDataMaskDatabase = 04,
> +    kWKOriginDataMaskKeyValueStorage = 010,
> +    kWKOriginDataMaskMediaCache = 020,
> +    kWKOriginDataMaskPluginData = 040,
> +    kWKOriginDataMaskResourceCache = 0100,
> +
> +    kWKOriginDataMaskAll = 0177

Probably this should use bitshifts like WKLayoutMilestones?

[EFL EWS Bot]

Comment on attachment 214200 [details]
Add WebOriginDataManager and friends

Attachment 214200 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3947948

[Martin Hock]

Created attachment 214204 [details]
Fix style errors

[EFL EWS Bot]

Comment on attachment 214204 [details]
Fix style errors

Attachment 214204 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4043080

[Tim Horton]

(In reply to comment #6)
> (From update of attachment 214204 [details])
> Attachment 214204 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/4043080


> CMakeFiles/WebKit2.dir/UIProcess/API/C/WKContext.cpp.o: In function `WKContextGetOriginDataManager':
> WKContext.cpp:(.text.WKContextGetOriginDataManager+0x5): undefined reference to `WebKit::WebOriginDataManagerProxy::supplementName()'

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 214200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214200&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:44
> > +    kWKOriginDataMaskApplicationCache = 01,
> > +    kWKOriginDataMaskCookie = 02,
> > +    kWKOriginDataMaskDatabase = 04,
> > +    kWKOriginDataMaskKeyValueStorage = 010,
> > +    kWKOriginDataMaskMediaCache = 020,
> > +    kWKOriginDataMaskPluginData = 040,
> > +    kWKOriginDataMaskResourceCache = 0100,
> > +
> > +    kWKOriginDataMaskAll = 0177
> 
> Probably this should use bitshifts like WKLayoutMilestones?

Definitely.

[Martin Hock]

(In reply to comment #8)
> (In reply to comment #3)
> > (From update of attachment 214200 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214200&action=review
> > 
> > > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:44
> > > +    kWKOriginDataMaskApplicationCache = 01,
> > > +    kWKOriginDataMaskCookie = 02,
> > > +    kWKOriginDataMaskDatabase = 04,
> > > +    kWKOriginDataMaskKeyValueStorage = 010,
> > > +    kWKOriginDataMaskMediaCache = 020,
> > > +    kWKOriginDataMaskPluginData = 040,
> > > +    kWKOriginDataMaskResourceCache = 0100,
> > > +
> > > +    kWKOriginDataMaskAll = 0177
> > 
> > Probably this should use bitshifts like WKLayoutMilestones?
> 
> Definitely.

I was being overly cautious about putting expressions in enums.  Apparently gcc with -std=c89 -pedantic thinks it's fine.

For the kWKOriginDataMaskAll value, I'll use the identity sum(i=0..n)(2^i) = 2^(n+1)-1, so in that case, kWKOriginDataMaskAll = (1 << 7) - 1 [parens are important!]

[Martin Hock]

Created attachment 214223 [details]
build fix + enum change

[EFL EWS Bot]

Comment on attachment 214223 [details]
build fix + enum change

Attachment 214223 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4025046

[Martin Hock]

Created attachment 214226 [details]
build fix

[EFL EWS Bot]

Comment on attachment 214226 [details]
build fix

Attachment 214226 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3578056

[kov's GTK+ EWS bot]

Comment on attachment 214223 [details]
build fix + enum change

Attachment 214223 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4067025

[Brady Eidson]

> /mnt/eflews/webkit/WebKit/Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:32:47: fatal error: WebOriginDataManagerProxyMessages.h: No such file or directory
 
EFL (apparently) isn't generating the messages.in file here.

[kov's GTK+ EWS bot]

Comment on attachment 214226 [details]
build fix

Attachment 214226 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3578077

[Martin Hock]

Created attachment 214235 [details]
another build fix

[EFL EWS Bot]

Comment on attachment 214235 [details]
another build fix

Attachment 214235 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3851007

[kov's GTK+ EWS bot]

Comment on attachment 214235 [details]
another build fix

Attachment 214235 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/3452047

[kov's GTK+ EWS bot]

Comment on attachment 214235 [details]
another build fix

Attachment 214235 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3723010

[Martin Hock]

Created attachment 214288 [details]
build fix + more stubs

[Martin Hock]

Created attachment 214294 [details]
fix gtk build

[Brady Eidson]

Comment on attachment 214294 [details]
fix gtk build

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

Lots and lots of plumbing out of the way, great start!  I'm going to be super picky on the exposed API before reviewing any further.

We should get Anders to look at this when he's around, and the 3 of us can even hash out ideas in person.

I do have some individual feedback that relegates this to an r-

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:42
> +    kWKOriginDataMaskApplicationCache = 1 << 0,
> +    kWKOriginDataMaskCookie = 1 << 1,
> +    kWKOriginDataMaskDatabase = 1 << 2,
> +    kWKOriginDataMaskKeyValueStorage = 1 << 3,
> +    kWKOriginDataMaskMediaCache = 1 << 4,
> +    kWKOriginDataMaskPluginData = 1 << 5,
> +    kWKOriginDataMaskResourceCache = 1 << 6,

I don't think "Mask" is a good component of one of these names, even if they are logically bit masks.

As mentioned, we could follow the previously established WKLayoutMilestone pattern for these bit masks.  This pattern includes not necessarily predicated each of these values with the same prefix.

I'd suggest something like:
kWKApplicationCacheOriginData,
kWKCookieOriginData,
etc etc

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:46
> +typedef uint32_t WKOriginDataMask;

And for the typedef here, I'd go with WKOriginDataTypes.

To me, just like with "WKLayoutMilestones", the pluralization of "Types" illustrates that one variable might reflect multiple values in the mask.

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:53
> +enum {
> +    kWKOriginDataAcceptPolicyAlways = 0,
> +    kWKOriginDataAcceptPolicyNever = 1,
> +    kWKOriginDataAcceptPolicyOnlyFromMainDocumentDomain = 2
> +};
> +typedef uint32_t WKOriginDataAcceptPolicy;

This is primarily about cookies, and has nothing to do with origins.  I think the WKCookieManager can live on and provide only this functionality.

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:77
> +// Currently, only mask == WKOriginDataManagerCookie is supported for the following.
> +WK_EXPORT void WKOriginDataManagerSetAcceptPolicy(WKOriginDataManagerRef originDataManager, WKOriginDataMask mask, WKOriginDataAcceptPolicy policy);
> +typedef void (*WKOriginDataManagerGetAcceptPolicyFunction)(WKOriginDataAcceptPolicy, WKErrorRef, void*);
> +WK_EXPORT void WKOriginDataManagerGetAcceptPolicy(WKOriginDataManagerRef originDataManager, WKOriginDataMask mask, void* context, WKOriginDataManagerGetAcceptPolicyFunction callback);

As mentioned above, these can live only in the cookie manager.

[Martin Hock]

Created attachment 214312 [details]
Address Brady's comments

[Martin Hock]

Created attachment 214318 [details]
Fix merge conflict

[Martin Hock]

Created attachment 214337 [details]
Rebase patch

[kov's GTK+ EWS bot]

Comment on attachment 214337 [details]
Rebase patch

Attachment 214337 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/4106249

[Martin Hock]

Created attachment 214378 [details]
gtk-wk2 build fix

[Martin Hock]

Created attachment 214387 [details]
rebase

[Martin Hock]

Created attachment 214395 [details]
typo in GNUmakefile.list.am

[Brady Eidson]

Comment on attachment 214395 [details]
typo in GNUmakefile.list.am

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

Looks good overall.  One thing that gets an r- from me, but should be easy to address.

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:46
> +typedef uint32_t WKOriginDataTypes;

Since we never really store these and aren't concerned about memory usage/etc, and since we'd like to make this one API as future proof as possible, let's make this a uint64_t here and everywhere else that uses it.

Otherwise we'd be cursing ourselves when we add the 33rd data type...

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:85
> +void WebOriginDataManager::deleteEntriesForOrigin(WKOriginDataTypes types, const SecurityOriginData& originData)
> +{
> +    RefPtr<SecurityOrigin> origin = SecurityOrigin::create(originData.protocol, originData.host, originData.port);
> +    if (!origin)
> +        return;
> +
> +    // FIXME: delete cache

Not really "delete cache", it's more than that!

[Brady Eidson]

(In reply to comment #31)
> (From update of attachment 214395 [details])
> > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:46
> > +typedef uint32_t WKOriginDataTypes;
> 
> Since we never really store these and aren't concerned about memory usage/etc, and since we'd like to make this one API as future proof as possible, let's make this a uint64_t here and everywhere else that uses it.
> 
> Otherwise we'd be cursing ourselves when we add the 33rd data type...

Because of the C-ness of the API, this won't work.  we'll leave as-is.

[WebKit Commit Bot]

Comment on attachment 214395 [details]
typo in GNUmakefile.list.am

Rejecting attachment 214395 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 214395, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
nes).
1 out of 17 hunks FAILED -- saving rejects to file Source/WebKit2/WebKit2.xcodeproj/project.pbxproj.rej
patching file Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp
patching file Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h
patching file Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.messages.in

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brady Eidson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/3856063

[Martin Hock]

Created attachment 214494 [details]
rebase + fix comment

[WebKit Commit Bot]

Comment on attachment 214494 [details]
rebase + fix comment

Clearing flags on attachment: 214494

Committed r157595: <http://trac.webkit.org/changeset/157595>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.