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

[bugzilla-daemon at webkit.org]

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

Attachment 392788: Patch

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




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

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

> Source/WebKit/ChangeLog:11
> +	   the UI process whether a domain is app-bound. 

You're not really reporting it to the UIProcess, but you are reading the file
from the UIProcess.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:71
> +static std::atomic<bool> initializedAppBoundDomains = false;

Boolean variables in WebKit need a prefix. So maybe
hasInitializedAppBoundDomains ?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:448
> +

I would drop this empty line.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:456
> +		   URL url { URL(), domain };

Do we really need to have URLs saved on disk? This means we have to parse the
URL unnecessarily just to get the host and then compute the registrable domain.
Why cannot we simply store registrable domains in the plist?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:458
> +		   if (!appBoundDomain.isEmpty()) {

To avoid nesting, maybe this:
if (appBoundDomain.isEmpty())
    continue;

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

Sorry, I know I recommended this approach but there is actually a subtle bug
now that I think about it more. The issue is that you're using the
WebsiteDataStore's WorkQueue. Each WebsiteDataStore has its own WorkQueue so:
1. WebsiteDataStore A could have queued the task to initialize on WorkQueue WQA
2. WebsiteDataStore B then tries to read app bound domains and
initializedAppBoundDomains is false, so its queue a task on WorkQueue WQB.
-> Because they are 2 separate work queues, there is no serialization guarantee
and you may not have initialized the domains yet by the time you get back to
the main thread for reading

One way to fix this would be to use a single static WorkQueue dedicated to
reading appBoundDomains.

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

Could we return a 'const HashSet<RegistrableDomain>&' here? It is kind of sad
right now because we go from:
HashSet<RegistrableDomain> -> Vector<RegistrableDomain> ->
Vector<RefPtr<API::Object>> -> API::Array

I like this better:
HashSet<RegistrableDomain> -> Vector<RefPtr<API::Object>> -> API::Array

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:121
> +    initializeAppBoundDomains();

Please do this in platformInitialize() instead.

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

We don't use get prefix for getters in WebKit. Seems like this method should be
const.