Bug 210344

<rdar://problem/61583925>

Created attachment 396101 [details]
Patch

Comment on attachment 396101 [details]
Patch

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

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:127
> +#if PLATFORM(IOS_FAMILY)

Why is this not needed on Mac?

(In reply to Alex Christensen from comment #3)
> Comment on attachment 396101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396101&action=review
> 
> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:127
> > +#if PLATFORM(IOS_FAMILY)
> 
> Why is this not needed on Mac?

This is an iOS feature only, so this option should only be set if the tests are running iOS (they are skipped on other platforms).

Comment on attachment 396101 [details]
Patch

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

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:131
> +        [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"WebKitDebugIsInAppBrowserPrivacyEnabled"];

Two thoughts:
1. TestController::platformAddTestOptions is a place where we are supposed to be writing to options, and this reads from options.  I think TestController::platformResetPreferencesToConsistentValues or TestController::platformResetStateToConsistentValues  and maybe TestController::updatePlatformSpecificTestOptionsForTest would be a better place to reset and set these values in NSUserDefaults.
2. Instead of setting the bool to NO, let's remove object for key.

(In reply to Alex Christensen from comment #5)
> Comment on attachment 396101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396101&action=review
> 
> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:131
> > +        [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"WebKitDebugIsInAppBrowserPrivacyEnabled"];
> 
> Two thoughts:
> 1. TestController::platformAddTestOptions is a place where we are supposed
> to be writing to options, and this reads from options.  I think
> TestController::platformResetPreferencesToConsistentValues or
> TestController::platformResetStateToConsistentValues  and maybe
> TestController::updatePlatformSpecificTestOptionsForTest would be a better
> place to reset and set these values in NSUserDefaults.
> 2. Instead of setting the bool to NO, let's remove object for key.

I don't think this will work because updatePlatformSpecificTestOptionsForTest() is called before updateTestOptionsFromTestHeader() in TestController::testOptionsForTest(), so it doesn't have the updated value. Then I think platformResetStateToConsistentValues is called at the end of the test, so I can remove the key there but not set it prior to the test.

Created attachment 396313 [details]
Patch

(In reply to katherine_cheney from comment #6)
> (In reply to Alex Christensen from comment #5)
> > Comment on attachment 396101 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396101&action=review
> > 
> > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:131
> > > +        [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"WebKitDebugIsInAppBrowserPrivacyEnabled"];
> > 
> > Two thoughts:
> > 1. TestController::platformAddTestOptions is a place where we are supposed
> > to be writing to options, and this reads from options.  I think
> > TestController::platformResetPreferencesToConsistentValues or
> > TestController::platformResetStateToConsistentValues  and maybe
> > TestController::updatePlatformSpecificTestOptionsForTest would be a better
> > place to reset and set these values in NSUserDefaults.
> > 2. Instead of setting the bool to NO, let's remove object for key.
> 
> I don't think this will work because
> updatePlatformSpecificTestOptionsForTest() is called before
> updateTestOptionsFromTestHeader() in TestController::testOptionsForTest(),
> so it doesn't have the updated value. Then I think
> platformResetStateToConsistentValues is called at the end of the test, so I
> can remove the key there but not set it prior to the test.

Maybe I'm missing something? Because it seems like all internal/experimental features are set before the header comments are read, and those seem to work fine. But this test still fails if I call it before updateTestOptionsFromTestHeader().

r=me

Committed r260040: <https://trac.webkit.org/changeset/260040>

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