Bug 197463 – Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests

[youenn fablet]

<rdar://problem/47403621>

[youenn fablet]

Created attachment 368677 [details]
Patch

[youenn fablet]

Created attachment 369439 [details]
Patch

[EWS Watchlist]

Comment on attachment 369439 [details]
Patch

Attachment 369439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12137935

New failing tests:
http/wpt/cache-storage/quota-third-party.https.html
http/tests/IndexedDB/storage-limit.https.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
inspector/unit-tests/objectStore/clear.html
inspector/unit-tests/objectStore/put.html
inspector/unit-tests/objectStore/putObject.html
inspector/unit-tests/objectStore/delete.html
http/wpt/cache-storage/cache-quota.any.html
http/wpt/cache-storage/cache-quota-after-restart.any.html
imported/w3c/IndexedDB-private-browsing/idbcursor_iterating.html
inspector/unit-tests/objectStore/basic.html
http/tests/IndexedDB/storage-limit-1.https.html
http/tests/IndexedDB/storage-limit-2.https.html
imported/w3c/IndexedDB-private-browsing/idbindex-multientry-big.html
http/wpt/cache-storage/cache-quota-add.any.html
inspector/unit-tests/objectStore/deleteObject.html

[EWS Watchlist]

Created attachment 369449 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 369591 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  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:179:  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 11 files


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

[youenn fablet]

ping review

[Alex Christensen]

Comment on attachment 369591 [details]
Patch

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

Seems ok, but needs some refinement.

> Source/WebKit/UIProcess/WebPageProxy.cpp:365
> +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)

This seems kind of hacky.  It assumes that there is one dominant WKWebView, which I don't think is a good assumption in some apps.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4385
> +    m_isQuotaIncreaseDenied = false;

Is this called during fragment navigations?  If it is, we might want to put this elsewhere, or keep track of this some other way.  Otherwise, a website could just do a quick hash navigation then ask again.  Please add a test that tries this.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const WTF::Function<void(WebPageProxy&)>& callback)

forEachWebPage
WTF:: unnecessary here and in header.  Same with WTF::CompletionHandler in WebPageProxy.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreClient.h:41
> +    virtual bool implementsRequestStorageSpaceHandler() const { return false; }
>      virtual void requestStorageSpace(const WebCore::SecurityOriginData& topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota, uint64_t currentSize, uint64_t spaceRequired, CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)

I think we prefer to just pass in a CompletionHandler& and if it's null afterwards then it was used otherwise do something else with it.  That keeps our client interfaces a bit cleaner than adding a separate function to do the check.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2019

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:215
> +#if PLATFORM(MAC)
> +    [webView1.get().window setIsVisible:YES];
> +#else
> +    webView1.get().window.hidden = NO;
> +#endif

This is used 3 times here.  Maybe a good utility function.

[youenn fablet]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 369591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369591&action=review
> 
> Seems ok, but needs some refinement.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:365
> > +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)
> 
> This seems kind of hacky.  It assumes that there is one dominant WKWebView,
> which I don't think is a good assumption in some apps.

It assumes that in most cases, there is only one web view with a given origin that is visible at a time.
This heuristic can be improved over time.
If we find that the heuristic is broken in too many places, we will have to rely on the current WebsiteDataStore SPI.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4385
> > +    m_isQuotaIncreaseDenied = false;
> 
> Is this called during fragment navigations?  If it is, we might want to put
> this elsewhere, or keep track of this some other way.  Otherwise, a website
> could just do a quick hash navigation then ask again.  Please add a test
> that tries this.

A fragment navigation is never replacing the context so this cannot happen.

> 
> > Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> > +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const WTF::Function<void(WebPageProxy&)>& callback)
> 
> forEachWebPage
> WTF:: unnecessary here and in header.  Same with WTF::CompletionHandler in
> WebPageProxy.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreClient.h:41
> > +    virtual bool implementsRequestStorageSpaceHandler() const { return false; }
> >      virtual void requestStorageSpace(const WebCore::SecurityOriginData& topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota, uint64_t currentSize, uint64_t spaceRequired, CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)
> 
> I think we prefer to just pass in a CompletionHandler& and if it's null
> afterwards then it was used otherwise do something else with it.  That keeps
> our client interfaces a bit cleaner than adding a separate function to do
> the check.

I have not seen this pattern and I do not really like it.
Also, this additional API is not visible to any application developer so this is purely internal.

I'll change the behavior so that calling completionHandler(WTF::nullOpt) means that there is no decision taken and a default decision should be made by WebKit.

[youenn fablet]

Created attachment 369883 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  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:179:  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 10 files


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

[youenn fablet]

(In reply to youenn fablet from comment #10)
> (In reply to Alex Christensen from comment #9)
> > Comment on attachment 369591 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=369591&action=review
> > 
> > Seems ok, but needs some refinement.
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:365
> > > +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)
> > 
> > This seems kind of hacky.  It assumes that there is one dominant WKWebView,
> > which I don't think is a good assumption in some apps.
> 
> It assumes that in most cases, there is only one web view with a given
> origin that is visible at a time.
> This heuristic can be improved over time.
> If we find that the heuristic is broken in too many places, we will have to
> rely on the current WebsiteDataStore SPI.

Discussed this with Alex and cleared up the fact that the selected page has the same origin as the request.

[youenn fablet]

Created attachment 369897 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  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:183:  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 10 files


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

[youenn fablet]

Created attachment 369902 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  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:183:  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 10 files


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

[EWS Watchlist]

Comment on attachment 369902 [details]
Patch

Attachment 369902 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12193124

New failing tests:
fast/shadow-dom/svg-thref-href-change-in-shadow-tree.html

[EWS Watchlist]

Created attachment 369916 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Alex Christensen]

Comment on attachment 369902 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:297
> +    bool m_areBeingProcessing { false };

m_isProcessing or m_requestsAreBeingProcessed

> Source/WebKit/UIProcess/WebPageProxy.cpp:7262
> +            StorageRequests::singleton().processNextIfAny();

I think it would be a better abstraction if the StorageRequests made its own lambda that called this.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const Function<void(WebPageProxy&)>& callback)

forWebPagesWithOrigin

[youenn fablet]

Thanks for the review.

(In reply to Alex Christensen from comment #20)
> Comment on attachment 369902 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369902&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:297
> > +    bool m_areBeingProcessing { false };
> 
> m_isProcessing or m_requestsAreBeingProcessed

OK

> > Source/WebKit/UIProcess/WebPageProxy.cpp:7262
> > +            StorageRequests::singleton().processNextIfAny();
> 
> I think it would be a better abstraction if the StorageRequests made its own
> lambda that called this.

Agreed, I thought about this.
My initial idea was to pass to the request completion handler another completion handler for the purpose of calling StorageRequests::singleton().processNextIfAny().
I concluded this was less readable since we end up with two completion handlers.

> > Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> > +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const Function<void(WebPageProxy&)>& callback)
> 
> forWebPagesWithOrigin

OK

[youenn fablet]

Created attachment 369960 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  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:183:  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 10 files


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

[WebKit Commit Bot]

Comment on attachment 369960 [details]
Patch for landing

Clearing flags on attachment: 369960

Committed r245328: <https://trac.webkit.org/changeset/245328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.