Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994173002:
Copy priority from original request for redirects (Closed)
|
Created:
4 years, 7 months ago by dproy-google Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopy priority from original request for redirects
We construct a new request to represent a redirect, but never set its priority.
This fixes that.
BUG=613165
Committed: https://crrev.com/785071ee5caf124d6e9d24ad7cb279815261748a
Cr-Commit-Position: refs/heads/master@{#397167}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 4
Patch Set 3 : fix nits #Patch Set 4 : 80 char #
Messages
Total messages: 24 (12 generated)
dproy@chromium.org changed reviewers: + caseq@chromium.org, esprehn@chromium.org
dproy@chromium.org changed reviewers: + dproy@chromium.org
Description was changed from ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This is fixes that. BUG=613165 ========== to ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This fixes that. BUG=613165 ==========
The CQ bit was checked by dproy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994173002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
No test?
On 2016/05/19 16:46:47, esprehn wrote: > No test? Added test. PTAL.
this lgtm, but can you explain how the test is checking the value is set properly? https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... File content/child/web_url_loader_impl_unittest.cc (right): https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... content/child/web_url_loader_impl_unittest.cc:134: if(check_redirect_request_priority_) missing space before (, did the presubmit not warn about this? https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... content/child/web_url_loader_impl_unittest.cc:397: EXPECT_EQ(kTestData, client()->received_data()); Can you explain how this test asserts that the priority is handled correctly?
https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... File content/child/web_url_loader_impl_unittest.cc (right): https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... content/child/web_url_loader_impl_unittest.cc:134: if(check_redirect_request_priority_) On 2016/06/01 05:10:32, esprehn wrote: > missing space before (, did the presubmit not warn about this? I don't think it did. Maybe because it's a test file. Will fix. https://codereview.chromium.org/1994173002/diff/20001/content/child/web_url_l... content/child/web_url_loader_impl_unittest.cc:397: EXPECT_EQ(kTestData, client()->received_data()); On 2016/06/01 05:10:32, esprehn wrote: > Can you explain how this test asserts that the priority is handled correctly? client->set_redirect_request_priority(p) makes sure when willFollowRedirect is called, the mock client will check the priority of new request is p. The real check is done on line 135. The other asserts are copied from the test above. I just left them there because I thought it wouldn't hurt to make sure when we set a non-default priority, everything else still works as usual.
The CQ bit was checked by dproy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1994173002/#ps40001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994173002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dproy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1994173002/#ps60001 (title: "80 char")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994173002/60001
Message was sent while issue was closed.
Description was changed from ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This fixes that. BUG=613165 ========== to ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This fixes that. BUG=613165 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This fixes that. BUG=613165 ========== to ========== Copy priority from original request for redirects We construct a new request to represent a redirect, but never set its priority. This fixes that. BUG=613165 Committed: https://crrev.com/785071ee5caf124d6e9d24ad7cb279815261748a Cr-Commit-Position: refs/heads/master@{#397167} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/785071ee5caf124d6e9d24ad7cb279815261748a Cr-Commit-Position: refs/heads/master@{#397167} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
