Bug 198133 - Set default WebsiteDataStore storage quota based on StorageQuotaManager
Summary: Set default WebsiteDataStore storage quota based on StorageQuotaManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-22 10:56 PDT by youenn fablet
Modified: 2019-05-23 11:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2019-05-22 10:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2019-05-22 16:23 PDT, 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-05-22 10:56:26 PDT
Set default WebsiteDataStore storage quota based on StorageQuotaManager
Comment 1 youenn fablet 2019-05-22 10:57:27 PDT
<rdar://problem/51031436>
Comment 2 youenn fablet 2019-05-22 10:59:27 PDT
Created attachment 370422 [details]
Patch
Comment 3 Geoffrey Garen 2019-05-22 11:18:06 PDT
Can you add a test case? Seems like an API test could detect whether a permission delegate message was sent or not. Or you could use an html test case, with WebKitTestRunner configured to log or fail when sent the permission delegate message.
Comment 4 youenn fablet 2019-05-22 14:09:53 PDT
(In reply to Geoffrey Garen from comment #3)
> Can you add a test case? Seems like an API test could detect whether a
> permission delegate message was sent or not. Or you could use an html test
> case, with WebKitTestRunner configured to log or fail when sent the
> permission delegate message.

WebKitTestRunner is being used to test the WebsiteDataStore delegate.
The UIDelegate delegate is tested through API tests in StorageQuota.mm.
In both cases, we do not rely on the default quota value and set it to a lower value (400ko).

We can add another test in StorageQuota.mm which would use the default quota value.
This would require to write 1GB on disk to hit the limit and trigger the delegate.
This seems a bit expensive to me but can write the test. wdyt?
Comment 5 Geoffrey Garen 2019-05-22 15:12:02 PDT
> We can add another test in StorageQuota.mm which would use the default quota
> value.

Sounds good.

> This would require to write 1GB on disk to hit the limit and trigger the
> delegate.
> This seems a bit expensive to me but can write the test. wdyt?

Hmmm... As a regression test, it would be sufficient just to verify that 50MB + 1b did not trigger the delegate. Of course, 1GB is an even more valuable test. Here's a suggestion: Let's do a 1GB test (and make sure to delete afterwards!) if the test can run in less than 1s, and otherwise do a 50MB + 1b test. How does that sound?
Comment 6 youenn fablet 2019-05-22 16:23:20 PDT
Created attachment 370464 [details]
Patch
Comment 7 youenn fablet 2019-05-22 16:24:52 PDT
> Hmmm... As a regression test, it would be sufficient just to verify that
> 50MB + 1b did not trigger the delegate. Of course, 1GB is an even more
> valuable test. Here's a suggestion: Let's do a 1GB test (and make sure to
> delete afterwards!) if the test can run in less than 1s, and otherwise do a
> 50MB + 1b test. How does that sound?

I tried the 1GB and we are far from 1s, maybe we should try to optimize.
Anyway, I added a test that stores 100MB without hitting the delegate.
Comment 8 Build Bot 2019-05-22 16:25:20 PDT
Attachment 370464 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:189:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:212:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Geoffrey Garen 2019-05-23 10:26:04 PDT
Comment on attachment 370464 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2019-05-23 11:08:12 PDT
Comment on attachment 370464 [details]
Patch

Clearing flags on attachment: 370464

Committed r245698: <https://trac.webkit.org/changeset/245698>
Comment 11 WebKit Commit Bot 2019-05-23 11:08:14 PDT
All reviewed patches have been landed.  Closing bug.