[webkit-reviews] review granted: [Bug 176943] Storage Access API: UI process should update network process about granted access : [Attachment 327126] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 176943: Storage Access API: UI process should update network process about
granted access
https://bugs.webkit.org/show_bug.cgi?id=176943

Attachment 327126: Patch

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




--- Comment #13 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 327126
  --> https://bugs.webkit.org/attachment.cgi?id=327126
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:161
> +    HashMap<String, HashSet<String>> m_storageAccess;

I would give this a better name like m_domainsGrantedStorageAccess.  It took a
bit of reading to realize that the key is the top domain and the value is the
set of domains that have been granted storage access on that domain.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:301
> +	       auto accessEntry = m_storageAccess.add(firstPartyDomain,
HashSet<String>());
> +	       accessEntry.iterator->value.add(resourceDomain);

HashSet has a constructor that takes a std::initializer_list.  Using it could
make this one line.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:339
> +	  
parentProcessConnection()->send(Messages::NetworkProcessProxy::StorageAccessReq
uestResult(true, contextId), 0);
> +    } else
> +	  
parentProcessConnection()->send(Messages::NetworkProcessProxy::StorageAccessReq
uestResult(false, contextId), 0);

Should the value be true or shouldGrantStorage?
The else should have an ASSERT_NOT_REACHED in it.
We could make one call to send to reduce duplication.  Just make a local bool.

> Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:292
> +#if PLATFORM(COCOA)
> +   
processPool()->sendToNetworkingProcess(Messages::NetworkProcess::SetStorageAcce
ssAPIEnabled(enabled));

I'm not sure there's anything cocoa-specific about the storage access API
enabled bool.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:142
> +    WebResourceLoadStatisticsStore(const String&, Function<void(const
String&)>&& testingCallback,
UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&&,
UpdateStorageAccessForPrevalentDomainsHandler&&,
RemovePrevalentDomainsHandler&&);

There's a growing number of these callback functions.  Maybe a better design
would be to pass one client.  Probably not necessary for this patch, though.