Created attachment 61919 [details] Fix Qt WebKit HTTP 204 response According to RFC 2616 section 10.2.5, a response of "204 No Content" should not cause a user agent to update its document view. Qt WebKit incorrectly displays a blank page. The attached patch seems to fix the problem. I lifted the solution from the corresponding Chromium class.
Arg. A hidden patch :( Your patch was not seen by anyone for 2 reason: 1) you did not use the template to report the bug, so it was not triaged. See http://trac.webkit.org/wiki/QtWebKitBugs 2) you did not ask for review when attaching the patch. See http://trac.webkit.org/wiki/QtWebKitContrib I suggest you to update the patch, create a changelog entry, and resubmit with "r?".
Created attachment 87763 [details] patch
Comment on attachment 87763 [details] patch A test would be nice.
Comment on attachment 87763 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=87763&action=review A test would be nice. > Source/WebKit/qt/ChangeLog:8 > + Ignoring http responses which have status code equal to 204 or 205. I suggest to give the code name in the changelog so people don't have to look by themself what are those code for. > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1184 > + if (statusCode == 204 || statusCode == 205) { > + // FIXME: a 205 response requires that the requester reset the document view. Shouldn't 205 just be ignored then so the view is reloaded? Call me crazy but what about?: enum { HttpStatusNoContent = 204 } ... if (statusCode == HttpStatusNoContent, etc)
> Call me crazy but what about?: > enum { > HttpStatusNoContent = 204 > } > ... > if (statusCode == HttpStatusNoContent, etc) I was complaining about something like that some days ago. I'm actually pretty sure that such a thing already exists in WebKit or at least used to.
(In reply to comment #4) > (From update of attachment 87763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87763&action=review > > A test would be nice. I need to find a way to test it. > > > Source/WebKit/qt/ChangeLog:8 > > + Ignoring http responses which have status code equal to 204 or 205. > > I suggest to give the code name in the changelog so people don't have to look by themself what are those code for. ok > > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1184 > > + if (statusCode == 204 || statusCode == 205) { > > + // FIXME: a 205 response requires that the requester reset the document view. > > Shouldn't 205 just be ignored then so the view is reloaded? The 205 response must be ignored in the sense that its content (that should be empty) will not be shown. But the forms in the current content should be cleared as well. RFC 2616 says: 10.2.6 205 Reset Content The server has fulfilled the request and the user agent SHOULD reset the document view which caused the request to be sent. This response is primarily intended to allow input for actions to take place via user input, followed by a clearing of the form in which the input is given so that the user can easily initiate another input action. The response MUST NOT include an entity. > > Call me crazy but what about?: > enum { > HttpStatusNoContent = 204 > } > ... > if (statusCode == HttpStatusNoContent, etc) what about this? switch (response.httpStatusCode()) { case 205: // Reset Content // FIXME: a 205 response requires that the requester reset the document view. case 204: // No Content callPolicyFunction(function, PolicyIgnore); return; }
(In reply to comment #5) > > Call me crazy but what about?: > > enum { > > HttpStatusNoContent = 204 > > } > > ... > > if (statusCode == HttpStatusNoContent, etc) > > I was complaining about something like that some days ago. I'm actually pretty sure that such a thing already exists in WebKit or at least used to. I did not find such a thing.
Created attachment 87876 [details] patch
Comment on attachment 87876 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=87876&action=review I stand by the enum for the Http response code, it is harder to get an enum out of sync with the value, and I hate magic numbers in general :) > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1182 > + whitespace? > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1185 > + You should add // Fallthrough, and have fixme separately somehow. I have the feeling someone will think a "break;" is missing here :) > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1189 > + whitespace?
Created attachment 88848 [details] patch
Attachment 88848 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:278: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 88850 [details] patch
Comment on attachment 88850 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=88850&action=review > LayoutTests/ChangeLog:8 > + Responses which have status code equal to 204 should be ignored. Oops! We're also changing the behavior of 205 responses without a test or mention. > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:609 > if (m_webFrame && m_frame->page()) > - emit loadFinished(m_loadError.isNull()); > + emit loadFinished(m_loadError.isNull() || m_loadError.isCancellation()); This looks vaguely unrelated, what's this change about, and how can we test it? > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:281 > + enum HTTPStatusCodes { > + NoContent = 204, > + ResetContent = 205 > + }; This enum should be called HTTPStatusCode, and I think we should put it in a central location, something like Source/WebCore/platform/network/HTTPStatusCode.h There are a number of places in WebKit that would look nicer without such numeric constants inline. I also think the values should be prefixed with HTTP, i.e HTTPNoContent, HTTPResetContent.
(In reply to comment #13) > (From update of attachment 88850 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88850&action=review > > > LayoutTests/ChangeLog:8 > > + Responses which have status code equal to 204 should be ignored. > > Oops! We're also changing the behavior of 205 responses without a test or mention. This change log is in LayoutTests and the test that I am adding is just about 204. That is why the comment is just about 204. I did not make tests to 205 because we do not implement its specified behavior fully. The other change log mentions 205. > > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:609 > > if (m_webFrame && m_frame->page()) > > - emit loadFinished(m_loadError.isNull()); > > + emit loadFinished(m_loadError.isNull() || m_loadError.isCancellation()); > > This looks vaguely unrelated, what's this change about, and how can we test it? If a request is sent to the server and it returns to the client a 204 the client screen will not be updated but no error occurred. Why would loadFinished notify an error. It is actually been tested. Without this change the layout test that this patch adds times out. :) > > > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:281 > > + enum HTTPStatusCodes { > > + NoContent = 204, > > + ResetContent = 205 > > + }; > > This enum should be called HTTPStatusCode, and I think we should put it in a central location, something like Source/WebCore/platform/network/HTTPStatusCode.h > There are a number of places in WebKit that would look nicer without such numeric constants inline. > I also think the values should be prefixed with HTTP, i.e HTTPNoContent, HTTPResetContent. ok.
the tests names seem to be wrong. 204 and 205 are not errors!
Created attachment 92177 [details] patch
Comment on attachment 92177 [details] patch Looks nicer. Now I think you can post a mail on webkit-dev to see if some people are interested to use the enum in their port. A least to made them aware.
Comment on attachment 92177 [details] patch LGTM
Comment on attachment 92177 [details] patch Rejecting attachment 92177 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2 Last 500 characters of output: imerredirect-subframeload.html -> failed . http/tests/navigation/useragent.php -> failed . http/tests/navigation/window-open-adds-history-item2.html -> failed . http/tests/navigation/window-open-adds-history-item.html -> failed http/tests/plugins . http/tests/plugins/cross-frame-object-access.html -> failed Exiting early after 20 failures. 22720 tests run. 643.46s total testing time 22700 test cases (99%) succeeded 20 test cases (<1%) had incorrect layout 13 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8571085
Created attachment 92289 [details] Archive of layout-test-results from cr-jail-4 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-4 Port: Mac Platform: Mac OS X 10.6.7
This change may have caused the 400+ new failures on the Leopard bot: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r85783%20(31877)/results.html Please investigate.
http://trac.webkit.org/changeset/85774 might have broken GTK Linux 64-bit Debug The following tests are not passing: http/tests/navigation/response204.html
(In reply to comment #21) > This change may have caused the 400+ new failures on the Leopard bot: > http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r85783%20(31877)/results.html Please investigate. I was expecting the new test (http/tests/navigation/response204.html) to fail in some platforms but it should fail alone. It seems that for Mac and Win if the new test fail then tons of other tests fail as well. The new test was skipped (r85784) for Mac, Win and GTK. Only Qt and Chromium are running it.
commited. http://trac.webkit.org/changeset/85774