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

Issue 1130073004: Increase WebSocket limit to 255 (Closed)

Created:
5 years, 7 months ago by dkelson
Modified:
5 years, 1 month ago
Reviewers:
Adam Rice, eroman, mmenke
CC:
cbentzel+watch_chromium.org, chromium-reviews, Yuta Kitamura
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Increase WebSocket limit to 255 As the existing comments in code indicate the current limit of 30 WebSockets was chosen arbitrarily. The limit of 30 WebSockets is impacting real world web sites, for an example, See http://crbug.com/486800 As a reference Firefox has a 200 WebSocket limit. R=asanka@chromium.org BUG=486800 Committed: https://crrev.com/64cb80d3dab248fd15972700a71dc2fb1130a809 Cr-Commit-Position: refs/heads/master@{#359051}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Updated patch to address comments #

Patch Set 3 : New patch that clamps max sockets to proxy limits if needed #

Total comments: 1

Patch Set 4 : Updated limit to 255 as requested #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -20 lines) Patch
M AUTHORS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M net/socket/client_socket_pool_manager.cc View 1 2 3 1 chunk +4 lines, -6 lines 1 comment Download
M net/socket/client_socket_pool_manager_impl.cc View 1 2 8 chunks +27 lines, -14 lines 0 comments Download

Messages

Total messages: 53 (13 generated)
dkelson
5 years, 7 months ago (2015-05-11 17:43:22 UTC) #1
dkelson
Please review one line change.
5 years, 7 months ago (2015-05-11 17:47:43 UTC) #2
asanka
-> mmenke
5 years, 7 months ago (2015-05-11 22:02:13 UTC) #5
mmenke
[+ricea]
5 years, 7 months ago (2015-05-11 22:11:33 UTC) #8
mmenke
https://codereview.chromium.org/1130073004/diff/1/net/socket/client_socket_pool_manager.cc File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/1/net/socket/client_socket_pool_manager.cc#newcode42 net/socket/client_socket_pool_manager.cc:42: // for such use, thus we use a larger ...
5 years, 7 months ago (2015-05-11 22:14:02 UTC) #9
Adam Rice
Sorry for the delayed response; I am on holiday. The short answer for why we ...
5 years, 7 months ago (2015-05-19 22:01:11 UTC) #11
Adam Rice
I think it can be made to work if you also change client_socket_pool_manager_impl.cc to respect ...
5 years, 7 months ago (2015-05-26 02:41:18 UTC) #13
Adam Rice
lgtm mmenke, what do you think?
5 years, 3 months ago (2015-08-25 15:52:57 UTC) #14
Adam Rice
https://codereview.chromium.org/1130073004/diff/60001/net/socket/client_socket_pool_manager.cc File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/60001/net/socket/client_socket_pool_manager.cc#newcode45 net/socket/client_socket_pool_manager.cc:45: 200 // WEBSOCKET_SOCKET_POOL Can you change this to 255? ...
5 years, 3 months ago (2015-08-27 08:28:54 UTC) #15
Adam Rice
Actually adding mmenke for real this time.
5 years, 3 months ago (2015-09-01 01:38:45 UTC) #17
mmenke
Just for the record, I confirmed Dax has indeed signed the CLA. I'm concerned about ...
5 years, 3 months ago (2015-09-01 15:22:13 UTC) #18
dkelson
On 2015/09/01 15:22:13, mmenke wrote: > > Does the websocket spec contain any guidelines for ...
5 years, 3 months ago (2015-09-01 16:25:12 UTC) #19
mmenke
On 2015/09/01 16:25:12, dkelson wrote: > On 2015/09/01 15:22:13, mmenke wrote: > > > > ...
5 years, 3 months ago (2015-09-01 16:35:01 UTC) #20
dkelson
On 2015/09/01 16:35:01, mmenke wrote: > > Does Firefox also use a global limit of ...
5 years, 3 months ago (2015-09-01 22:16:54 UTC) #21
Adam Rice
On 2015/09/01 15:22:13, mmenke wrote: > I'm concerned about this change. When using a proxy, ...
5 years, 3 months ago (2015-09-02 06:41:23 UTC) #22
dkelson
On 2015/09/02 06:41:23, Adam Rice wrote: > However, this CL doesn't make the situation any ...
5 years, 3 months ago (2015-09-02 16:35:46 UTC) #23
mmenke
On 2015/09/02 06:41:23, Adam Rice wrote: > On 2015/09/01 15:22:13, mmenke wrote: > > I'm ...
5 years, 3 months ago (2015-09-02 16:45:49 UTC) #24
mmenke
On 2015/09/02 16:35:46, dkelson wrote: > On 2015/09/02 06:41:23, Adam Rice wrote: > > However, ...
5 years, 3 months ago (2015-09-02 16:47:35 UTC) #25
mmenke
On 2015/09/02 16:47:35, mmenke wrote: > On 2015/09/02 16:35:46, dkelson wrote: > > On 2015/09/02 ...
5 years, 3 months ago (2015-09-02 16:48:18 UTC) #26
Adam Rice
On 2015/09/02 at 16:48:18, mmenke wrote: > On 2015/09/02 16:47:35, mmenke wrote: > > On ...
5 years, 2 months ago (2015-10-09 09:36:30 UTC) #27
mmenke
On 2015/10/09 09:36:30, Adam Rice wrote: > On 2015/09/02 at 16:48:18, mmenke wrote: > > ...
5 years, 2 months ago (2015-10-09 15:18:59 UTC) #28
Adam Rice
On 2015/10/09 15:18:59, mmenke wrote: > I'm not following - what weird interactions? Seems like ...
5 years, 1 month ago (2015-10-28 15:45:13 UTC) #29
dkelson
On Wed, Oct 28, 2015 at 9:45 AM, <ricea@chromium.org> wrote: > > I'm not sure. ...
5 years, 1 month ago (2015-10-28 16:07:27 UTC) #30
mmenke
[+eroman]: Do you have any opinion on this, as it relates to proxies? In particular, ...
5 years, 1 month ago (2015-10-28 16:22:01 UTC) #32
Adam Rice
On 2015/10/28 16:22:01, mmenke wrote: > [+eroman]: Do you have any opinion on this, as ...
5 years, 1 month ago (2015-10-28 17:41:55 UTC) #33
mmenke
On 2015/10/28 17:41:55, Adam Rice wrote: > On 2015/10/28 16:22:01, mmenke wrote: > > [+eroman]: ...
5 years, 1 month ago (2015-10-28 17:46:17 UTC) #34
eroman
Can you add some tests? Otherwise the code change itself LGTM. This is a large ...
5 years, 1 month ago (2015-10-28 19:05:18 UTC) #35
eroman
https://codereview.chromium.org/1130073004/diff/80001/net/socket/client_socket_pool_manager.cc File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/80001/net/socket/client_socket_pool_manager.cc#newcode41 net/socket/client_socket_pool_manager.cc:41: // than normal other connections. Use a limit of ...
5 years, 1 month ago (2015-10-28 19:06:05 UTC) #36
dkelson
On Wed, Oct 28, 2015 at 1:06 PM, <eroman@chromium.org> wrote: > The CL description says ...
5 years, 1 month ago (2015-10-28 19:15:25 UTC) #37
Adam Rice
On 2015/10/28 19:05:18, eroman wrote: > Can you add some tests? > > Otherwise the ...
5 years, 1 month ago (2015-10-28 19:16:03 UTC) #38
eroman
The CL descriptions reads: "Increase WebSocket limit to 200 (matches Firefox)" However that is clearly ...
5 years, 1 month ago (2015-10-28 19:24:49 UTC) #39
dkelson
On 2015/10/28 19:24:49, eroman wrote: > The CL descriptions reads: Sorry, misunderstood. The CL description ...
5 years, 1 month ago (2015-10-28 19:35:30 UTC) #41
dkelson
So is everything good to go?
5 years, 1 month ago (2015-11-10 22:51:28 UTC) #42
eroman
yes, still LGTM
5 years, 1 month ago (2015-11-10 22:57:47 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130073004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1130073004/80001
5 years, 1 month ago (2015-11-10 23:01:07 UTC) #45
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/128999)
5 years, 1 month ago (2015-11-11 00:35:24 UTC) #47
Adam Rice
dkelson: I think you can click the Commit checkbox.
5 years, 1 month ago (2015-11-11 00:46:59 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130073004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1130073004/80001
5 years, 1 month ago (2015-11-11 03:29:08 UTC) #51
commit-bot: I haz the power
Committed patchset #4 (id:80001)
5 years, 1 month ago (2015-11-11 04:30:54 UTC) #52
commit-bot: I haz the power
5 years, 1 month ago (2015-11-11 04:31:45 UTC) #53
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/64cb80d3dab248fd15972700a71dc2fb1130a809
Cr-Commit-Position: refs/heads/master@{#359051}

Powered by Google App Engine
This is Rietveld 408576698