Bug 77380

Why do you expect the results to be port specific (other than chromium vs. everyone else)?

(In reply to comment #1) > Why do you expect the results to be port specific (other than chromium vs. everyone else)? I don't, I may have got confused yesterday... maybe I saw many expected results for almost all ports, but I've checked it again now and it looks like how you've just pointed out, only chromium has/should have a different result. Thanks. :)

Funny story, we're getting the very same result as expected for every port (except chromium) and I just figured it out. But... now I already did all the research on this failure: FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/. We go through KURL::init and then we reach the static function encodeHostnames, which identifies the size of the host inside the string and then append the encoded host to the output. Note how odd this is, we don't keep any info about where the host was at this point, we calculate it there and at some point later we do it again. As we're using ICU now, we convert this '／' to '/' and return this string containing only ascii characters back to KURL::init. As %2F is actually the percent encoded '/', this is exactly what we need (as shown in the test result). But we no longer know that it is inside the host name (that function shouldn't be static). Later we try to identify the host name again and then this '/' mislead. We end up with host being 'example.com' and path, '/foo/'. In this case, there's no need to percent encode anything because it is not in the host name. We could even make the code faster if we could save the host info at that point, so we won't need to waste time doing it again (wrongly) later. Alexey, Darin, do you have any thoughts on this? :-)

The reason why WK1 and WK2 differ results is because of DumpRenderTree code. On WK2, we print WTF::String directly to stdout, so we get the expected result for it. But for WK1, we manage to (badly) convert to QString before printing to stdout. After that conversion, if you create a new WTF::String with the data on this QString, we may not end up with the same string as before... so this is very worrying. The case that fails for Qt5-WK1 is: FAIL canonicalize('http://example.com/�zyx') We produce: FAIL canonicalize('http://example.com/?zyx') Modifying the return of QWebFrame::toPlainText to use QString::fromUtf8 should have done the trick, but the strings didn't match yet. it seems that fromUtf8 is replacing its invalid character by one of the replacement characters (as described on http://doc.qt.nokia.com/5.0-snapshot/qstring.html#fromUtf8), so it will not match when we convert back to WTF::String or when we compare the expected file with this one. Besides we're failing on this diff, we should keep in mind that the behavior is the same internally and we canonicalize just like the other ports (which is the purpose of the test). When Jesus finish his work on bug #77450, I'll remove the skip only for Qt5-WK2.

Created attachment 143653 [details] Patch

Btw, this bug must still open just in case someone decides to work on wk1.

Comment on attachment 143653 [details] Patch Clearing flags on attachment: 143653 Committed r118256: <http://trac.webkit.org/changeset/118256>

All reviewed patches have been landed. Closing bug.

Reopening as we still have it skipped on WK1. Basically we only need to change the way DumpRenderTree prints the result to do that directly to stdout rather than concatenating everything to a QString. Another way is to fix the conversion to QString on Qt.

(In reply to comment #7) > (From update of attachment 143653 [details]) > Clearing flags on attachment: 143653 > > Committed r118256: <http://trac.webkit.org/changeset/118256> I don't understand this unskipping ... this test still fail on WK1 and WK2 too: Qt 5.0-WK1 fail: ----------------- --- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-expected.txt +++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-actual.txt @@ -41,7 +41,7 @@ PASS canonicalize('http://example.com/‥/foo') is 'http://example.com/%E2%80%A5/foo' PASS canonicalize('http://example.com//foo') is 'http://example.com/%EF%BB%BF/foo' PASS canonicalize('http://example.com//foo//bar') is 'http://example.com/%E2%80%AE/foo/%E2%80%AD/bar' -FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http:/. +FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/. PASS successfullyParsed is true TEST COMPLETE Qt 5.0-WK2 fail: ----------------- --- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-expected.txt +++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-actual.txt @@ -37,11 +37,11 @@ PASS canonicalize('http://example.com/%7Ffp3%3Eju%3Dduvgw%3Dd') is 'http://example.com/%7Ffp3%3Eju%3Dduvgw%3Dd' PASS canonicalize('http://example.com/@asdf%40') is 'http://example.com/@asdf%40' PASS canonicalize('http://example.com/你好你好') is 'http://example.com/%E4%BD%A0%E5%A5%BD%E4%BD%A0%E5%A5%BD' -FAIL canonicalize('http://example.com/?zyx') should be http://example.com/%EF%BF%BDzyx. Was http://example.com/%EF%B7%90zyx. +FAIL canonicalize('http://example.com/﷐zyx') should be http://example.com/%EF%BF%BDzyx. Was http://example.com/%EF%B7%90zyx. PASS canonicalize('http://example.com/‥/foo') is 'http://example.com/%E2%80%A5/foo' PASS canonicalize('http://example.com//foo') is 'http://example.com/%EF%BB%BF/foo' PASS canonicalize('http://example.com//foo//bar') is 'http://example.com/%E2%80%AE/foo/%E2%80%AD/bar' -FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http:/. +FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/. PASS successfullyParsed is true TEST COMPLETE

It should block bug79666, because it is a regression. (It passes with Qt 4.8, but fails with newer.)

Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372

(In reply to comment #12) > Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372 It fails on Qt5-wk2?

(In reply to comment #13) > (In reply to comment #12) > > Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372 > > It fails on Qt5-wk2? Ossy, Rafael got it, I forgot to remove the specific Qt expected file for this test in the patch, sorry. I'll re-upload it again.

Created attachment 143851 [details] Patch Expected file moved to Qt4.8

This still seems to be valid. Proposed patch works for WK1 and WK2.

Comment on attachment 143851 [details] Patch Clearing flags on attachment: 143851 Committed r126761: <http://trac.webkit.org/changeset/126761>

All reviewed patches have been landed. Closing bug.