Bug 193323 - Add a new SPI to request for cache storage quota increase
Summary: Add a new SPI to request for cache storage quota increase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 193296
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-10 09:39 PST by youenn fablet
Modified: 2019-01-20 15:07 PST (History)
6 users (show)

See Also:


Attachments
Patch (34.42 KB, patch)
2019-01-10 11:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (18.08 KB, patch)
2019-01-16 12:41 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.97 KB, patch)
2019-01-16 12:43 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.00 KB, patch)
2019-01-17 10:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (29.96 KB, patch)
2019-01-17 11:17 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.29 KB, patch)
2019-01-17 12:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.20 KB, patch)
2019-01-17 14:29 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (39.33 KB, patch)
2019-01-17 16:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (39.38 KB, patch)
2019-01-18 09:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-01-10 09:39:08 PST
Add a new SPI to request for cache storage quota increase
Comment 1 youenn fablet 2019-01-10 11:22:43 PST
Created attachment 358810 [details]
Patch
Comment 2 youenn fablet 2019-01-16 12:41:18 PST
Created attachment 359292 [details]
Patch
Comment 3 youenn fablet 2019-01-16 12:43:14 PST
Created attachment 359293 [details]
Patch
Comment 4 Build Bot 2019-01-16 17:21:10 PST
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.
Comment 5 youenn fablet 2019-01-17 10:04:59 PST
Created attachment 359386 [details]
Patch
Comment 6 Build Bot 2019-01-17 10:07:27 PST
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.
Comment 7 youenn fablet 2019-01-17 11:17:21 PST
Created attachment 359394 [details]
Patch
Comment 8 Build Bot 2019-01-17 11:20:54 PST
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.
Comment 9 Alex Christensen 2019-01-17 11:23:41 PST
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.
Comment 10 youenn fablet 2019-01-17 12:03:16 PST
Created attachment 359400 [details]
Patch
Comment 11 Build Bot 2019-01-17 12:05:05 PST
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.
Comment 12 Alex Christensen 2019-01-17 13:05:21 PST
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
Comment 13 youenn fablet 2019-01-17 14:29:40 PST
Created attachment 359413 [details]
Patch
Comment 14 Build Bot 2019-01-17 14:31:06 PST
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.
Comment 15 youenn fablet 2019-01-17 14:32:09 PST
> > 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
Comment 16 youenn fablet 2019-01-17 16:59:34 PST
Created attachment 359428 [details]
Patch
Comment 17 Build Bot 2019-01-17 17:02:21 PST
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.
Comment 18 Alex Christensen 2019-01-17 21:46:58 PST
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.
Comment 19 youenn fablet 2019-01-18 09:04:10 PST
Created attachment 359497 [details]
Patch for landing
Comment 20 Build Bot 2019-01-18 09:07:20 PST
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.
Comment 21 WebKit Commit Bot 2019-01-18 10:19:47 PST
Comment on attachment 359497 [details]
Patch for landing

Clearing flags on attachment: 359497

Committed r240156: <https://trac.webkit.org/changeset/240156>
Comment 22 WebKit Commit Bot 2019-01-18 10:19:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2019-01-18 10:20:36 PST
<rdar://problem/47388418>