Bug 195283

Created attachment 363560 [details] Patch

Created attachment 363600 [details] Patch

Comment on attachment 363600 [details] Patch Attachment 363600 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11374110 New failing tests: fast/events/beforeunload-alert-user-interaction2.html fast/shadow-dom/click-text-inside-linked-slot.html fast/scrolling/ios/hit-testing-iframe-003.html fast/forms/datalist/datalist-textinput-suggestions-order.html fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html editing/selection/thai-word-at-document-end.html fast/events/click-handler-on-body-simple.html

Created attachment 363615 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 363615 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 Errors are probably unrelated

Comment on attachment 363600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363600&action=review > Source/WebKit/ChangeLog:13 > + Extra blank line. > Source/WebCore/storage/QuotaManager.cpp:61 > +void QuotaManager::requestSpaceIncreaseCompleted(Optional<uint64_t> quota) quota -> newQuota ? > Source/WebCore/storage/QuotaManager.cpp:66 > + auto pendingRequests = WTFMove(m_pendingRequests); Seems weird to take all pending requests here... > Source/WebCore/storage/QuotaManager.cpp:77 > + requestSpace(request.spaceIncrease, WTFMove(request.callback)); ... just to add them all back here. This also means that if the request.callback() call above keeps making quota requests, we'll never process following requests. I think this needs to be refactored a bit to be more reliable. We really want to process the requests in the order they came in and it looks like you are not guaranteeing this here. > Source/WebCore/storage/QuotaManager.h:38 > +class QuotaManager : public CanMakeWeakPtr<QuotaManager> { The name seem too generic given that it is not in a "Storage" namespace. I would suggest StorageQuotaManager. A quota could be for many things otherwise. Should be fast allocated. We should also likely prevent copying. > Source/WebCore/storage/QuotaManager.h:48 > + void addQuotaUser(QuotaUser& user) QuotaUser should probably be const. > Source/WebCore/storage/QuotaManager.h:54 > + void removeQuotaUser(QuotaUser& user) QuotaUser should probably be const. > Source/WebCore/storage/QuotaManager.h:71 > + HashSet<QuotaUser*> m_quotaUsers; QuotaUser should probably be const. Given the number of quota users we'll have, we can probably use a Vector instead of a HashSet. > Source/WebCore/storage/QuotaUser.h:30 > +class QuotaUser { I think this would be nicer as a "User" class inside the "StorageQuotaManager" class scope. I.e. StorageQuotaManager::User. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2307 > + return *m_quotaManagers.ensure(origin, [this, sessionID, &origin] { I think something needs to destroy quota managers when the session is destroyed, otherwise this is ever growing and really bad for private browsing where we have a private session per tab. > Source/WebKit/NetworkProcess/NetworkProcess.h:326 > + WebCore::QuotaManager& quotaManager(PAL::SessionID, const WebCore::ClientOrigin&); storageQuotaManager ? > Source/WebKit/NetworkProcess/NetworkProcess.h:530 > + HashMap<WebCore::ClientOrigin, std::unique_ptr<WebCore::QuotaManager>> m_quotaManagers; m_storageQuotaManager? > Source/WebKit/NetworkProcess/NetworkProcess.h:531 > + HashMap<PAL::SessionID, uint64_t> m_quotas; m_perSessionStorageQuotas ?

Created attachment 363704 [details] Patch

> This also means that if the request.callback() call above keeps making quota > requests, we'll never process following requests. I think this needs to be > refactored a bit to be more reliable. We really want to process the requests > in the order they came in and it looks like you are not guaranteeing this > here. In practice, this should not happen but yes, I refactored it to make sure we keep a reliable order. > > Source/WebCore/storage/QuotaManager.h:38 > > +class QuotaManager : public CanMakeWeakPtr<QuotaManager> { > > The name seem too generic given that it is not in a "Storage" namespace. I > would suggest StorageQuotaManager. A quota could be for many things > otherwise. Went with StorageQuotaManager/StorageQuotaUser > I think this would be nicer as a "User" class inside the > "StorageQuotaManager" class scope. I.e. StorageQuotaManager::User. I kept it separate, one small advantage is reduced include. Other points should be fixed in the above patch.

Created attachment 363705 [details] Patch

Created attachment 363706 [details] Patch

Created attachment 363767 [details] Patch

Comment on attachment 363767 [details] Patch r=me

Comment on attachment 363767 [details] Patch Clearing flags on attachment: 363767 Committed r242599: <https://trac.webkit.org/changeset/242599>

All reviewed patches have been landed. Closing bug.

<rdar://problem/48677961>

*** Bug 106507 has been marked as a duplicate of this bug. ***