Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(662)

Issue 2341893002: Don't attempt to revalidate a Resource with a 304 response. (Closed)

Created:
4 years, 3 months ago by Nate Chapin
Modified:
4 years, 3 months ago
Reviewers:
hiroshige
CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, gavinp+loader_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't attempt to revalidate a Resource with a 304 response. ...which is counter-intuitive. Normally, a revalidation is started on a Resource with a 2xx response. If the revalidation is successful (i.e, a 304 response), Resource updates it's m_response with the headers that were received on the 304 response, but leaves the original 2xx response status code. A Resource with a 304 status code on its m_response therefore received the 304 without thinking it issued a revaldation. This could happen as a result of a server behaving badly, or it could happen because an XHR was created with manually added revalidation headers. In either case, the Resource is not guaranteed to have sufficient information to perform a successful revalidtion, so don't attempt a revalidation with it. BUG=643659 TEST=ResourceTest.Revalidate304 Committed: https://crrev.com/d343f801f8cd63f985180bb4acb9cd4949e4c4b9 Cr-Commit-Position: refs/heads/master@{#420771}

Patch Set 1 #

Patch Set 2 : Move fix to ResourceFetcher::determineRevalidationPolicy, test to ResourceFetcherTest #

Total comments: 6

Patch Set 3 : Fix comment issues #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -2 lines) Patch
M third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp View 1 2 1 chunk +10 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp View 1 2 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (14 generated)
Nate Chapin
hiroshige, does this seem reasonable to you?
4 years, 3 months ago (2016-09-14 21:09:23 UTC) #6
hiroshige
On 2016/09/14 21:09:23, Nate Chapin wrote: > hiroshige, does this seem reasonable to you? Thanks ...
4 years, 3 months ago (2016-09-15 06:27:25 UTC) #7
Nate Chapin
On 2016/09/15 06:27:25, hiroshige wrote: > On 2016/09/14 21:09:23, Nate Chapin wrote: > > hiroshige, ...
4 years, 3 months ago (2016-09-19 23:31:48 UTC) #10
Nate Chapin
On 2016/09/19 23:31:48, Nate Chapin wrote: > On 2016/09/15 06:27:25, hiroshige wrote: > > On ...
4 years, 3 months ago (2016-09-21 20:29:07 UTC) #13
hiroshige
lgtm with comments. https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode715 third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:715: // aremains 304. In this case, ...
4 years, 3 months ago (2016-09-23 16:00:19 UTC) #14
Nate Chapin
https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode715 third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:715: // aremains 304. In this case, the Resource likely ...
4 years, 3 months ago (2016-09-23 20:45:51 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2341893002/40001
4 years, 3 months ago (2016-09-23 20:46:29 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/285370)
4 years, 3 months ago (2016-09-23 22:22:13 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2341893002/40001
4 years, 3 months ago (2016-09-23 22:23:56 UTC) #22
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2016-09-23 23:09:44 UTC) #23
commit-bot: I haz the power
4 years, 3 months ago (2016-09-23 23:11:29 UTC) #25
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/d343f801f8cd63f985180bb4acb9cd4949e4c4b9
Cr-Commit-Position: refs/heads/master@{#420771}

Powered by Google App Engine
This is Rietveld 408576698