[webkit-reviews] review denied: [Bug 45393] WebKitTestRunner should be portable : [Attachment 68229] proposed patch - updated

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Balazs Kelemen <kbalazs at webkit.org>'s
request for review:
Bug 45393: WebKitTestRunner should be portable
https://bugs.webkit.org/show_bug.cgi?id=45393

Attachment 68229: proposed patch - updated
https://bugs.webkit.org/attachment.cgi?id=68229&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68229&action=review

> WebKitTools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:70
> -    JSRetainPtr<JSStringRef> jsStringNodeName(Adopt,
JSValueToStringCopy(context, nodeNameValue, 0));
> +    JSStringRef jsStringNodeName = JSValueToStringCopy(context,
nodeNameValue, 0);

This is not correct. It creates a storage leak.

> WebKitTools/WebKitTestRunner/PlatformString.h:27
> +#if PLATFORM(MAC) || PLATFORM(WIN)
> +#include <wtf/RetainPtr.h>
> +typedef CFStringRef NativeString;
> +typedef RetainPtr<CFStringRef> NativeStringRef;
> +#elif PLATFORM(QT)
> +#include <QString>
> +typedef QString NativeString;
> +typedef QString NativeStringRef;
> +#endif

Normally the word “Native” is not good in cases like this. If you work on
WebKit, then you think that WTF::String is native. If you work on Mac then you
think that CFStringRef is native. There’s no unambiguous meaning of the term.
We tried to use this term in JavaScriptCore and half of the people thought it
meant one thing and the other half thought it meant the other.

> WebKitTools/WebKitTestRunner/PlatformString.h:45
> +    friend WTR_ALWAYS_INLINE bool operator==(const PlatformString& a, const
PlatformString& b);
> +    friend WTR_ALWAYS_INLINE bool operator==(const PlatformString& a, const
char* b);

No need for "a" and "b" argument names here.

> WebKitTools/WebKitTestRunner/PlatformString.h:55
> +    NativeStringRef m_impl;

I am not a fan of the name “impl”. We should find another name to use here.=

> WebKitTools/WebKitTestRunner/StringFunctions.h:43
> +    return PlatformString(string).toWK();

It seems wrong that we have to go through a platform-specific string to go from
a JSStringRef to a WKStringRef. A better fix would be to make it work directly
without including the platform-specific string class at all.

> WebKitTools/WebKitTestRunner/TestController.cpp:213
> +    PlatformURL url("about:blank");

I think we should have a function that returns the about:blank URL, as we do in
WebCore.

> WebKitTools/WebKitTestRunner/TestInvocation.cpp:42
> +    : m_url(AdoptWK, PlatformURL(pathOrURL).toWK())

Makes no sense to me that we have to go through PlatformURL to get to WKURLRef
from a const char*.