[webkit-reviews] review denied: [Bug 122781] Refactor stored website data APIs : [Attachment 214294] fix gtk build

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied  review:
Bug 122781: Refactor stored website data APIs
https://bugs.webkit.org/show_bug.cgi?id=122781

Attachment 214294: fix gtk build
https://bugs.webkit.org/attachment.cgi?id=214294&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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.