[webkit-reviews] review denied: [Bug 20534] DumpRenderTree needs a way to override settings on a per-test basis : [Attachment 33495] Hopefully the final cut.

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 20534: DumpRenderTree needs a way to override settings on a per-test basis
https://bugs.webkit.org/show_bug.cgi?id=20534

Attachment 33495: Hopefully the final cut.
https://bugs.webkit.org/attachment.cgi?id=33495&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   * security/override-preferences-2-expected.txt:
> +	   * security/override-preferences-2.html: Verifies
overridePreverence("WebKitDefaultFontSize", "24")
> +	   * security/override-preferences-expected.txt:
> +	   * security/override-preferences.html: Verifies
overridePreference("WebKitJavaScriptEnabled", false).
> +	   * security/override-zzz-reset-expected.txt:
> +	   * security/override-zzz-reset.html: Because of 'zzz' this test will
run after the tests above and
> +	   verify that override of preferences does not 'spill' to the
subsequent tests in a batch.

The added tests are missing from this patch.

security/ seems like a strange place to put them. Maybe we need a new directory
for tests of the test harness?

> +- (void)_setPreference:(NSString *)key stringValue:(NSString *)value
> +{
> +    NSString *_key = KEY(key);
> +    [self _setStringValue:value forKey:_key];	
> +}

This is wrong. _setStringValue:forKey: also passes the key through KEY(), so
you'll end up with the preferences identifier prepended twice.

_setPreference:stringValue: is not a very Cocoa-y name. The obvious name would
be _setStringValue:forKey:. Maybe we should just expose that method through
WebPreferencesPrivate.h? Or maybe we can come up with a name that indicates
this is for testing purposes only. I don't think it's super-important to have
similar names on Mac and Windows for this.

> @@ -70,6 +70,10 @@ interface IWebPreferencesPrivate : IUnknown
>      HRESULT setFontSmoothingContrast([in] float contrast);
>      HRESULT fontSmoothingContrast([out, retval] float* contrast);
>  
> +    // This method is meant for overriding preferences for tests run
> +    // with DumpRenderTree only.
> +    HRESULT setPreference([in] BSTR key, [in] BSTR value);
> +
>      HRESULT isWebSecurityEnabled([out, retval] BOOL* enabled);
>      HRESULT setWebSecurityEnabled([in] BOOL enabled);

Please add the new function at the end of the interface, to avoid the risk of
breaking nightly builds.

> +HRESULT STDMETHODCALLTYPE WebPreferences::setPreference(BSTR key, BSTR
value)
> +{

There's no need to have STDMETHODCALLTYPE on the implementation. It's only
needed on the declaration.

> +    LayoutTestController* controller =
reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));

static_cast is preferable here.

> +void LayoutTestController::overridePreference(JSStringRef key, JSStringRef
flag)
> +{
> +    RetainPtr<CFStringRef> keyCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, key));
> +    NSString *keyNS = (NSString *)keyCF.get();
> +
> +    RetainPtr<CFStringRef> flagCF(AdoptCF,
JSStringCopyCFString(kCFAllocatorDefault, flag));
> +    NSString *flagNS = (NSString *)flagCF.get();

I think we tend to use 0 instead of kCFAllocatorDefault, though either is fine.


> +static void resetDefaultsToConsistentValues(IWebPreferences* preferences)
> +{
> +#ifdef USE_MAC_FONTS
> +    BSTR standardFamily = SysAllocString(TEXT("Times"));
> +    BSTR fixedFamily = SysAllocString(TEXT("Courier"));
> +    BSTR sansSerifFamily = SysAllocString(TEXT("Helvetica"));
> +    BSTR cursiveFamily = SysAllocString(TEXT("Apple Chancery"));
> +    BSTR fantasyFamily = SysAllocString(TEXT("Papyrus"));
> +#else
> +    BSTR standardFamily = SysAllocString(TEXT("Times New Roman"));
> +    BSTR fixedFamily = SysAllocString(TEXT("Courier New"));
> +    BSTR sansSerifFamily = SysAllocString(TEXT("Arial"));
> +    BSTR cursiveFamily = SysAllocString(TEXT("Comic Sans MS")); // Not
actually cursive, but it's what IE and Firefox use.
> +    BSTR fantasyFamily = SysAllocString(TEXT("Times New Roman"));
> +#endif
> +
> +    preferences->setStandardFontFamily(standardFamily);
> +    preferences->setFixedFontFamily(fixedFamily);
> +    preferences->setSerifFontFamily(standardFamily);
> +    preferences->setSansSerifFontFamily(sansSerifFamily);
> +    preferences->setCursiveFontFamily(cursiveFamily);
> +    preferences->setFantasyFontFamily(fantasyFamily);
> +
> +    SysFreeString(standardFamily);
> +    SysFreeString(fixedFamily);
> +    SysFreeString(sansSerifFamily);
> +    SysFreeString(cursiveFamily);
> +    SysFreeString(fantasyFamily);

You could make the font names be leaked static BSTRs.

> +	   preferences->resetDefaultsToConsistentValues();

This won't compile. resetDefaultsToConsistentValues() isn't a member of
IWebPreferences.