Bug 215027

<rdar://problem/63688232>

Created attachment 405727 [details] Patch

Created attachment 405745 [details] Patch

This was inspired by a comment from Darin on Bug 212426

Created attachment 405756 [details] Patch

Created attachment 408372 [details] Patch

Created attachment 408373 [details] Patch

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?

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

Committed r266829: <https://trac.webkit.org/changeset/266829> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408373 [details].