[webkit-reviews] review granted: [Bug 42331] WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks : [Attachment 140499] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 42331: WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
https://bugs.webkit.org/show_bug.cgi?id=42331

Attachment 140499: Patch
https://bugs.webkit.org/attachment.cgi?id=140499&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(dictionar
y, 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(ur
l)));

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?