Bug 42331 – WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks

• Component: WebKit2
• Status: RESOLVED FIXED

[Maciej Stachowiak]

WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks

[Maciej Stachowiak]

<rdar://problem/8193641>

[Jon Lee]

Created attachment 140498 [details]
Patch

[Jon Lee]

Created attachment 140499 [details]
Patch

Missing trailing space for willPerformClientRedirectToURL

[Darin Adler]

Comment on attachment 140499 [details]
Patch

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

r=me if you fix the storage leaks

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:197
> +bool booleanForKey(WKDictionaryRef dictionary, const char* key)
> +{
> +    WKRetainPtr<WKStringRef> wkKey(AdoptWK, WKStringCreateWithUTF8CString(key));
> +    return WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(dictionary, wkKey.get())));
> +}

This function does a bad cast if the value is not a boolean. And it calls WKBooleanGetValue on a null pointer of there is no item in the dictionary with that key. Given that this is part of a testing framework, I was thinking it might be even better to check the type and do something different in those cases, like report an error somehow.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:109
> +    void beginTesting(WKDictionaryRef);

This argument needs a name. It’s not obvious why beginTesting would take a dictionary argument.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:350
> +// Similar to -[WebFrame _drt_descriptionSuitableForTestResult]

Not sure that “similar” is a helpful comment. Maybe there’s something stronger you want to say here?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:780
> +    InjectedBundle::shared().stringBuilder()->append(toWTFString(WKURLCopyString(url)));

There’s a storage leak here. You can fix it by writing adoptWK(WKURLCopyString(url)).get() instead of WKURLCopyString(url).

> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:75
> +    void dumpFrameLoadCallbacks(bool value = true) { m_dumpFrameLoadCallbacks = value; }

These functions have really bad names, and it’s even worse for a version that takes a boolean. setShouldDumpFrameLoadCallbacks should be a far better name.

> Tools/WebKitTestRunner/TestInvocation.cpp:132
> +    return strstr(pathOrURL, "loading/");

Is this really the right rule? Who came up with this? We are seriously hardcoding this into all our test runner tools?

[Darin Adler]

(In reply to comment #4)
> r=me if you fix the storage leaks

storage leak singular I mean

[Jon Lee]

Comment on attachment 140499 [details]
Patch

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

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:197
>> +}
> 
> This function does a bad cast if the value is not a boolean. And it calls WKBooleanGetValue on a null pointer of there is no item in the dictionary with that key. Given that this is part of a testing framework, I was thinking it might be even better to check the type and do something different in those cases, like report an error somehow.

Sure, I can make this function behave a lot better. I will have it output an error.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:109
>> +    void beginTesting(WKDictionaryRef);
> 
> This argument needs a name. It’s not obvious why beginTesting would take a dictionary argument.

Done.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:780
>> +    InjectedBundle::shared().stringBuilder()->append(toWTFString(WKURLCopyString(url)));
> 
> There’s a storage leak here. You can fix it by writing adoptWK(WKURLCopyString(url)).get() instead of WKURLCopyString(url).

Done.

>> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:75
>> +    void dumpFrameLoadCallbacks(bool value = true) { m_dumpFrameLoadCallbacks = value; }
> 
> These functions have really bad names, and it’s even worse for a version that takes a boolean. setShouldDumpFrameLoadCallbacks should be a far better name.

I wanted to avoid having two of these, since dumpFrameLoadCallbacks is part of the layoutTestController idl, but it's easy to have the setter function for WTR usage, and leave this function in for JS.

>> Tools/WebKitTestRunner/TestInvocation.cpp:132
>> +    return strstr(pathOrURL, "loading/");
> 
> Is this really the right rule? Who came up with this? We are seriously hardcoding this into all our test runner tools?

There's a whole series of these in DRT.

[Jon Lee]

Committed r116338: <http://trac.webkit.org/changeset/116338>