Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests
<rdar://problem/47403621>
Created attachment 368677 [details] Patch
Created attachment 369439 [details] Patch
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
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
Created attachment 369591 [details] Patch
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.
ping review
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.
(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.
Created attachment 369883 [details] Patch
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.
(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.
Created attachment 369897 [details] Patch
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.
Created attachment 369902 [details] Patch
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.
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
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
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
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
Created attachment 369960 [details] Patch for landing
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.
Comment on attachment 369960 [details] Patch for landing Clearing flags on attachment: 369960 Committed r245328: <https://trac.webkit.org/changeset/245328>
All reviewed patches have been landed. Closing bug.