Bug 36690

Created attachment 51960 [details] Patch helps in detetcting http redirect loops in Qt

Attachment 51960 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/network/qt/QNetworkReplyHandler.cpp:252: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 52035 [details] Re-submitting patch according to style rules

Comment on attachment 52035 [details] Re-submitting patch according to style rules LayoutTests ChangeLog is bogus.

Created attachment 52447 [details] Patch file adhereing to style rules

Comment on attachment 52447 [details] Patch file adhereing to style rules Tab in the ChangeLog will prevent this from being committed. I'm surprised the style bot didn't catch that. This looks sane, although I'm surprised the Qt network layer doesn't do this for you.

Comment on attachment 52447 [details] Patch file adhereing to style rules r- as this now has to be reworked after fix for 37097 is landed. The same bug (missing feature) cause both xhr and app cache failures. Bug 37097 is using 10 as a redirect limit, I'm ok with that instead of 16 proposed as part of this patch. Krithiga, can you rewor your patch so that it works on top of patch for bug 37097. I think your error handling code and the Skipped list update is still needed.

(In reply to comment #7) > (From update of attachment 52447 [details]) > r- as this now has to be reworked after fix for 37097 is landed. The same bug > (missing feature) cause both xhr and app cache failures. > > Bug 37097 is using 10 as a redirect limit, I'm ok with that instead of 16 > proposed as part of this patch. > > Krithiga, can you rewor your patch so that it works on top of patch for bug > 37097. I think your error handling code and the Skipped list update is still > needed. Thanks for finding this out for us, Laszlo.

(In reply to comment #6) > (From update of attachment 52447 [details]) > Tab in the ChangeLog will prevent this from being committed. I'm surprised the > style bot didn't catch that. > > This looks sane, although I'm surprised the Qt network layer doesn't do this > for you. QtNetwork is pretty agnostic on this score. It passes back redirects to the client by setting an attribute in the QNetworkRequest: \value RedirectionTargetAttribute Replies only, type: QVariant::Url (no default) If present, it indicates that the server is redirecting the request to a different URL. The Network Access API does not by default follow redirections: it's up to the application to determine if the requested redirection should be allowed, according to its security policies. The returned URL might be relative. Use QUrl::resolved() to create an absolute URL out of it. So if QtWebKit wants to respect redirects it needs to sanity check them as well.

(In reply to comment #9) > (In reply to comment #6) > > (From update of attachment 52447 [details] [details]) > So if QtWebKit wants to respect redirects it needs to sanity check them as > well. Thanks for the clarification. This confirms the correctness of webkit approach.

Created attachment 52654 [details] Patch file for appcache layout tests that can be removed from the skipped list. New patch file containing the changes to layout skipped tests and corresponding change log.

ChangeLog has a small text alignment issue, otherwise looks good. Krithiga, can you fix it ?

Created attachment 52659 [details] new patch file following tab rules for Change log

Comment on attachment 52659 [details] new patch file following tab rules for Change log lgtm, r+.

Comment on attachment 52659 [details] new patch file following tab rules for Change log Clearing flags on attachment: 52659 Committed r57175: <http://trac.webkit.org/changeset/57175>

All reviewed patches have been landed. Closing bug.

Revision r57175 cherry-picked into qtwebkit-2.0 with commit f8d7b7e06126519131d47d84f5d8cc9f0873b9fb

(In reply to comment #17) > Revision r57175 cherry-picked into qtwebkit-2.0 with commit f8d7b7e06126519131d47d84f5d8cc9f0873b9fb This make http/tests/appcache/fallback.html crash on the qtwebkit-2.0 bot. Have you got any idea why? I tried against Qt 4.6.0, 4.6.2 and 4.7.0, but they didn't solve this crash.