[webkit-reviews] review denied: [Bug 209016] [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests failing. : [Attachment 393519] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied katherine_cheney at apple.com's request
for review:
Bug 209016: [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests
failing.
https://bugs.webkit.org/show_bug.cgi?id=209016

Attachment 393519: Patch

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




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

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

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:527
> +    hasInitializedAppBoundDomains = false;

I think that for this code to actually be correct, you'd have to reset
hasInitializedAppBoundDomains to false in the lambda inside
clearAppBoundDomains(), after clearing the domains. That said, I think we can
simplify this code a lot like by tweaking existing code like so:

enum class ForceReinitialization : bool { No, Yes };
void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization
forceReinitialization)
{
    ASSERT(RunLoop::isMain());

    if (hasInitializedAppBoundDomains && forceReinitialization !=
ForceReinitialization::Yes) // Updated
	return;

    static const auto maxAppBoundDomainCount = 10;

    appBoundDomainQueue().dispatch([forceReinitialization] () mutable {
	if (hasInitializedAppBoundDomains && forceReinitialization !=
ForceReinitialization::Yes) // Updated
	    return;

	NSArray<NSString *> *domains = [[NSBundle mainBundle]
objectForInfoDictionaryKey:@"WKAppBoundDomains"];

	RunLoop::main().dispatch([forceReinitialization , domains =
retainPtr(domains)] {
	    if ( forceReinitialization == ForceReinitialization::Yes) // New
		appBoundDomains().clear(); // New

	    for (NSString *domain in domains.get()) {
		URL url { URL(), domain };
		if (!url.isValid())
		    continue;
		WebCore::RegistrableDomain appBoundDomain { url };
		if (appBoundDomain.isEmpty())
		    continue;
		appBoundDomains().add(appBoundDomain);
		if (appBoundDomains().size() >= maxAppBoundDomainCount)
		    break;
	    }
	    WEBSITE_DATA_STORE_ADDITIONS
	    hasInitializedAppBoundDomains = true;
	});
    });
}

Then you only have to call
initializeAppBoundDomains(ForceReinitialization::Yes) in your new
reinitializeAppBoundDomains() method.

What do you think?