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

Issue 2079783002: Move HSTS redirection from URLRequest to URLRequestHTTPJob. (Closed)

Created:
4 years, 6 months ago by Mike West
Modified:
4 years, 6 months ago
Reviewers:
mmenke
CC:
cbentzel+watch_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move HSTS redirection from URLRequest to URLRequestHTTPJob. As discussed in https://codereview.chromium.org/2053693002#msg22: HSTS is an HTTP feature, not generally applicable to URLRequests. It makes sense to move processing out of URLRequest, and into URLRequestHttpJob (where the work was actually being done anyway). This patch refactors the implementation a bit, and moves it over to the latter class. R=mmenke@chromium.org Committed: https://crrev.com/549a5c63cc0be42bae1669abddd6ade78221f477 Cr-Commit-Position: refs/heads/master@{#400674}

Patch Set 1 #

Total comments: 12

Patch Set 2 : mmenke@ #

Total comments: 2

Patch Set 3 : mmenke@ too #

Patch Set 4 : iOS #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -48 lines) Patch
M net/url_request/url_request.h View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M net/url_request/url_request.cc View 1 2 1 chunk +0 lines, -16 lines 0 comments Download
M net/url_request/url_request_http_job.cc View 1 2 chunks +26 lines, -7 lines 0 comments Download
M net/url_request/url_request_http_job_unittest.cc View 1 2 3 4 chunks +68 lines, -1 line 1 comment Download
M net/url_request/url_request_unittest.cc View 1 2 1 chunk +0 lines, -20 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 23 (10 generated)
Mike West
Extracted out of the other CL, what do you think of this move?
4 years, 6 months ago (2016-06-17 13:14:01 UTC) #1
mmenke
LGTM, modulo comments. Thanks for beefing up the test! https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job_unittest.cc File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job_unittest.cc#newcode642 net/url_request/url_request_http_job_unittest.cc:642: ...
4 years, 6 months ago (2016-06-17 15:06:18 UTC) #2
mmenke
https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job.h File net/url_request/url_request_http_job.h (right): https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job.h#newcode43 net/url_request/url_request_http_job.h:43: class URLRequestRedirectJob; Rather not forward declare this - can ...
4 years, 6 months ago (2016-06-17 15:08:07 UTC) #3
Mike West
Thanks! https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job.h File net/url_request/url_request_http_job.h (right): https://codereview.chromium.org/2079783002/diff/1/net/url_request/url_request_http_job.h#newcode43 net/url_request/url_request_http_job.h:43: class URLRequestRedirectJob; On 2016/06/17 at 15:08:07, mmenke wrote: ...
4 years, 6 months ago (2016-06-17 16:35:17 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079783002/20001
4 years, 6 months ago (2016-06-17 16:35:28 UTC) #7
mmenke
https://codereview.chromium.org/2079783002/diff/20001/net/url_request/url_request_http_job_unittest.cc File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2079783002/diff/20001/net/url_request/url_request_http_job_unittest.cc#newcode1010 net/url_request/url_request_http_job_unittest.cc:1010: class URLRequestHttpJobInternalRedirectTest : public ::testing::Test { Remove this.
4 years, 6 months ago (2016-06-17 16:40:30 UTC) #9
Mike West
https://codereview.chromium.org/2079783002/diff/20001/net/url_request/url_request_http_job_unittest.cc File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2079783002/diff/20001/net/url_request/url_request_http_job_unittest.cc#newcode1010 net/url_request/url_request_http_job_unittest.cc:1010: class URLRequestHttpJobInternalRedirectTest : public ::testing::Test { On 2016/06/17 at ...
4 years, 6 months ago (2016-06-17 19:44:34 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079783002/40001
4 years, 6 months ago (2016-06-17 19:45:07 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/23011)
4 years, 6 months ago (2016-06-17 20:29:06 UTC) #15
Mike West
https://codereview.chromium.org/2079783002/diff/80001/net/url_request/url_request_http_job_unittest.cc File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2079783002/diff/80001/net/url_request/url_request_http_job_unittest.cc#newcode653 net/url_request/url_request_http_job_unittest.cc:653: #if !defined(OS_IOS) mmenke@: WebSockets aren't enabled on iOS. I'm ...
4 years, 6 months ago (2016-06-20 13:36:42 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079783002/80001
4 years, 6 months ago (2016-06-20 13:37:04 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:80001)
4 years, 6 months ago (2016-06-20 14:17:23 UTC) #21
commit-bot: I haz the power
4 years, 6 months ago (2016-06-20 14:19:38 UTC) #23
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/549a5c63cc0be42bae1669abddd6ade78221f477
Cr-Commit-Position: refs/heads/master@{#400674}

Powered by Google App Engine
This is Rietveld 408576698