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

Issue 678003002: Correct manual proxy selection for WebSockets. (Closed)

Created:
6 years, 1 month ago by Adam Rice
Modified:
6 years, 1 month ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Correct manual proxy selection for WebSockets. When an HTTPS and/or HTTP proxy was manually configured, WebSocket connections were mistakenly going direct to the origin server. According to RFC6455, WebSocket connections should prefer SOCKS proxies, then HTTPS, then HTTP proxies in that order. Fix the behaviour when proxies are manually selected to match. BUG=426736 TEST=net_unittests, manual Committed: https://crrev.com/bec9560de247f82410abe2ed1741bccdb9ee7dc5 Cr-Commit-Position: refs/heads/master@{#302037}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Don't look for ws schemes at parse time. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -2 lines) Patch
M net/proxy/proxy_config.h View 1 1 chunk +8 lines, -2 lines 0 comments Download
M net/proxy/proxy_config.cc View 1 2 chunks +13 lines, -0 lines 0 comments Download
M net/proxy/proxy_config_unittest.cc View 1 chunk +71 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (3 generated)
Adam Rice
6 years, 1 month ago (2014-10-27 14:36:02 UTC) #2
tyoshino (SeeGerritForStatus)
unittest looks good https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc File net/proxy/proxy_config.cc (right): https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc#newcode133 net/proxy/proxy_config.cc:133: if (entry) { before this change, ...
6 years, 1 month ago (2014-10-27 15:28:30 UTC) #3
Adam Rice
https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc File net/proxy/proxy_config.cc (right): https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc#newcode133 net/proxy/proxy_config.cc:133: if (entry) { On 2014/10/27 15:28:29, tyoshino wrote: > ...
6 years, 1 month ago (2014-10-28 02:22:23 UTC) #4
Adam Rice
On 2014/10/28 02:22:23, Adam Rice wrote: > https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc > File net/proxy/proxy_config.cc (right): > > https://codereview.chromium.org/678003002/diff/1/net/proxy/proxy_config.cc#newcode133 ...
6 years, 1 month ago (2014-10-29 05:10:28 UTC) #5
tyoshino (SeeGerritForStatus)
sorry. lgtm
6 years, 1 month ago (2014-10-29 05:29:48 UTC) #6
Adam Rice
+eroman for net/ OWNERS.
6 years, 1 month ago (2014-10-29 06:29:58 UTC) #8
eroman
lgtm
6 years, 1 month ago (2014-10-29 20:45:49 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/678003002/20001
6 years, 1 month ago (2014-10-30 04:24:33 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001)
6 years, 1 month ago (2014-10-30 05:05:17 UTC) #12
commit-bot: I haz the power
6 years, 1 month ago (2014-10-30 05:06:04 UTC) #13
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/bec9560de247f82410abe2ed1741bccdb9ee7dc5
Cr-Commit-Position: refs/heads/master@{#302037}

Powered by Google App Engine
This is Rietveld 408576698