Bug 42331

Summary: WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: WebKit2Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, jonlee, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 82658, 96740    
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Maciej Stachowiak 2010-07-14 20:55:26 PDT
WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
Comment 1 Maciej Stachowiak 2010-07-14 20:59:26 PDT
<rdar://problem/8193641>
Comment 2 Jon Lee 2012-05-07 02:37:14 PDT
Created attachment 140498 [details]
Patch
Comment 3 Jon Lee 2012-05-07 02:41:43 PDT
Created attachment 140499 [details]
Patch

Missing trailing space for willPerformClientRedirectToURL
Comment 4 Darin Adler 2012-05-07 10:02:59 PDT
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?
Comment 5 Darin Adler 2012-05-07 10:03:35 PDT
(In reply to comment #4)
> r=me if you fix the storage leaks

storage leak singular I mean
Comment 6 Jon Lee 2012-05-07 11:24:18 PDT
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.
Comment 7 Jon Lee 2012-05-07 12:21:13 PDT
Committed r116338: <http://trac.webkit.org/changeset/116338>