Bug 193323 – Add a new SPI to request for cache storage quota increase

• Component: WebKit API
• Status: RESOLVED FIXED

[youenn fablet]

Add a new SPI to request for cache storage quota increase

[youenn fablet]

Created attachment 358810 [details]
Patch

[youenn fablet]

Created attachment 359292 [details]
Patch

[youenn fablet]

Created attachment 359293 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:74:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


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

[youenn fablet]

Created attachment 359386 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:74:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


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

[youenn fablet]

Created attachment 359394 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:74:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


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

[Alex Christensen]

Comment on attachment 359394 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKStorageQuotaDelegatePrivate.h:32
> +@protocol WKStorageQuotaDelegatePrivate <NSObject>

_WKWebsiteDataStoreDelegate since this is not public API, then the callback selectors don't need underscores.
Also, needs availability macros.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStoreInternal.h:44
> +    RetainPtr<id <WKStorageQuotaDelegatePrivate> > _quotaDelegate;

Other delegates use a WeakObjCPtr.  I see no compelling reason to not do the same here, especially since the ObjC property is labeled weak.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreQuotaManager.h:36
> +class WebsiteDataStoreQuotaManager {

Why not make this more general so we can add other callbacks here? WebsiteDataStoreDelegate

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreQuotaManager.h:40
> +    virtual void requestCacheStorageSpace(const WebCore::SecurityOriginData& topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota, uint64_t currentSize, uint64_t spaceRequired, WTF::CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)

WTF:: is unnecessary here.

> LayoutTests/http/wpt/cache-storage/cache-quota.any.js:157
>     }).then(() => {

You could also add a space here.

[youenn fablet]

Created attachment 359400 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:74:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


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

[Alex Christensen]

Comment on attachment 359400 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:48
> +class WebsiteDataStoreManager : public WebKit::WebsiteDataStoreManager {

I like "Client" more than "Manager"

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreManager.h:36
> +class WebsiteDataStoreManager {

This doesn't really manage anything.  It's more of a client.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreManager.h:42
> +        completionHandler({ });

WTF::nullopt

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:9838
> +				4118DC1F21E7BF5D00DE04C7 /* _WKWebsiteDataStoreDelegate.h in Headers */,
> +				4118DC1F21E7BF5D00DE04C7 /* _WKWebsiteDataStoreDelegate.h in Headers */,

This should probably not be duplicated.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:58
> +@interface WebsiteDataStoreDelegateManager: NSObject <_WKWebsiteDataStoreDelegate> {

This doesn't manage delegates.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:60
> +    BOOL shouldAllowRaisingQuota;

ivars typically start with an underscore.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:116
> +    globalWebsiteDataStoreDelegateManager = [[WebsiteDataStoreDelegateManager alloc] init];

It seems like this should be owned by the TestController.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:293
> +    globalWebsiteDataStoreDelegateManager->shouldAllowRaisingQuota = false;

This should call a setter.

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:430
> +    globalWebsiteDataStoreDelegateManager->shouldAllowRaisingQuota = true;

ditto

[youenn fablet]

Created attachment 359413 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm:40:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


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

[youenn fablet]

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:48
> > +class WebsiteDataStoreManager : public WebKit::WebsiteDataStoreManager {
> 
> I like "Client" more than "Manager"

Moved from Manager to Client.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreManager.h:42
> > +        completionHandler({ });
> 
> WTF::nullopt

I kept { }.
This is equivalent, terse and is not WTF specific.

> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:9838
> > +				4118DC1F21E7BF5D00DE04C7 /* _WKWebsiteDataStoreDelegate.h in Headers */,
> > +				4118DC1F21E7BF5D00DE04C7 /* _WKWebsiteDataStoreDelegate.h in Headers */,
> 
> This should probably not be duplicated.

Not sure what happened there.


> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:60
> > +    BOOL shouldAllowRaisingQuota;
> 
> ivars typically start with an underscore.

OK

> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:116
> > +    globalWebsiteDataStoreDelegateManager = [[WebsiteDataStoreDelegateManager alloc] init];
> 
> It seems like this should be owned by the TestController.

TestController is fully C++ so I am not really sure how to handle the includes properly.
I kept it as a global.

> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:293
> > +    globalWebsiteDataStoreDelegateManager->shouldAllowRaisingQuota = false;
> 
> This should call a setter.

OK

[youenn fablet]

Created attachment 359428 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm:40:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


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

[Alex Christensen]

Comment on attachment 359428 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreDelegate.h:32
> +@protocol _WKWebsiteDataStoreDelegate <NSObject>

This needs availability macros.

[youenn fablet]

Created attachment 359497 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm:40:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


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

[WebKit Commit Bot]

Comment on attachment 359497 [details]
Patch for landing

Clearing flags on attachment: 359497

Committed r240156: <https://trac.webkit.org/changeset/240156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47388418>