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).
Created attachment 66900 [details] proposed patch
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.
Created attachment 66903 [details] proposed patch Fix the true positive check-webkit-style errors.
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.
Created attachment 67008 [details] proposed patch This one removes all CFString and CFURL usage from WTR from the common (not port specific) files.
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.
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.
(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?
> > 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?
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.
(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.
(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.
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.
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.
Is there somebody who could review my poor patch?
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*.
> > > 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.
(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.
> 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.
(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.
(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.
Created attachment 69203 [details] proposed patch
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.
Created attachment 69217 [details] proposed patch Fixed style and debug build issues.
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.
(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.
> > 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.
(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?
(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.
> 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.)
Created attachment 69457 [details] proposed patch
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.
Comment on attachment 69457 [details] proposed patch Removing r? because it would need to be rebased when Sam's patch lands.
Created attachment 69546 [details] rebased patch
Attachment 69546 [details] did not build on qt: Build output: http://queues.webkit.org/results/4145039
Created attachment 69569 [details] patch v9 Build fixed.
Comment on attachment 69569 [details] patch v9 I am late again: http://trac.webkit.org/changeset/68961, http://trac.webkit.org/changeset/68966.
Sam, could you implement URL stuff? I am getting tired of rebasing...
(In reply to comment #38) > Sam, could you implement URL stuff? I am getting tired of rebasing... Yeah, I will handle the remaining issues,
Created attachment 69932 [details] Patch
Created attachment 69933 [details] Patch Typo fix for the previous one.
Comment on attachment 69933 [details] Patch This looks pretty OK to me, but let's give Darin some time to comment before landing.
(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.
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.
Committed r69297: <http://trac.webkit.org/changeset/69297>
Comment on attachment 69933 [details] Patch Clearing flags.