[webkit-reviews] review denied: [Bug 208528] UIProcess needs mechanism to specify AppBound domains : [Attachment 392753] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied  review:
Bug 208528: UIProcess needs mechanism to specify AppBound domains
https://bugs.webkit.org/show_bug.cgi?id=208528

Attachment 392753: Patch

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




--- Comment #13 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 392753
  --> https://bugs.webkit.org/attachment.cgi?id=392753
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:605
> +    auto completionHandlerCopy = makeBlockPtr(completionHandler);

This can be done in the lambda capture.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:607
> +	   Vector<RefPtr<API::Object>> apiDomains;

Seems like a good use case for WTF::map()

> Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp:51
> +    ASSERT(isMainThread());

RunLoop::isMain() is preferred in WebKit2 code, especially in the UIProcess
layer where an app could be using both WK1 and WK2 (and thus have a WebThread).
isMainThread() is never correct in the UIProcess for this reason.

> Source/WebKit/UIProcess/WebFramePolicyListenerProxy.h:44
> +enum class ShouldExpectAppBoundDomainResult { No, Yes };

enum class ShouldExpectAppBoundDomainResult : bool { No, Yes };

> Source/WebKit/UIProcess/WebPageProxy.cpp:5060
> +	   setIsNavigatingToAppBoundDomain(frame->isMainFrame(),
navigation->currentRequest().url(), isAppBoundDomain);

What if the policyAction is Ignore? Then we won't navigate. Is it still correct
to go set this IsNavigatingToAppBoundDomain flag?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:431
> +    ASSERT(isMainThread());

RunLoop::isMain()

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:436
> +void WebsiteDataStore::beginAppBoundDomainCheck(const
WebCore::RegistrableDomain domain, WebFramePolicyListenerProxy& listener)

WebCore::RegistrableDomain&&

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:438
> +    ASSERT(RunLoop::isMain());

RunLoop::isMain()

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:442
> +    if (initializedAppBoundDomains) {

This does not look quite right. If beginAppBoundDomainCheck() gets called twice
before initializedAppBoundDomains is set to true, then you'll dispatch to the
background thread twice and read the file on disk twice.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:447
> +    m_queue->dispatch([domain = domain.isolatedCopy(), listener =
makeRef(listener)] () mutable {

domain = WTFMove(domain) once you switch to an rvalue reference.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:448
> +	   NSArray *domains = [[NSBundle mainBundle]
objectForInfoDictionaryKey:@"AppBoundDomains"];

Would 'NSArray<NSString *> *domains' build?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:449
> +	   RunLoop::main().dispatch([domain = domain.isolatedCopy(), listener =
WTFMove(listener), domains = retainPtr(domains)] {

domain = WTFMove(domain) once you switch to an rvalue reference.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:452
> +	       for (unsigned i = 0; i < appBoundDomainsCount && domainsAdded <
maxAppBoundDomainCount; ++i) {

for (NSString *domain in domains)

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:457
> +		       domainsAdded++;

if (appBoundDomains() >= maxAppBoundDomainCount)
    break;

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:467
> +void
WebsiteDataStore::getAppBoundDomainsForTesting(CompletionHandler<void(Vector<We
bCore::RegistrableDomain>)>&& completionHandler)

Vector<WebCore::RegistrableDomain>&&

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:469
> +    completionHandler(copyToVector(appBoundDomains()));

Nothing guarantees here that appBoundDomains() has been initialized.

My personal opinion is that we should start initializing the app bound domains
when the first WebsiteDataStore object is constructed to avoid slowing down the
very first page load.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:286
> +    void beginAppBoundDomainCheck(const WebCore::RegistrableDomain,
WebFramePolicyListenerProxy&);

WebCore::RegistrableDomain&&

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:287
> +    void
getAppBoundDomainsForTesting(CompletionHandler<void(Vector<WebCore::Registrable
Domain>)>&&);

Vector<WebCore::RegistrableDomain>&&