[webkit-reviews] review canceled: [Bug 187055] Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model : [Attachment 343632] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has canceled Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 187055: Split memory store logic out of WebResourceLoadStatisticsStore to
clarify threading model
https://bugs.webkit.org/show_bug.cgi?id=187055

Attachment 343632: Patch

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




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

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

As discussed offline, I will refactor hasStorageAccess() to be more consistent
with the rest and not use an std::optional.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:210
>> +		    completionHandler(*hasStorageAccess);
> 
> This code is confusing (see naming comment above) and wrong as far as I can
tell.
> 
> First, it looks like it's asking the memory store whether the
subFramePrimaryDomain has been granted storage access but that cannot happen.
Instead the memory store just knows if a) the domain is unable to request
access or b) doesn't need to request access. I suggest
mayRequestStorageAccess().
> 
> Second, out of the three current return values, both 'true' and 'false'
should return early and skip the call to the network process. Only the
std::nullopt case, meaning the domain *may have requested* storage access,
should trigger a call to the network process. The network process will of
course respond with the right value, so no test will fail. It's just
unnecessary.

hasStorageAccess is actually a std::optional