The KURL dependency introduced in http://trac.webkit.org/changeset/77794 brakes non-mac platforms: symbol lookup error: /home/balazs/WebKitGit/WebKitBuild/Release/lib/libWTRInjectedBundle.so: undefined symbol: _ZN7WebCore4KURLC1ENS_18ParsedURLStringTagEPKc #CRASHED - WebProcess To fix this we should either export those symbols everywhere or do not use WebCore internally. Since the second is already a general concept of WTR I believe that would be the right solution.
*** Bug 54002 has been marked as a duplicate of this bug. ***
Created attachment 81822 [details] Patch
Attachment 81822 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:627: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
I removed a comment accidentally, this should be fixed before commit. The style issue is better to be ignored in my opinion, it is pretty confusing to write |!urlString.find("http")| to say that "if http is prefix of the string".
Comment on attachment 81822 [details] Patch The patch makes two tests failing. Need to find a better approach.
Created attachment 82453 [details] Patch
Attachment 82453 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:636: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:637: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:638: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:639: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:644: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:647: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Tests are OK on my Mac (there are a few error but not related to this).
Comment on attachment 82453 [details] Patch As discussed on IRC this should rather be fixed by adding API to WKURL.
Created attachment 82975 [details] Patch
*** Bug 54900 has been marked as a duplicate of this bug. ***
Darin can you have a look at the new C API?
Comment on attachment 82975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82975&action=review > Source/WebCore/WebCore.exp.in:-632 > -__ZN7WebCore4KURLC1ENS_18ParsedURLStringTagEPKc You should also revert r77816. > Source/WebKit2/Shared/API/c/WKString.h:50 > -WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char* string); > +WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char*); > > -WK_EXPORT bool WKStringIsEmpty(WKStringRef string); > +WK_EXPORT bool WKStringIsEmpty(WKStringRef); > > -WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef string); > +WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef); > WK_EXPORT size_t WKStringGetUTF8CString(WKStringRef string, char* buffer, size_t bufferSize); > > -WK_EXPORT bool WKStringIsEqual(WKStringRef a, WKStringRef b); > -WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b); > +WK_EXPORT bool WKStringIsEqual(WKStringRef, WKStringRef); > +WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef, const char*); > +WK_EXPORT bool WKStringIsEqualToUTF8CStringIgnoringCase(WKStringRef, const char*); So far all of our C API headers do not omit argument names. I'd suggest undoing those changes. We can always discuss that separately from this patch. > Source/WebKit2/Shared/API/c/WKURL.h:41 > -WK_EXPORT WKURLRef WKURLCreateWithUTF8CString(const char* string); > +WK_EXPORT WKURLRef WKURLCreateWithUTF8CString(const char*); > > -WK_EXPORT WKStringRef WKURLCopyString(WKURLRef URL); > +WK_EXPORT WKStringRef WKURLCopyString(WKURLRef); > > -WK_EXPORT bool WKURLIsEqual(WKURLRef a, WKURLRef b); > +WK_EXPORT bool WKURLIsEqual(WKURLRef, WKURLRef); Same comment here about argument names. > Source/WebKit2/Shared/API/c/WKURL.h:45 > +WK_EXPORT WKStringRef WKURLCopyProtocol(WKURLRef); The correct term is "scheme", not "protocol". > Source/WebKit2/Shared/WebURL.h:62 > + String host() const > + { > + WebCore::KURL url(WebCore::KURL(), m_string); > + return url.isValid() ? url.host() : String(); > + } > + > + String protocol() const > + { > + WebCore::KURL url(WebCore::KURL(), m_string); > + return url.isValid() ? url.protocol() : String(); > + } It seems unfortunate to create and throw away a KURL each time these functions are called. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:627 > + WKRetainPtr<WKStringRef> host = WKURLCopyHostName(url.get()); > + WKRetainPtr<WKStringRef> protocol = WKURLCopyProtocol(url.get()); These two lines will leak. You need to adopt the result of Copy-style functions. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:628 > + if (host.get() No need for .get() here. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:632 > + && (!WKStringIsEqualToUTF8CString(host.get(), "127.0.0.1")) > + && (!WKStringIsEqualToUTF8CString(host.get(), "255.255.255.255")) // Used in some tests that expect to get back an error. > + && (!WKStringIsEqualToUTF8CStringIgnoringCase(host.get(), "localhost"))) { I don't think the extra parentheses are helping here. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:633 > + InjectedBundle::shared().os() << "Blocked access to external URL " << toSTD(adoptWK(WKURLCopyString(url.get()))) << "\n"; We should add an operator<< that takes a WKURLRef.
Comment on attachment 82975 [details] Patch I guess my previous comments were enough to warrant an r- on this version.
Created attachment 83899 [details] Patch Incorporated review comments with the exception of still using temporary KURL's.
Attachment 83899 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/API/c/WKURL.h:39: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/API/c/WKURL.h:43: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/API/c/WKURL.h:45: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ping. Sam, it has been sad that you are the most appropriate man to review this, but I don't find you on IRC. Could you review the patch? Thanks.
Comment on attachment 83899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83899&action=review I think this is really close! Just a few comments. >> Source/WebKit2/Shared/API/c/WKURL.h:43 >> +WK_EXPORT WKStringRef WKURLCopyHostName(WKURLRef url); > > The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] I think you should put these new functions just beneath WKURLCopyString, all in the same paragraph. > Source/WebKit2/Shared/WebURL.h:62 > + String host() const > + { > + WebCore::KURL url(WebCore::KURL(), m_string); > + return url.isValid() ? url.host() : String(); > + } > + > + String protocol() const > + { > + WebCore::KURL url(WebCore::KURL(), m_string); > + return url.isValid() ? url.protocol() : String(); > + } Maybe it would be better to have an OwnPtr<KURL> data member. It would be lazily initialized using m_string. That way only the first call to one of these functions would have to pay the cost of parsing the string. > Tools/WebKitTestRunner/StringFunctions.h:101 > + WKRetainPtr<WKStringRef> urlString(AdoptWK, WKURLCopyString(urlRef)); > + return out << toSTD(urlString.get()); Can you use the adoptWK function here? No need for the .get().
Created attachment 85598 [details] Patch
Attachment 85598 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/API/c/WKURL.h:39: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/API/c/WKURL.h:40: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/API/c/WKURL.h:41: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #18) > (From update of attachment 83899 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83899&action=review > > I think this is really close! Just a few comments. > > >> Source/WebKit2/Shared/API/c/WKURL.h:43 > >> +WK_EXPORT WKStringRef WKURLCopyHostName(WKURLRef url); > > > > The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] > > I think you should put these new functions just beneath WKURLCopyString, all in the same paragraph. Done. > > > Source/WebKit2/Shared/WebURL.h:62 > > + String host() const > > + { > > + WebCore::KURL url(WebCore::KURL(), m_string); > > + return url.isValid() ? url.host() : String(); > > + } > > + > > + String protocol() const > > + { > > + WebCore::KURL url(WebCore::KURL(), m_string); > > + return url.isValid() ? url.protocol() : String(); > > + } > > Maybe it would be better to have an OwnPtr<KURL> data member. It would be lazily initialized using m_string. That way only the first call to one of these functions would have to pay the cost of parsing the string. Done. > > > Tools/WebKitTestRunner/StringFunctions.h:101 > > + WKRetainPtr<WKStringRef> urlString(AdoptWK, WKURLCopyString(urlRef)); > > + return out << toSTD(urlString.get()); > > Can you use the adoptWK function here? Done. adoptWK was defined in InjectedBundlePage.cpp so I moved it to here (this file is included almost everywhere). I will upload the bug for the check-webkit-style as discussed on IRC in a day or two.
(In reply to comment #20) > Attachment 85598 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > > Source/WebKit2/Shared/API/c/WKURL.h:39: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/Shared/API/c/WKURL.h:40: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/Shared/API/c/WKURL.h:41: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 3 in 12 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Filed bug with patch against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=56321
Comment on attachment 85598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85598&action=review > Source/WebKit2/Shared/WebURL.h:72 > + if (!!m_parsedURL) No need for the !! here. > Source/WebKit2/Shared/WebURL.h:74 > + m_parsedURL.set(new WebCore::KURL(WebCore::KURL(), m_string)); Please use adoptPtr and assignment here instead of OwnPtr::set. > Tools/WebKitTestRunner/StringFunctions.h:100 > +inline std::ostream& operator<<(std::ostream& out, const WKURLRef urlRef) No need for the "const" here.
Committed r81084: <http://trac.webkit.org/changeset/81084>