[webkit-reviews] review denied: [Bug 197463] Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests : [Attachment 369591] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 197463: Reuse existing WebPageProxy quota handler for NetworkProcessProxy
quota requests
https://bugs.webkit.org/show_bug.cgi?id=197463

Attachment 369591: Patch

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




--- Comment #9 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 369591
  --> https://bugs.webkit.org/attachment.cgi?id=369591
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.