[webkit-reviews] review denied: [Bug 227326] Clean up App Privacy Report code : [Attachment 432197] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Kate Cheney
<katherine_cheney at apple.com>'s request for review:
Bug 227326: Clean up App Privacy Report code
https://bugs.webkit.org/show_bug.cgi?id=227326

Attachment 432197: Patch

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




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 432197
  --> https://bugs.webkit.org/attachment.cgi?id=432197
Patch

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

I think this looks good, but I'm concerned there are some places where we have
the wrong default set, and I'm not sure if the places where 'AppBound' was used
to set 'AppInitiated' is right. r- for those reasons.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:138
> +    return m_document->loader() ?
m_document->loader()->lastNavigationWasAppInitiated() : false;

Shouldn't this default to 'true'?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:572
> +	   auto contextData = ServiceWorkerContextData { std::nullopt,
WTFMove(registration), workerIdentifier, WTFMove(script),
WTFMove(*certificateInfo), WTFMove(*contentSecurityPolicy),
WTFMove(referrerPolicy), WTFMove(scriptURL), *workerType, true,
LastNavigationWasAppInitiated::No, WTFMove(scriptResourceMap) };

Should this be LastNavigationWasAppInitiated::Yes, unless we have data
indicating it was user initiated?

> Source/WebCore/workers/service/server/SWServer.cpp:658
> +	   return LastNavigationWasAppInitiated::No;

Wouldn't the assumption be that it's AppInitiated unless we have data saying it
was user initiated? Seems like the default return should be Yes?

> Source/WebCore/workers/service/server/SWServer.cpp:665
> +	       return LastNavigationWasAppInitiated::Yes;

Seems like we might want to early return on 'No', since that would be the
unusual case.

> Source/WebCore/workers/service/server/SWServer.cpp:668
> +    return LastNavigationWasAppInitiated::No;

And if we early return on No, we would want to return Yes here.

> Source/WebCore/workers/service/server/SWServer.cpp:902
> +    if (data.lastNavigationWasAppInitiated ==
LastNavigationWasAppInitiated::Yes) {

It seems like we'd almost always be AppInitiated, and would want to relay the
User-Initiated nature here, wouldn't we?

> Source/WebCore/workers/service/server/SWServerRegistration.h:138
> +    bool m_isAppInitiated { false };

Shouldn't this default to 'true', like the regular ResourceRequest case?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:258
> +	   registration->scheduleSoftUpdate(m_loader.isAppBound() ?
WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);

Is this conditional right? Why does an App-Bound loader indicate it was app
initiated? Should loader have an app Initiated predicate?

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:191
> +	       registration->scheduleSoftUpdate(loader.isAppBound() ?
WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);

Ditto.

> Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:372
> +	   retrieveSubresourcesEntry(resourceKey, [this, weakThis =
makeWeakPtr(*this), frameID, pendingFrameLoad = WTFMove(pendingFrameLoad),
requestIsAppBound = request.isAppInitiated(),
isNavigatingToAppBoundDomain](std::unique_ptr<SubresourcesEntry> entry) {

Shouldn't 'requestIsAppBound' be renamed, too?

> LayoutTests/platform/ios-wk2/TestExpectations:-1827
> -http/tests/in-app-browser-privacy/app-bound-attribution-post-request.html [
Skip ]

yay!