[webkit-reviews] review granted: [Bug 215027] Clean up App-Bound Domains code to only compile for IOS with its own macro : [Attachment 408373] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted katherine_cheney at apple.com's request
for review:
Bug 215027: Clean up App-Bound Domains code to only compile for IOS with its
own macro
https://bugs.webkit.org/show_bug.cgi?id=215027

Attachment 408373: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 408373
  --> https://bugs.webkit.org/attachment.cgi?id=408373
Patch

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

Looks fine like this. Some opportunity to slightly improve, maybe could do
separately as a follow-up?

> Source/WTF/wtf/PlatformEnable.h:198
> +#if !defined(ENABLE_APP_BOUND_DOMAINS)
> +#define ENABLE_APP_BOUND_DOMAINS 0
> +#endif

This should not be needed. All enable flags are off by default and we should
not add zero defaults here (even though there are some existing ones).

> Source/WebCore/platform/network/NetworkStorageSession.h:218
> +#if ENABLE(APP_BOUND_DOMAINS)
>      WEBCORE_EXPORT void setAppBoundDomains(HashSet<RegistrableDomain>&&);
>      WEBCORE_EXPORT void resetAppBoundDomains();
>  #endif

I don’t think this should be nested inside the #if PLATFORM(COCOA) ||
USE(CFURLCONNECTION) section. Nested #if is hard to read and doesn’t provide
additional value here.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2525
> +#if ENABLE(APP_BOUND_DOMAINS)
>	   }, [this, sessionID](auto&& completionHandler) {
>	      
parentProcessConnection()->sendWithAsyncReply(Messages::NetworkProcessProxy::Ge
tAppBoundDomains { sessionID }, WTFMove(completionHandler), 0);
> +#else
> +	   }, [] (auto&& completionHandler) {
> +	       completionHandler({ });
> +#endif
>	   });

Seems like we could do this more elegantly with a local variable to avoid the
#if bracketing a strange subexpression.

> LayoutTests/platform/mac-wk2/TestExpectations:1293
> +# App Bound Domains is for iOS only
> +http/tests/resourceLoadStatistics/exemptDomains/ [ Skip ]

Normally we’d want to skip this in the main LayoutTests/TestExpectations and
then unskip in an iOS family TestExpectations. But maybe this is a more
expedient way to do this for now, or even more elegant. Seems particularly
strange to skip only in WK2, but I’m guessing maybe it’s un-skipped in a
WK2-specific file?