Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676443004:
Do not treat substitute-data resources as having successful HTTP status codes. (Closed)
|
Created:
3 years, 10 months ago by Mike West Modified:
3 years, 9 months ago Reviewers:
Nate Chapin CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, kinuko+watch, loading-reviews_chromium.org, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not treat substitute-data resources as having successful HTTP status codes.
Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP
status code of `0`. [1] changed this behavior to synthesize a 200 status code. This
patch changes that behavior back to the original expectation, matching the
expectation that non-network requests don't have network status information.
[1]: https://chromium.googlesource.com/chromium/blink/+/63b8349e922a3691bb77f74d7b2f143a5f023626
BUG=656845
Review-Url: https://codereview.chromium.org/2676443004
Cr-Commit-Position: refs/heads/master@{#455810}
Committed: https://chromium.googlesource.com/chromium/src/+/1709b0599a7329585a3d781b21b1cbbb15379d7c
Patch Set 1 #Patch Set 2 : Tests. #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : Rebase #Patch Set 5 : Rebaseline #
Messages
Total messages: 39 (30 generated)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
mkwst@chromium.org changed reviewers: + japhet@chromium.org
Nate, mind taking a look at this?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not treat substitute-data resources as having successful HTTP status codes. BUG=656845 ========== to ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on `data:` URLs and error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. BUG=656845 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM, though unclear to me if those android test failures are related. https://codereview.chromium.org/2676443004/diff/2/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (left): https://codereview.chromium.org/2676443004/diff/2/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:386: response.setHTTPStatusCode(200); This also means data: urls won't have status code. I assume that's ok?
On 2017/02/03 at 18:00:22, japhet wrote: > LGTM, though unclear to me if those android test failures are related. They don't look related, but I'll poke at them, I suppose, if the CQ continues to be unhappy with me. > https://codereview.chromium.org/2676443004/diff/2/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (left): > > https://codereview.chromium.org/2676443004/diff/2/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:386: response.setHTTPStatusCode(200); > This also means data: urls won't have status code. I assume that's ok? I think that's fine. I mean, they don't hit the network, right? Giving them a network status seems strange. Since only 3 tests depend on that behavior, and all of them only depend on it by writing out the status code into the expectation file, I think this is worth landing as-is.
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on `data:` URLs and error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. BUG=656845 ========== to ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. BUG=656845 ==========
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:30001) has been deleted
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. BUG=656845 ========== to ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. [1]: https://chromium.googlesource.com/chromium/blink/+/63b8349e922a3691bb77f74d7b... BUG=656845 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2676443004/#ps70001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2676443004/#ps90001 (title: "Rebaseline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1489079109870560,
"parent_rev": "6c3437a6c08a5e4882d5bb6592db12dcdfbc07f6", "commit_rev":
"1709b0599a7329585a3d781b21b1cbbb15379d7c"}
Message was sent while issue was closed.
Description was changed from ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. [1]: https://chromium.googlesource.com/chromium/blink/+/63b8349e922a3691bb77f74d7b... BUG=656845 ========== to ========== Do not treat substitute-data resources as having successful HTTP status codes. Some embedders of Blink (Chrome in particular) rely on error pages having an HTTP status code of `0`. [1] changed this behavior to synthesize a 200 status code. This patch changes that behavior back to the original expectation, matching the expectation that non-network requests don't have network status information. [1]: https://chromium.googlesource.com/chromium/blink/+/63b8349e922a3691bb77f74d7b... BUG=656845 Review-Url: https://codereview.chromium.org/2676443004 Cr-Commit-Position: refs/heads/master@{#455810} Committed: https://chromium.googlesource.com/chromium/src/+/1709b0599a7329585a3d781b21b1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001) as https://chromium.googlesource.com/chromium/src/+/1709b0599a7329585a3d781b21b1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
