Bug 215027 – Clean up App-Bound Domains code to only compile for IOS with its own macro

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

This should replace the IOS, COCOA, or IOS_FAMILY macros to be more consistent

[Kate Cheney]

<rdar://problem/63688232>

[Kate Cheney]

Created attachment 405727 [details]
Patch

[Kate Cheney]

Created attachment 405745 [details]
Patch

[Kate Cheney]

This was inspired by a comment from Darin on Bug 212426

[Kate Cheney]

Created attachment 405756 [details]
Patch

[Kate Cheney]

Created attachment 408372 [details]
Patch

[Kate Cheney]

Created attachment 408373 [details]
Patch

[Darin Adler]

Comment on attachment 408373 [details]
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::GetAppBoundDomains { 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?

[Kate Cheney]

(In reply to Darin Adler from comment #8)
> Comment on attachment 408373 [details]
> 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?
> 

Thanks for the review! Good call, tracking the followup here: rdar://68645704.

> > 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).
> 

Will fix in the followup.

> > 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.
> 

Ditto.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2525
> > +#if ENABLE(APP_BOUND_DOMAINS)
> >          }, [this, sessionID](auto&& completionHandler) {
> >              parentProcessConnection()->sendWithAsyncReply(Messages::NetworkProcessProxy::GetAppBoundDomains { 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.
> 

Ditto.

> > 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?

Yes, all resourceLoadStatistics tests are set to 'pass' in the main TestExpectations, so I skipped the app-bound domains tests specifically here. In the followup I'll change the exemptDomains directory to be skipped in the main TestExpectations and un-skipped in the iOS-family expectations.

[EWS]

Committed r266829: <https://trac.webkit.org/changeset/266829>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408373 [details].