Bug 45393 – WebKitTestRunner should be portable

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

Currently WTR uses CF types (CFString and CFURL) what makes impossible to porting it. I believe that a common implementation would be much better then maintaining each platform's own WTR (like Qt's own DRT what makes a lot of trouble to keep feature equivalent with the common one).

[Balazs Kelemen]

Created attachment 66900 [details]
proposed patch

[WebKit Review Bot]

Attachment 66900 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/WebKitTestRunner/StringFunctions.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformURLInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/PlatformURL.h:5:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/PlatformURL.h:21:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKitTools/WebKitTestRunner/PlatformString.h:8:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKitTools/WebKitTestRunner/PlatformString.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformStringInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 15 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 66903 [details]
proposed patch

Fix the true positive check-webkit-style errors.

[WebKit Review Bot]

Attachment 66903 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/WebKitTestRunner/StringFunctions.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformURLInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/PlatformString.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformStringInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 67008 [details]
proposed patch

This one removes all CFString and CFURL usage from WTR from the common (not port specific) files.

[WebKit Review Bot]

Attachment 67008 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/WebKitTestRunner/qt/PlatformURLInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformStringInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/PlatformString.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/StringFunctions.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

I have realized that the InjectedBundle depends on the cf run loop. I guess it would not be so hard to rework that in a platform independent manner but I would like to get feedback before starting to work on that.

[Sam Weinig]

(In reply to comment #7)
> I have realized that the InjectedBundle depends on the cf run loop. I guess it would not be so hard to rework that in a platform independent manner but I would like to get feedback before starting to work on that.

This looks good to me, thanks for doing this.  I don't think all the inlining is necessary though. I also don't know what you mean about the CFRunLoop. Can you elaborate on that or point to the code you are talking about?

[Balazs Kelemen]

> 
> This looks good to me, thanks for doing this.  I don't think all the inlining is necessary though. I also don't know what you mean about the CFRunLoop. Can you elaborate on that or point to the code you are talking about?

I see other CF uses mostly in InjectedBundle/LayoutTestController.cpp, for example: http://trac.webkit.org/browser/trunk/WebKitTools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp#L116

Inlining could be avoided by using the same files for WTR and InjectedBundle. Do you thing it would be ok? Moreover we could introducing a new static lib target for those files or duplicate them. Which solution do you vote for?

[Balazs Kelemen]

Let me stating the obvious: the root of this problems is that WRT is relying on the public C API. Is it a possibility to using the internals directly? This way we would not need wrappers and the whole thing would be easier as I see.

[Sam Weinig]

(In reply to comment #10)
> Let me stating the obvious: the root of this problems is that WRT is relying on the public C API. Is it a possibility to using the internals directly? This way we would not need wrappers and the whole thing would be easier as I see.

No. I would like to keep it using API.

[Balazs Kelemen]

(In reply to comment #11)
> (In reply to comment #10)
> > Let me stating the obvious: the root of this problems is that WRT is relying on the public C API. Is it a possibility to using the internals directly? This way we would not need wrappers and the whole thing would be easier as I see.
> 
> No. I would like to keep it using API.

Ok. Do you thing the patch is ready for review? I can extend it with a solution for the CFRunLoop staff if you would like.

[Balazs Kelemen]

Created attachment 68229 [details]
proposed patch - updated

Updated patch. Please review it if you really like to go into this direction, or inform me about that you don't like this.

[WebKit Review Bot]

Attachment 68229 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/WebKitTestRunner/qt/PlatformURLInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/qt/PlatformStringInlinesQt.h:3:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/PlatformString.h:6:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/WebKitTestRunner/StringFunctions.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Is there somebody who could review my poor patch?

[Darin Adler]

Comment on attachment 68229 [details]
proposed patch - updated

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*.

[Balazs Kelemen]

> 
> > 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.
> 

There is no public API to do this. The API does not provide a function for getting a char* or UChar* from a WKStringRef. I will try to limit this kind of conversations to StringFunctions.h in the next patch that will hopefully solve all the other problems you mentioned.

[Adam Roben (:aroben)]

(In reply to comment #17)
> > 
> > > 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.
> > 
> 
> There is no public API to do this. The API does not provide a function for getting a char* or UChar* from a WKStringRef. I will try to limit this kind of conversations to StringFunctions.h in the next patch that will hopefully solve all the other problems you mentioned.

The API is under our control. We can (and should) add the API functions we need if we think it's likely that other clients will need them, too.

[Balazs Kelemen]

> The API is under our control. We can (and should) add the API functions we need if we think it's likely that other clients will need them, too.

I hardly think that this kind of API would be useful for other clients.

[Darin Adler]

(In reply to comment #19)
> > The API is under our control. We can (and should) add the API functions we need if we think it's likely that other clients will need them, too.
> 
> I hardly think that this kind of API would be useful for other clients.

I disagree.

I think that clients might may to work directly with WKString objects without being forced to allocate a new platform string each time, especially in code that could be performance critical.

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> (In reply to comment #19)
> > > The API is under our control. We can (and should) add the API functions we need if we think it's likely that other clients will need them, too.
> > 
> > I hardly think that this kind of API would be useful for other clients.
> 
> I disagree.
> 
> I think that clients might may to work directly with WKString objects without being forced to allocate a new platform string each time, especially in code that could be performance critical.

I have to agree with Darin here. We should add the API we need, and not try to work around it.

[Balazs Kelemen]

Created attachment 69203 [details]
proposed patch

[WebKit Review Bot]

Attachment 69203 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/WebKitTestRunner/TestInvocation.cpp:118:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit2/Shared/API/c/WKString.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 69217 [details]
proposed patch

Fixed style and debug build issues.

[Darin Adler]

Comment on attachment 69217 [details]
proposed patch

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

Seems like a reasonable basic approach, but there are some problems.

> WebKitTools/WebKitTestRunner/StringFunctions.h:80
> +    ASSERT(WKStringGetMaximumUTF8CStringSize(stringRef) <= staticBufferSize);

We can’t assert something just because we hope it’s true. There’s no guarantee this will be true, so the code is wrong!

> WebKit2/Shared/API/c/WKString.cpp:48
> +WKStringRef WKStringCreateWithUTF8CString(const char* string)
> +{
> +    return toRef(WebString::create(string).leakRef());
> +}

The name of this function does not match the implementation. The name indicates the string is treated as UTF-8, but the code actually treats the strings as ISO Latin-1.

> WebKit2/Shared/API/c/WKString.cpp:73
> +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> +{
> +    return *toWK(a) == b;
> +}

This function also won’t work right. The code treats the string as ISO Latin-1, not UTF-8.

> WebKit2/Shared/API/c/WKURL.cpp:40
> +WKURLRef WKURLCreateWithUTF8CString(const char* string)
> +{
> +    return toRef(WebURL::create(string).leakRef());
> +}

Same problem here. This won’t treat the input as a UTF-8 string.

> WebKit2/Shared/API/c/WKString.h:65
> +WK_EXPORT WKStringRef WKStringCreateWithCharacters(const WKChar* characters, size_t length);
> +
> +WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char* string);
> +
> +WK_EXPORT const WKChar* WKStringGetCharactersPtr(WKStringRef string);
> +
> +WK_EXPORT size_t WKStringGetLength(WKStringRef string);
> +
>  WK_EXPORT bool WKStringIsEmpty(WKStringRef string);
>  
> +WK_EXPORT bool WKStringIsEqual(WKStringRef a, WKStringRef b);
> +
> +WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b);
> +
> +WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef string);
> +
> +WK_EXPORT size_t WKStringGetUTF8CString(WKStringRef string, char* buffer, size_t bufferSize);

This might be too many operations. Do we really need all of these? For example, if we have GetCharactersPtr, I am not sure we need GetUTF8CString.

[Balazs Kelemen]

(In reply to comment #25)
> (From update of attachment 69217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69217&action=review
> 
> Seems like a reasonable basic approach, but there are some problems.
> 
> > WebKitTools/WebKitTestRunner/StringFunctions.h:80
> > +    ASSERT(WKStringGetMaximumUTF8CStringSize(stringRef) <= staticBufferSize);
> 
> We can’t assert something just because we hope it’s true. There’s no guarantee this will be true, so the code is wrong!
> 
> > WebKit2/Shared/API/c/WKString.cpp:48
> > +WKStringRef WKStringCreateWithUTF8CString(const char* string)
> > +{
> > +    return toRef(WebString::create(string).leakRef());
> > +}
> 
> The name of this function does not match the implementation. The name indicates the string is treated as UTF-8, but the code actually treats the strings as ISO Latin-1.
> 
> > WebKit2/Shared/API/c/WKString.cpp:73
> > +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> > +{
> > +    return *toWK(a) == b;
> > +}
> 
> This function also won’t work right. The code treats the string as ISO Latin-1, not UTF-8.
> 
> > WebKit2/Shared/API/c/WKURL.cpp:40
> > +WKURLRef WKURLCreateWithUTF8CString(const char* string)
> > +{
> > +    return toRef(WebURL::create(string).leakRef());
> > +}
> 
> Same problem here. This won’t treat the input as a UTF-8 string.
> 
> > WebKit2/Shared/API/c/WKString.h:65
> > +WK_EXPORT WKStringRef WKStringCreateWithCharacters(const WKChar* characters, size_t length);
> > +
> > +WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char* string);
> > +
> > +WK_EXPORT const WKChar* WKStringGetCharactersPtr(WKStringRef string);
> > +
> > +WK_EXPORT size_t WKStringGetLength(WKStringRef string);
> > +
> >  WK_EXPORT bool WKStringIsEmpty(WKStringRef string);
> >  
> > +WK_EXPORT bool WKStringIsEqual(WKStringRef a, WKStringRef b);
> > +
> > +WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b);
> > +
> > +WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef string);
> > +
> > +WK_EXPORT size_t WKStringGetUTF8CString(WKStringRef string, char* buffer, size_t bufferSize);
> 
> This might be too many operations. Do we really need all of these? For example, if we have GetCharactersPtr, I am not sure we need GetUTF8CString.

This is (near to) the minimal set we need in WTR.
I have introduced WKStringGetCharactersPtr to be able to create a JSString from a WKString without copying and I have introduced WKStringGetUTF8CString to be able to dump the string through iostream. However, according to the coding problems you mentioned, the latter should have been WKStringGetLatin1CString.
I am thinking about introducing a cpp API for streaming. The other coding problems will be fixed in the next patch.

[Balazs Kelemen]

> > WebKit2/Shared/API/c/WKString.cpp:73
> > +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> > +{
> > +    return *toWK(a) == b;
> > +}
> 
> This function also won’t work right. The code treats the string as ISO Latin-1, not UTF-8.

I do not agree with that. As I see operator==(const WTF::String&, const char*) do the right thing for both Latin-1 and UTF-8 strings.
Only creation with C strings should be fixed.

[Darin Adler]

(In reply to comment #27)
> > > WebKit2/Shared/API/c/WKString.cpp:73
> > > +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> > > +{
> > > +    return *toWK(a) == b;
> > > +}
> > 
> > This function also won’t work right. The code treats the string as ISO Latin-1, not UTF-8.
> 
> I do not agree with that. As I see operator==(const WTF::String&, const char*) do the right thing for both Latin-1 and UTF-8 strings.
> Only creation with C strings should be fixed.

That’s wrong.

Maybe you think that the const char* is guaranteed to be entirely ASCII?

[Darin Adler]

(In reply to comment #28)
> (In reply to comment #27)
> > > > WebKit2/Shared/API/c/WKString.cpp:73
> > > > +bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b)
> > > > +{
> > > > +    return *toWK(a) == b;
> > > > +}
> > > 
> > > This function also won’t work right. The code treats the string as ISO Latin-1, not UTF-8.
> > 
> > I do not agree with that. As I see operator==(const WTF::String&, const char*) do the right thing for both Latin-1 and UTF-8 strings.
> > Only creation with C strings should be fixed.
> 
> That’s wrong.
> 
> Maybe you think that the const char* is guaranteed to be entirely ASCII?

It’s easy to show that’s wrong. Use a WTF::String that has a single non-breaking space in it and compare with a const char* that has the UTF-8 sequence for a non-breaking space. The function will return false but it should return true.

If you’d like me to write out the test code for you, I’d be happy to.

[Balazs Kelemen]

> It’s easy to show that’s wrong. Use a WTF::String that has a single non-breaking space in it and compare with a const char* that has the UTF-8 sequence for a non-breaking space. The function will return false but it should return true.
> 
> If you’d like me to write out the test code for you, I’d be happy to.

I have take another look into StringImpl.cpp so I have realized that you are right. No need to write out the test code. (Maybe finally I will learn what does it mean UTF-8 and Latin-1.)

[Balazs Kelemen]

Created attachment 69457 [details]
proposed patch

[Sam Weinig]

Hm, I didn't realize this bug turned into new API for WKString.  I have done that in a separate bug as well (https://bugs.webkit.org/show_bug.cgi?id=46958), in a way that I thinks fits the style of our code better (the actual implementation is in WebString for instance) and includes tests.

[Balazs Kelemen]

Comment on attachment 69457 [details]
proposed patch

Removing r? because it would need to be rebased when Sam's patch lands.

[Balazs Kelemen]

Created attachment 69546 [details]
rebased patch

[Early Warning System Bot]

Attachment 69546 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4145039

[Balazs Kelemen]

Created attachment 69569 [details]
patch v9

Build fixed.

[Balazs Kelemen]

Comment on attachment 69569 [details]
patch v9

I am late again:
http://trac.webkit.org/changeset/68961, http://trac.webkit.org/changeset/68966.

[Balazs Kelemen]

Sam, could you implement URL stuff? I am getting tired of rebasing...

[Sam Weinig]

(In reply to comment #38)
> Sam, could you implement URL stuff? I am getting tired of rebasing...

Yeah, I will handle the remaining issues,

[Balazs Kelemen]

Created attachment 69932 [details]
Patch

[Balazs Kelemen]

Created attachment 69933 [details]
Patch

Typo fix for the previous one.

[Kenneth Rohde Christiansen]

Comment on attachment 69933 [details]
Patch

This looks pretty OK to me, but let's give Darin some time to comment before landing.

[Balazs Kelemen]

(In reply to comment #42)
> (From update of attachment 69933 [details])
> This looks pretty OK to me, but let's give Darin some time to comment before landing.

Roger that. Actually it has a trivial build error but of course I can fix it before landing.

[Darin Adler]

Comment on attachment 69933 [details]
Patch

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

> WebKit2/Shared/API/c/WKURL.h:37
> +WK_EXPORT WKURLRef WKURLCreateWithUTF8CString(const char* string);

This seems OK. Another way to do it is to make this take a WKStringRef.

[Balazs Kelemen]

Committed r69297: <http://trac.webkit.org/changeset/69297>

[Andras Becsi]

Comment on attachment 69933 [details]
Patch

Clearing flags.