Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 690903002: Remove timing limitation of SetOption invocation for PPAPI sockets. (Closed)

Created:
4 years ago by hidehiko
Modified:
3 years, 11 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, jam, raymes+watch_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, mmenke, ihf+watch_chromium.org, Nike, wtc, Ryan Hamilton, jar (doing other things), Ryan Sleevi, Sergey Ulanov, rvargas (doing something else), Scott Hess - ex-Googler
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Remove timing limitation of SetOption invocation for PPAPI sockets. Currently PPAPI has timing limitation for sockets' SetOption. NODELAY, and BROADCAST need to be before Connect() or Bind(), while RCVBUF_SIZE and SNFBUF_SIZE need to be after it. This CL removes such a limitation. Along with the change, pepper_udp_socket_message_filter starts to use UDPSocket instead of UDPServerSocket, so that the implementation direction gets closer to TCP message filter a little bit. BUG=425563, 420697 TEST=Ran trybots. Committed: https://crrev.com/fd305c12b3fe8b4d2496f67aed089ff703f19b6d Cr-Commit-Position: refs/heads/master@{#307867}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : #

Total comments: 25

Patch Set 5 : Rebase #

Patch Set 6 : #

Total comments: 6

Patch Set 7 : Rebase #

Patch Set 8 : #

Patch Set 9 : Rebase #

Patch Set 10 : #

Total comments: 8

Patch Set 11 : #

Total comments: 2

Patch Set 12 : #

Total comments: 4

Patch Set 13 : Rebase and fixed implementation. #

Total comments: 22

Patch Set 14 : #

Total comments: 2

Patch Set 15 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+757 lines, -140 lines) Patch
M content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +14 lines, -0 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +82 lines, -30 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +16 lines, -4 lines 0 comments Download
M content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +114 lines, -48 lines 0 comments Download
M ppapi/api/ppb_tcp_socket.idl View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +31 lines, -4 lines 0 comments Download
M ppapi/api/ppb_udp_socket.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +30 lines, -6 lines 0 comments Download
M ppapi/c/ppb_tcp_socket.h View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +46 lines, -7 lines 0 comments Download
M ppapi/c/ppb_udp_socket.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +37 lines, -7 lines 0 comments Download
M ppapi/cpp/tcp_socket.cc View 1 2 3 4 5 6 7 8 9 11 chunks +53 lines, -3 lines 0 comments Download
M ppapi/cpp/udp_socket.cc View 1 2 3 4 5 6 7 8 9 7 chunks +35 lines, -2 lines 0 comments Download
M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +150 lines, -0 lines 0 comments Download
M ppapi/proxy/tcp_socket_private_resource.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M ppapi/proxy/tcp_socket_resource.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M ppapi/proxy/tcp_socket_resource.cc View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -1 line 0 comments Download
M ppapi/proxy/tcp_socket_resource_base.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/proxy/tcp_socket_resource_base.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -2 lines 0 comments Download
M ppapi/proxy/udp_socket_private_resource.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M ppapi/proxy/udp_socket_resource.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M ppapi/proxy/udp_socket_resource.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +12 lines, -1 line 0 comments Download
M ppapi/proxy/udp_socket_resource_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -0 lines 0 comments Download
M ppapi/proxy/udp_socket_resource_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +11 lines, -2 lines 0 comments Download
M ppapi/tests/test_tcp_socket.cc View 3 chunks +4 lines, -5 lines 0 comments Download
M ppapi/tests/test_udp_socket.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -7 lines 0 comments Download
M ppapi/thunk/interfaces_ppb_public_stable.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M ppapi/thunk/ppb_tcp_socket_api.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M ppapi/thunk/ppb_tcp_socket_thunk.cc View 1 2 3 4 5 6 7 8 9 4 chunks +33 lines, -1 line 0 comments Download
M ppapi/thunk/ppb_udp_socket_api.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M ppapi/thunk/ppb_udp_socket_thunk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +37 lines, -8 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 71 (17 generated)
hidehiko
Dave, could you review this CL from PPAPI's point of view? Matt, could you review ...
4 years ago (2014-10-30 09:10:56 UTC) #2
mmenke
I'm not terribly familiar with the UDP code. Maybe someone who's been working on QUIC, ...
4 years ago (2014-10-30 14:40:38 UTC) #3
hidehiko
On 2014/10/30 14:40:38, mmenke wrote: > I'm not terribly familiar with the UDP code. Maybe ...
4 years ago (2014-10-30 16:38:41 UTC) #4
dmichael (off chromium)
FYI, bbudge is going to look, since he's also working on the socket APIs right ...
4 years ago (2014-10-30 17:59:25 UTC) #6
mmenke
Just FYI, moving myself from the reviewer to the CC list, to clean up my ...
4 years ago (2014-10-31 14:21:26 UTC) #8
hidehiko
Ryan, could you kindly review net/... part of this CL as an OWNER? I'd appreciate ...
4 years ago (2014-10-31 14:23:49 UTC) #10
hidehiko
https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libevent.cc File net/udp/udp_socket_libevent.cc (right): https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libevent.cc#newcode323 net/udp/udp_socket_libevent.cc:323: return OK; On 2014/10/30 14:40:38, mmenke wrote: > Sockets ...
4 years ago (2014-11-03 13:34:37 UTC) #11
hidehiko
+bbudge@. (I should have noticed he's not in the R= list...) Could you take a ...
4 years ago (2014-11-04 17:52:31 UTC) #13
Ryan Hamilton
Sorry to redirect you again. I'm going to actually hand this off to jar who ...
4 years ago (2014-11-04 20:08:20 UTC) #15
jar (doing other things)
Can you better motivate why this is a valuable change, rather than merely correcting the ...
4 years ago (2014-11-04 22:51:09 UTC) #16
jar (doing other things)
https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/690903002/diff/80001/net/udp/udp_socket_win.cc#newcode503 net/udp/udp_socket_win.cc:503: } On 2014/11/04 22:51:09, jar wrote: > Note that ...
4 years ago (2014-11-05 01:18:31 UTC) #17
hidehiko
Thank you for review! PATL. The main goal of this CL is fixing crbug.com/425563. Could ...
4 years ago (2014-11-05 12:48:30 UTC) #19
bbudge
Some nits, and a general comment: I'm worried about this behavior change. I don't think ...
4 years ago (2014-11-06 01:27:55 UTC) #20
hidehiko
Thank you for review. PTAL. As for browser_tests, which platform do you mean, Bill? It ...
4 years ago (2014-11-06 14:13:22 UTC) #22
jar (doing other things)
I too continue to be nervous about changing semantics, and vastly prefer avoiding such a ...
4 years ago (2014-11-06 19:01:36 UTC) #24
dmichael (off chromium)
On 2014/11/06 14:13:22, hidehiko wrote: > Thank you for review. PTAL. > > As for ...
4 years ago (2014-11-06 19:27:03 UTC) #25
hidehiko
jar@, which layer do you worry about? For PPAPI? or for net/... implementation? For PPAPI, ...
4 years ago (2014-11-06 19:45:21 UTC) #26
dmichael (off chromium)
On Thu, Nov 6, 2014 at 12:45 PM, <hidehiko@chromium.org> wrote: > jar@, which layer do ...
4 years ago (2014-11-06 21:16:11 UTC) #27
hidehiko
Thank you for advice, Dave. Versionized. PTAL. Added jar@ to R= again, for histograms.xml. Best ...
4 years ago (2014-11-07 12:27:57 UTC) #30
jar (doing other things)
Added asvitkine for (tiny/good looking) histograms.xml review, so as to avoid confusion with overall net/* ...
4 years ago (2014-11-07 17:41:18 UTC) #33
bbudge
Thanks for versioning the API. This is almost there, just a few comments. https://codereview.chromium.org/690903002/diff/240001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h File ...
4 years ago (2014-11-07 17:50:39 UTC) #34
Alexei Svitkine (slow)
histograms.xml lgtm
4 years ago (2014-11-07 20:10:29 UTC) #35
hidehiko
Thank you for review. PTAL. https://codereview.chromium.org/690903002/diff/240001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h (right): https://codereview.chromium.org/690903002/diff/240001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h#newcode195 content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:195: // each option is ...
4 years ago (2014-11-07 21:13:55 UTC) #36
bbudge
LGTM thanks for doing this.
4 years ago (2014-11-07 21:35:18 UTC) #37
bbudge
Whoops, 1 comment, still LGTM https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_resource_base.h File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/690903002/diff/260001/ppapi/proxy/udp_socket_resource_base.h#newcode113 ppapi/proxy/udp_socket_resource_base.h:113: // the timing on ...
4 years ago (2014-11-07 21:39:14 UTC) #38
hidehiko
Thank you for review, Bill, Alexei. Ricardo, Scott, could you review as net OWNER? M40 ...
4 years ago (2014-11-07 22:12:21 UTC) #39
rvargas (doing something else)
I made a first pass (very superficial). Regarding the branch, a change like this should ...
4 years ago (2014-11-08 02:17:20 UTC) #40
hidehiko
Thank you for review and comments. So, I looked into SO_REUSEADDR a bit more carefully, ...
4 years ago (2014-11-11 16:48:49 UTC) #41
bbudge
On 2014/11/11 16:48:49, hidehiko wrote: > Thank you for review and comments. > > So, ...
4 years ago (2014-11-11 18:09:33 UTC) #42
rvargas (doing something else)
On 2014/11/11 16:48:49, hidehiko wrote: > Thank you for review and comments. > > So, ...
4 years ago (2014-11-12 01:33:13 UTC) #43
rvargas (doing something else)
+ Ryan (^^)
4 years ago (2014-11-12 01:34:29 UTC) #44
hidehiko
Thank you. So, we are on the same page about REUSEADDR, TCP_NODELAY, and buf sizes. ...
4 years ago (2014-11-12 02:20:02 UTC) #45
hidehiko
Hmm, Ryan seems OOO for a while. Ricardo, is there anyone else we can ask? ...
4 years ago (2014-11-12 02:43:10 UTC) #46
rvargas (doing something else)
On 2014/11/12 02:43:10, hidehiko wrote: > Hmm, Ryan seems OOO for a while. > Ricardo, ...
4 years ago (2014-11-12 03:21:21 UTC) #47
hidehiko
On 2014/11/12 03:21:21, rvargas wrote: > On 2014/11/12 02:43:10, hidehiko wrote: > > Hmm, Ryan ...
4 years ago (2014-11-12 04:01:24 UTC) #48
Scott Hess - ex-Googler
On Tue, Nov 11, 2014 at 6:20 PM, <hidehiko@chromium.org> wrote: > > Let's assume s1 ...
4 years ago (2014-11-12 06:02:35 UTC) #49
rvargas (doing something else)
On 2014/11/12 04:01:24, hidehiko wrote: > On 2014/11/12 03:21:21, rvargas wrote: > > On 2014/11/12 ...
4 years ago (2014-11-12 19:01:58 UTC) #50
rvargas (doing something else)
On 2014/11/12 06:02:35, Scott Hess - On Leave wrote: > On Tue, Nov 11, 2014 ...
4 years ago (2014-11-12 19:11:24 UTC) #51
hidehiko
On 2014/11/12 19:01:58, rvargas wrote: > On 2014/11/12 04:01:24, hidehiko wrote: > > On 2014/11/12 ...
4 years ago (2014-11-13 17:55:18 UTC) #52
rvargas (doing something else)
On 2014/11/13 17:55:18, hidehiko wrote: > On 2014/11/12 19:01:58, rvargas wrote: > > On 2014/11/12 ...
4 years ago (2014-11-13 20:53:21 UTC) #53
hidehiko
On 2014/11/13 20:53:21, rvargas wrote: > On 2014/11/13 17:55:18, hidehiko wrote: > > On 2014/11/12 ...
4 years ago (2014-11-14 01:44:50 UTC) #54
Ryan Sleevi
I'm sorry, it's still taking time for me to catch up. However, I want to ...
4 years ago (2014-11-14 02:00:17 UTC) #56
rvargas (doing something else)
On 2014/11/14 01:44:50, hidehiko wrote: I'm cleaning a little this thread to make the response ...
4 years ago (2014-11-14 02:39:26 UTC) #57
hidehiko
On 2014/11/14 02:39:26, rvargas wrote: > On 2014/11/14 01:44:50, hidehiko wrote: > > I'm cleaning ...
3 years, 12 months ago (2014-11-19 05:57:24 UTC) #58
rvargas (doing something else)
On 2014/11/19 05:57:24, hidehiko wrote: > On 2014/11/14 02:39:26, rvargas wrote: > > On 2014/11/14 ...
3 years, 12 months ago (2014-11-19 21:14:04 UTC) #59
hidehiko
Moved net/ reviewers from R= to CC=, because the net/ part are submitted separately. Bill, ...
3 years, 11 months ago (2014-12-08 12:33:24 UTC) #62
bbudge
https://codereview.chromium.org/690903002/diff/300001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc#newcode66 content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:66: socket_options_(SOCKET_OPTION_NODELAY), Even though not strictly necessary in this case, ...
3 years, 11 months ago (2014-12-09 18:37:38 UTC) #63
hidehiko
Thank you for review! PTAL. https://codereview.chromium.org/690903002/diff/300001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc (right): https://codereview.chromium.org/690903002/diff/300001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc#newcode66 content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:66: socket_options_(SOCKET_OPTION_NODELAY), On 2014/12/09 18:37:37, ...
3 years, 11 months ago (2014-12-09 19:51:42 UTC) #64
hidehiko
On 2014/12/09 19:51:42, hidehiko wrote: > Thank you for review! PTAL. > > https://codereview.chromium.org/690903002/diff/300001/content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc > ...
3 years, 11 months ago (2014-12-11 02:35:09 UTC) #65
bbudge
LGTM w comment https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_resource_base.cc File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_resource_base.cc#newcode80 ppapi/proxy/udp_socket_resource_base.cc:80: // successful Bind() call, so it's ...
3 years, 11 months ago (2014-12-11 02:49:06 UTC) #66
hidehiko
Thank you for review. Submitting. https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_resource_base.cc File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/690903002/diff/320001/ppapi/proxy/udp_socket_resource_base.cc#newcode80 ppapi/proxy/udp_socket_resource_base.cc:80: // successful Bind() call, ...
3 years, 11 months ago (2014-12-11 05:02:33 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690903002/340001
3 years, 11 months ago (2014-12-11 05:03:25 UTC) #69
commit-bot: I haz the power
Committed patchset #15 (id:340001)
3 years, 11 months ago (2014-12-11 06:01:58 UTC) #70
commit-bot: I haz the power
3 years, 11 months ago (2014-12-11 06:02:53 UTC) #71
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/fd305c12b3fe8b4d2496f67aed089ff703f19b6d
Cr-Commit-Position: refs/heads/master@{#307867}

Powered by Google App Engine
This is Rietveld 408576698