Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
704133005:
Pepper: Add support for multicast in PPB_UDPSocket API (Closed)
|
Created:
6 years, 1 month ago by etrunko Modified:
5 years, 9 months ago CC:
binji+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, ihf+watch_chromium.org, jam, nfullagar1, noelallen1, piman+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, tzik, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for multicast in PPB_UDPSocket API
This change introduces two new values to be used with SetOption(), to
set up the behavior of the multicast packets:
- PP_UDPSOCKET_OPTION_MULTICAST_LOOP (Boolean)
Whether packets sent from the host to the multicast group will be
looped back to the host or not.
- PP_UDPSOCKET_OPTION_MULTICAST_TTL (Integer)
Sets time-to-live of multicast packets sent to the multicast group.
It also introduces two new functions JoinGroup() and LeaveGroup(), both
of them expecting the address of the multicast group as specified by
the PPB_NetAddress API. These two functions should only be called after
the socket is bound.
This CL adds specific test cases to browser_tests in order to exercise
the different versions of the SetOption function. For this reason, the
PPAPI UDPSocket tests were split into multiple tests, instead of a
single one that enclosed all of them. This way we get clearer
information about the results of each test.
BUG=430939
TEST=browser_tests
NOPRESUBMIT=true
Signed-off-by: Eduardo Lima (Etrunko) <eduardo.lima@intel.com>;
Committed: https://crrev.com/8171d46410b1ea937b0ae59577b9b36ed151149a
Cr-Commit-Position: refs/heads/master@{#320504}
Patch Set 1 #Patch Set 2 : Rebase, initial unit tests #
Total comments: 10
Patch Set 3 : Rebase, version new enum values #
Total comments: 1
Patch Set 4 : Rebase, remove IF option, JoinGroup and LeaveGroup as functions #
Total comments: 16
Patch Set 5 : Fixes as suggested in #20, tests for SetOption_1_0 and SetOption_1_1 #
Total comments: 2
Patch Set 6 : Restore mistakenly removed lines #
Total comments: 11
Patch Set 7 : Fix unit tests, thunk code auto generated #
Total comments: 8
Patch Set 8 : Rebase #
Total comments: 17
Patch Set 9 : M43 #
Total comments: 1
Patch Set 10 : fix nit #
Total comments: 13
Patch Set 11 : Replace socket_.get() tests with socket_ #Patch Set 12 : Check for dev channel and permissions for Multicast membership #
Total comments: 7
Patch Set 13 : Properly check for permissions, restore check for API version 1.2 so all trybots are happy #
Total comments: 16
Patch Set 14 : C++ wrappers #Patch Set 15 : Use namespace for GetAnyAddress call in order to fix windows build #
Total comments: 7
Patch Set 16 : Add missing declaration to interfaces_ppb_public_dev_channel.h, bots are happy now #
Total comments: 2
Patch Set 17 : Fix check for permissions to use multicast API #Patch Set 18 : Rebase #Patch Set 19 : histograms.xml #Patch Set 20 : Add debug to tests #Patch Set 21 : debug in pepper_udp_socket_message_filter.cc #Patch Set 22 : more detailed debug #Patch Set 23 : debug in upd_socket_libevent.cc #Patch Set 24 : debug in SetMulticastOptions() #
Total comments: 1
Patch Set 25 : Fix typo: SetMulticastInterface -> SetMulticastTimeToLive #Messages
Total messages: 84 (12 generated)
eduardo.lima@intel.com changed reviewers: + ygorshenin@chromium.org, yzshen@chromium.org
PTAL
yzshen@chromium.org changed reviewers: + bbudge@chromium.org - yzshen@chromium.org
Removed myself from the reviewer list and added Bill. Hi, Bill. Would you please take a look at this change? Thanks!
A couple of high level comments: 1) We'll probably have to version the API to make it clear when these options can be used, rather than fail and leave the caller wondering. Here's an enum value with versioning for example: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... 2) There is another CL under review that relaxes the restriction on when options can be set. It also requires a new version, so it would be nice if we could combine these two changes in one new version if possible. I'm not sure what the status of this other CL is right now: https://codereview.chromium.org/690903002/
https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:221: else if (family == PP_NETADDRESS_FAMILY_IPV6) { nit: joint lines #220 and #221. https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:228: else nit: join lines #228 and #227 and wrap return statement into curly braces. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... File ppapi/tests/test_udp_socket_private.cc (right): https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:280: nit: remove the blank line. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:282: std::string TestUDPSocketPrivate::TestMulticast() { nit: insert a blank line before #282. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:283: pp::UDPSocketPrivate server1(instance_), server2(instance_); Could you please clarify why these sockets are called server1 and server2 and why do you need two sockets at all?
https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:221: else if (family == PP_NETADDRESS_FAMILY_IPV6) { On 2014/11/14 08:03:43, ygorshenin1 wrote: > nit: joint lines #220 and #221. Done. https://codereview.chromium.org/704133005/diff/20001/content/browser/renderer... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:228: else On 2014/11/14 08:03:43, ygorshenin1 wrote: > nit: join lines #228 and #227 and wrap return statement into curly braces. Done. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... File ppapi/tests/test_udp_socket_private.cc (right): https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:280: On 2014/11/14 08:03:43, ygorshenin1 wrote: > nit: remove the blank line. Done. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:282: std::string TestUDPSocketPrivate::TestMulticast() { On 2014/11/14 08:03:43, ygorshenin1 wrote: > nit: insert a blank line before #282. Done. https://codereview.chromium.org/704133005/diff/20001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket_private.cc:283: pp::UDPSocketPrivate server1(instance_), server2(instance_); On 2014/11/14 08:03:43, ygorshenin1 wrote: > Could you please clarify why these sockets are called server1 and server2 and > why do you need two sockets at all? Oh, sorry, i submitted this test unfinished. It was just a stub.
On 2014/11/10 22:20:15, bbudge wrote: > A couple of high level comments: > > 1) We'll probably have to version the API to make it clear when these options > can be used, rather than fail and leave the caller wondering. Here's an enum > value with versioning for example: > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > Ok, working on it. > 2) There is another CL under review that relaxes the restriction on when options > can be set. It also requires a new version, so it would be nice if we could > combine these two changes in one new version if possible. I'm not sure what the > status of this other CL is right now: > > https://codereview.chromium.org/690903002/ So I will take that patch and base my work on top of it. Does CQ-DEPEND works for chromium too? I had used it for Chromium OS before.
On 2014/11/17 17:50:06, etrunko wrote: > On 2014/11/10 22:20:15, bbudge wrote: > > A couple of high level comments: > > > > 1) We'll probably have to version the API to make it clear when these options > > can be used, rather than fail and leave the caller wondering. Here's an enum > > value with versioning for example: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > Ok, working on it. > > > 2) There is another CL under review that relaxes the restriction on when > options > > can be set. It also requires a new version, so it would be nice if we could > > combine these two changes in one new version if possible. I'm not sure what > the > > status of this other CL is right now: > > > > https://codereview.chromium.org/690903002/ > > So I will take that patch and base my work on top of it. Does CQ-DEPEND works > for chromium too? I had used it for Chromium OS before. Not sure that CQ-DEPEND works for rietveld :(
On 2014/11/17 17:50:06, etrunko wrote: > On 2014/11/10 22:20:15, bbudge wrote: > > A couple of high level comments: > > > > 1) We'll probably have to version the API to make it clear when these options > > can be used, rather than fail and leave the caller wondering. Here's an enum > > value with versioning for example: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > Ok, working on it. > I am back on this issue after getting sidetracked. I versioned the enum but got an error, because the old version of the API expects a PP_UDPSocket_Option_1_0, but that enum is not generated. Should I do it manually? If so, how can I avoid redeclaring the values? > > 2) There is another CL under review that relaxes the restriction on when > options > > can be set. It also requires a new version, so it would be nice if we could > > combine these two changes in one new version if possible. I'm not sure what > the > > status of this other CL is right now: > > > > https://codereview.chromium.org/690903002/ > > So I will take that patch and base my work on top of it. Does CQ-DEPEND works > for chromium too? I had used it for Chromium OS before. After a few weeks, that issue is still on the same position; it seems there are some difficulties in finding an agreement there. How can we proceed with this one without depending on it?
On 2014/12/03 13:48:14, etrunko wrote: > On 2014/11/17 17:50:06, etrunko wrote: > > On 2014/11/10 22:20:15, bbudge wrote: > > > A couple of high level comments: > > > > > > 1) We'll probably have to version the API to make it clear when these > options > > > can be used, rather than fail and leave the caller wondering. Here's an enum > > > value with versioning for example: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > > > > Ok, working on it. > > > > I am back on this issue after getting sidetracked. I versioned the enum but got > an error, because the old version of the API expects a PP_UDPSocket_Option_1_0, > but that enum is not generated. Should I do it manually? If so, how can I avoid > redeclaring the values? > > > > > 2) There is another CL under review that relaxes the restriction on when > > options > > > can be set. It also requires a new version, so it would be nice if we could > > > combine these two changes in one new version if possible. I'm not sure what > > the > > > status of this other CL is right now: > > > > > > https://codereview.chromium.org/690903002/ > > > > So I will take that patch and base my work on top of it. Does CQ-DEPEND works > > for chromium too? I had used it for Chromium OS before. > > > After a few weeks, that issue is still on the same position; it seems there are > some difficulties in finding an agreement there. How can we proceed with this > one without depending on it? That CL finally landed. Can you rebase the patch?
On 2014/12/11 17:22:35, bbudge wrote: > > That CL finally landed. Can you rebase the patch? Thanks, I just took a look at it. One question that can be answered even before submitting the new version of the patch is the expected behavior for multicast options. Most 1options only take effect before bind, while Join/Leave will only work after. My suggestion is we keep the options that only take effect before bind restricted (IOW, user will get an error if those are called after bind) and implement the "cache" mechanism only for Join and Leave. WDYT?
On 2014/12/11 18:36:21, etrunko wrote: > On 2014/12/11 17:22:35, bbudge wrote: > > > > That CL finally landed. Can you rebase the patch? > > Thanks, I just took a look at it. One question that can be answered even before > submitting the new version of the patch is the expected behavior for multicast > options. Most 1options only take effect before bind, while Join/Leave will only > work after. > > My suggestion is we keep the options that only take effect before bind > restricted (IOW, user will get an error if those are called after bind) and > implement the "cache" mechanism only for Join and Leave. WDYT? Oh, nevermind. I have just figured how it works. Sorry for the noise
bbudge@ PTAL I uploaded this new version with the changes suggested and also following the new relaxed requirement for bind. Notice that this breaks the build because of undefined reference to PP_UDPSocket_Option_1_0, that will be automatically generated because the new introduced values are versioned. https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h File ppapi/c/ppb_udp_socket.h (right): https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h... ppapi/c/ppb_udp_socket.h:284: PP_UDPSocket_Option_1_0 name, Here is what we get from the generator when the enum is versioned, the build will fail due to undefined PP_UDPSocket_Option_1_0.
On 2014/12/17 17:39:36, etrunko wrote: > bbudge@ PTAL > > I uploaded this new version with the changes suggested and also following the > new relaxed requirement for bind. > > Notice that this breaks the build because of undefined reference to > PP_UDPSocket_Option_1_0, that will be automatically generated because the new > introduced values are versioned. > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h > File ppapi/c/ppb_udp_socket.h (right): > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h... > ppapi/c/ppb_udp_socket.h:284: PP_UDPSocket_Option_1_0 name, > Here is what we get from the generator when the enum is versioned, the build > will fail due to undefined PP_UDPSocket_Option_1_0. I'm not sure why this doesn't work. Here's an example in an existing IDL file: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... At any rate, if the generator does the wrong thing, we should fix it. I think we should add a new version to make it easier to code to the api.
On 2015/02/05 22:36:58, bbudge wrote: > On 2014/12/17 17:39:36, etrunko wrote: > > bbudge@ PTAL > > > > I uploaded this new version with the changes suggested and also following the > > new relaxed requirement for bind. > > > > Notice that this breaks the build because of undefined reference to > > PP_UDPSocket_Option_1_0, that will be automatically generated because the new > > introduced values are versioned. > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h > > File ppapi/c/ppb_udp_socket.h (right): > > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h... > > ppapi/c/ppb_udp_socket.h:284: PP_UDPSocket_Option_1_0 name, > > Here is what we get from the generator when the enum is versioned, the build > > will fail due to undefined PP_UDPSocket_Option_1_0. > > I'm not sure why this doesn't work. Here's an example in an existing IDL file: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > At any rate, if the generator does the wrong thing, we should fix it. I think we > should add a new version to make it easier to code to the api. I see why it works for PPB_FileIO - these are flags and are passed as an int32_t. Perhaps you can define a whole new enum and version that?
On 2015/02/05 22:39:50, bbudge wrote: > On 2015/02/05 22:36:58, bbudge wrote: > > On 2014/12/17 17:39:36, etrunko wrote: > > > bbudge@ PTAL > > > > > > I uploaded this new version with the changes suggested and also following > the > > > new relaxed requirement for bind. > > > > > > Notice that this breaks the build because of undefined reference to > > > PP_UDPSocket_Option_1_0, that will be automatically generated because the > new > > > introduced values are versioned. > > > > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h > > > File ppapi/c/ppb_udp_socket.h (right): > > > > > > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h... > > > ppapi/c/ppb_udp_socket.h:284: PP_UDPSocket_Option_1_0 name, > > > Here is what we get from the generator when the enum is versioned, the build > > > will fail due to undefined PP_UDPSocket_Option_1_0. > > > > I'm not sure why this doesn't work. Here's an example in an existing IDL file: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > At any rate, if the generator does the wrong thing, we should fix it. I think > we > > should add a new version to make it easier to code to the api. > > I see why it works for PPB_FileIO - these are flags and are passed as an > int32_t. Perhaps you can define a whole new enum and version that? OK, after some discussion, I think the best approach is to *NOT* version the enum values. Instead, we'll version the SetOption method. I would like to do a general review on the shape of this CL. We need to consider whether this change would go beyond what is provided in Chrome's sockets API. https://developer.chrome.com/apps/sockets_udp In particular, I wonder whether Join and Leave should be implemented as functions, rather than as SetOption with special values.
On 2015/02/05 23:01:52, bbudge wrote: > On 2015/02/05 22:39:50, bbudge wrote: > > On 2015/02/05 22:36:58, bbudge wrote: > > > On 2014/12/17 17:39:36, etrunko wrote: > > > > bbudge@ PTAL > > > > > > > > I uploaded this new version with the changes suggested and also following > > the > > > > new relaxed requirement for bind. > > > > > > > > Notice that this breaks the build because of undefined reference to > > > > PP_UDPSocket_Option_1_0, that will be automatically generated because the > > new > > > > introduced values are versioned. > > > > > > > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h > > > > File ppapi/c/ppb_udp_socket.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/704133005/diff/40001/ppapi/c/ppb_udp_socket.h... > > > > ppapi/c/ppb_udp_socket.h:284: PP_UDPSocket_Option_1_0 name, > > > > Here is what we get from the generator when the enum is versioned, the > build > > > > will fail due to undefined PP_UDPSocket_Option_1_0. > > > > > > I'm not sure why this doesn't work. Here's an example in an existing IDL > file: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > > > At any rate, if the generator does the wrong thing, we should fix it. I > think > > we > > > should add a new version to make it easier to code to the api. > > > > I see why it works for PPB_FileIO - these are flags and are passed as an > > int32_t. Perhaps you can define a whole new enum and version that? > > OK, after some discussion, I think the best approach is to *NOT* version the > enum values. Instead, we'll version the SetOption method. > > I would like to do a general review on the shape of this CL. We need to consider > whether this change would go beyond what is provided in Chrome's sockets API. > > https://developer.chrome.com/apps/sockets_udp > > In particular, I wonder whether Join and Leave should be implemented as > functions, rather than as SetOption with special values. OK, more high level comments: I think we should use the chrome sockets API as our guide here, rather than the POSIX socket API. This implies that: 1) No PP_UDPSOCKET_OPTION_MULTICAST_IF be exposed. 2) JoinGroup and LeaveGroup should be methods. This is clearer too, since restrictions on when SetOptions can be called were relaxed in the 1.1 version. Also, the version will have to be 1.2 now, and the milestone M42.
On 2015/02/05 23:31:58, bbudge wrote: > > > > > > > > I'm not sure why this doesn't work. Here's an example in an existing IDL > > file: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > > > > > > > At any rate, if the generator does the wrong thing, we should fix it. I > > think > > > we > > > > should add a new version to make it easier to code to the api. > > > > > > I see why it works for PPB_FileIO - these are flags and are passed as an > > > int32_t. Perhaps you can define a whole new enum and version that? > > > > OK, after some discussion, I think the best approach is to *NOT* version the > > enum values. Instead, we'll version the SetOption method. > > I agree that generator is not doing the right thing, but I guess there is no way to fix this easily. With a new enum, the values will be redefined, and the compiler will bail out. I will follow up with versioning the SetOption method. > > I would like to do a general review on the shape of this CL. We need to > consider > > whether this change would go beyond what is provided in Chrome's sockets API. > > > > https://developer.chrome.com/apps/sockets_udp > > > > In particular, I wonder whether Join and Leave should be implemented as > > functions, rather than as SetOption with special values. > > OK, more high level comments: > > I think we should use the chrome sockets API as our guide here, rather than the > POSIX socket API. This implies that: > 1) No PP_UDPSOCKET_OPTION_MULTICAST_IF be exposed. > 2) JoinGroup and LeaveGroup should be methods. This is clearer too, since > restrictions on when SetOptions can be called were relaxed in the 1.1 version. > > Also, the version will have to be 1.2 now, and the milestone M42. Thanks a lot for the input, I will adjust the CL to this new requirements.
https://codereview.chromium.org/704133005/diff/60001/ppapi/api/ppb_udp_socket... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/api/ppb_udp_socket... ppapi/api/ppb_udp_socket.idl:13: M42 = 1.2 This should be 'dev' channel until we've reviewed the API to make sure we can commit to supporting it. i.e. [channel=dev] M42 = 1.2 https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource.cc:81: scoped_refptr<TrackedCallback> callback) { Check name parameter here too: if (name > PP_UDPSOCKET_OPTION_RECV_BUFFER_SIZE) return PP_ERROR_BADARGUMENT; https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource.cc:95: true, // Check bind() state. This should be false (1.1 relaxed bind restrictions.) https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:71: // Check if socket is expected to be bound or not according to the option nit: period at end of sentence. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:262: return PP_ERROR_BADARGUMENT; A plugin error isn't quite right here, because the caller is UDPSocketResource, which returns PP_ERROR_BADRESOURCE on error. A DCHECK(group) is more appropriate here. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:267: base::Bind(&UDPSocketResourceBase::OnPluginMsgSetOptionReply, I think at this point we should rename this method. Maybe OnPluginMsgReply or OnPluginMsgGeneralReply ? https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:278: return PP_ERROR_BADARGUMENT; DCHECK https://codereview.chromium.org/704133005/diff/60001/ppapi/tests/test_udp_soc... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket.cc:203: You should verify invalid types and values fail, e.g. TTL < 0 or > 255. It might be nice to test that prior versions can't use these. That's harder though - you'd have to get the C interface for those versions and use that rather than the nicer C++ wrappers.
New version coming soon. https://codereview.chromium.org/704133005/diff/60001/ppapi/api/ppb_udp_socket... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/api/ppb_udp_socket... ppapi/api/ppb_udp_socket.idl:13: M42 = 1.2 On 2015/02/13 00:03:16, bbudge wrote: > This should be 'dev' channel until we've reviewed the API to make sure we can > commit to supporting it. > i.e. > [channel=dev] M42 = 1.2 Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource.cc:81: scoped_refptr<TrackedCallback> callback) { On 2015/02/13 00:03:16, bbudge wrote: > Check name parameter here too: > if (name > PP_UDPSOCKET_OPTION_RECV_BUFFER_SIZE) > return PP_ERROR_BADARGUMENT; Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource.cc:95: true, // Check bind() state. On 2015/02/13 00:03:16, bbudge wrote: > This should be false (1.1 relaxed bind restrictions.) Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:71: // Check if socket is expected to be bound or not according to the option On 2015/02/13 00:03:16, bbudge wrote: > nit: period at end of sentence. Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:262: return PP_ERROR_BADARGUMENT; On 2015/02/13 00:03:16, bbudge wrote: > A plugin error isn't quite right here, because the caller is UDPSocketResource, > which returns PP_ERROR_BADRESOURCE on error. A DCHECK(group) is more appropriate > here. Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:267: base::Bind(&UDPSocketResourceBase::OnPluginMsgSetOptionReply, On 2015/02/13 00:03:16, bbudge wrote: > I think at this point we should rename this method. Maybe > OnPluginMsgReply or OnPluginMsgGeneralReply ? Done. I guess so, by the time I wondered if this method could be reused or if it would be interesting to have dedicated methods for each of JoinGroup/LeaveGroup, but it was basic duplicated code. https://codereview.chromium.org/704133005/diff/60001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:278: return PP_ERROR_BADARGUMENT; On 2015/02/13 00:03:16, bbudge wrote: > DCHECK Done. https://codereview.chromium.org/704133005/diff/60001/ppapi/tests/test_udp_soc... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/60001/ppapi/tests/test_udp_soc... ppapi/tests/test_udp_socket.cc:203: On 2015/02/13 00:03:16, bbudge wrote: > You should verify invalid types and values fail, e.g. TTL < 0 or > 255. > > It might be nice to test that prior versions can't use these. That's harder > though - you'd have to get the C interface for those versions and use that > rather than the nicer C++ wrappers. Thankfully, TCP_Socket has similar test for different interface versions that I based on. Done.
https://codereview.chromium.org/704133005/diff/80001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (left): https://codereview.chromium.org/704133005/diff/80001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:160: recvfrom_addr_resource_ = addr; Why did these get removed?
https://codereview.chromium.org/704133005/diff/80001/ppapi/proxy/udp_socket_r... File ppapi/proxy/udp_socket_resource_base.cc (left): https://codereview.chromium.org/704133005/diff/80001/ppapi/proxy/udp_socket_r... ppapi/proxy/udp_socket_resource_base.cc:160: recvfrom_addr_resource_ = addr; On 2015/02/17 15:46:57, bbudge wrote: > Why did these get removed? For sure it was by mistake, will fix it.
https://codereview.chromium.org/704133005/diff/100001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/100001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:348: We should add a TODO comment here to check for multicast permissions. That will require us to fix how we check permissions here: https://code.google.com/p/chromium/issues/detail?id=434430 // TODO(etrunko) Check that app has multicast permission. https://codereview.chromium.org/704133005/diff/100001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/704133005/diff/100001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: check_bind_state = true; // fallthrough I think it's more readable the way it was before, without a fallthrough, since I don't have to think about how the 'check_bind_state' parameter is now changed. https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:301: struct PP_Var value = PP_MakeBool(true); Could we create a SetOption utility function to do this and return the error code, so we can exhaustively test this? e.g. int32_t TestUDPSocket::SetOptionValue(PP_Resource socket, PP_UDPSocket_Option option) { TestCompletionCallback cb(instance_->pp_instance(), callback_type()); cb.WaitForResult(socket_interface_1_0_->SetOption(socket, option, value, cb.GetCallback().pp_completion_callback())); } then: ASSERT_EQ(PP_ERROR_BADARGUMENT, SetOptionValue(socket, PP_UDPSOCKET_OPTION_MULTICAST_LOOP)); https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:309: PASS(); If you create a resource (in this case 'socket') you need to release it, via the PPB_Core interface. This directory should have examples. https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:325: PASS(); release 'socket'
https://codereview.chromium.org/704133005/diff/100001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/100001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:348: On 2015/02/19 21:27:04, bbudge wrote: > We should add a TODO comment here to check for multicast permissions. That will > require us to fix how we check permissions here: > https://code.google.com/p/chromium/issues/detail?id=434430 > > // TODO(etrunko) Check that app has multicast permission. Done. https://codereview.chromium.org/704133005/diff/100001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.cc (right): https://codereview.chromium.org/704133005/diff/100001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: check_bind_state = true; // fallthrough On 2015/02/19 21:27:04, bbudge wrote: > I think it's more readable the way it was before, without a fallthrough, since I > don't have to think about how the 'check_bind_state' parameter is now changed. Done. https://codereview.chromium.org/704133005/diff/100001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.cc:74: check_bind_state = true; // fallthrough On 2015/02/19 21:27:04, bbudge wrote: > I think it's more readable the way it was before, without a fallthrough, since I > don't have to think about how the 'check_bind_state' parameter is now changed. Yeah, basically a matter of preference, I like it better the way I did. :) Anyway, Done. https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:301: struct PP_Var value = PP_MakeBool(true); On 2015/02/19 21:27:04, bbudge wrote: > Could we create a SetOption utility function to do this and return the error > code, so we can exhaustively test this? e.g. > > int32_t TestUDPSocket::SetOptionValue(PP_Resource socket, PP_UDPSocket_Option > option) { > TestCompletionCallback cb(instance_->pp_instance(), callback_type()); > cb.WaitForResult(socket_interface_1_0_->SetOption(socket, option, value, > cb.GetCallback().pp_completion_callback())); > } > > then: > > ASSERT_EQ(PP_ERROR_BADARGUMENT, > SetOptionValue(socket, PP_UDPSOCKET_OPTION_MULTICAST_LOOP)); Done. https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:309: PASS(); On 2015/02/19 21:27:05, bbudge wrote: > If you create a resource (in this case 'socket') you need to release it, via the > PPB_Core interface. This directory should have examples. Done. https://codereview.chromium.org/704133005/diff/100001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:325: PASS(); On 2015/02/19 21:27:05, bbudge wrote: > release 'socket' Done.
https://codereview.chromium.org/704133005/diff/120001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/120001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:349: // TODO(etrunko) Check that app has multicast permission. Although I added the comment as requested, I am not sure what else I need to do to check for multicast permissions. According to the bug you referred, it works already. https://code.google.com/p/chromium/issues/detail?id=434430 https://codereview.chromium.org/704133005/diff/120001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/120001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:10: [generate_thunk] This was removed in a previous commit (Remove timing limitations ...), not really sure why. I compared both the auto-generated code and the manually added in that commit and I could not spot any critical difference that would justify this removal. Plus, it was error prone: I had forgotten to add JoinGroup and LeaveGroup functions to thunk code, which is now fixed. https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:94: I did not find an easier way to check for specific versions of interfaces. https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:524: PASS(); Again, if there is a better way to check for interface versions, please advise. All other tests are OK, this is the only one left. I'm working on it right now.
bbudge@ PTAL
Misc comments, getting close. Could you edit the issue to describe your change to browser_tests too? https://codereview.chromium.org/704133005/diff/120001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/120001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:349: // TODO(etrunko) Check that app has multicast permission. On 2015/02/23 22:22:15, etrunko wrote: > Although I added the comment as requested, I am not sure what else I need to do > to check for multicast permissions. According to the bug you referred, it works > already. > > https://code.google.com/p/chromium/issues/detail?id=434430 We can work that out in a follow on patch. https://codereview.chromium.org/704133005/diff/120001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/120001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:10: [generate_thunk] On 2015/02/23 22:22:15, etrunko wrote: > This was removed in a previous commit (Remove timing limitations ...), not > really sure why. > > I compared both the auto-generated code and the manually added in that commit > and I could not spot any critical difference that would justify this removal. > > Plus, it was error prone: I had forgotten to add JoinGroup and LeaveGroup > functions to thunk code, which is now fixed. OK https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:94: On 2015/02/23 22:22:15, etrunko wrote: > I did not find an easier way to check for specific versions of interfaces. This is fine. You don't need to test for 1.2 though, since this code will never run against Chrome without the 1.2 api. https://codereview.chromium.org/704133005/diff/120001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:524: PASS(); On 2015/02/23 22:22:15, etrunko wrote: > Again, if there is a better way to check for interface versions, please advise. > All other tests are OK, this is the only one left. I'm working on it right now. You don't have to check for 1.2 (see comment above.) https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:33: #include "ppapi/thunk/ppb_net_address_api.h" The last two #includes shouldn't be needed here. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:39: using ppapi::thunk::PPB_NetAddress_API; These two usings shouldn't be needed. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:165: // If the socket is already connected, proxy the value to UDPSocket. nit: s/connected/bound might be a little clearer since socket_ is set after DoBind finishes. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:220: // If the socket is already connected, proxy the value to UDPSocket. s/connected/bound https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:239: // If the socket is already connected, proxy the value to UDPSocket. s/connected/bound https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:338: DCHECK(context); This DCHECK isn't necessary. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:357: DCHECK(context); ditto https://codereview.chromium.org/704133005/diff/140001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/140001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:70: * This is only supported in version 1.2 of the API (Chrome 42) and later. s/42/43 here and below https://codereview.chromium.org/704133005/diff/140001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/140001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:93: has_interface_1_2_ = iface_1_2 != NULL; No need to check for 1.2 here like a normal plugin, this code only runs against Chrome with UDP 1.2
https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:33: #include "ppapi/thunk/ppb_net_address_api.h" On 2015/03/02 16:03:08, bbudge wrote: > The last two #includes shouldn't be needed here. Done. Those were leftovers from first version of the patch, thanks. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:39: using ppapi::thunk::PPB_NetAddress_API; On 2015/03/02 16:03:08, bbudge wrote: > These two usings shouldn't be needed. Done. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:165: // If the socket is already connected, proxy the value to UDPSocket. On 2015/03/02 16:03:08, bbudge wrote: > nit: s/connected/bound > might be a little clearer since socket_ is set after DoBind finishes. Done. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:220: // If the socket is already connected, proxy the value to UDPSocket. On 2015/03/02 16:03:08, bbudge wrote: > s/connected/bound Done. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:239: // If the socket is already connected, proxy the value to UDPSocket. On 2015/03/02 16:03:08, bbudge wrote: > s/connected/bound Done. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:338: DCHECK(context); On 2015/03/02 16:03:08, bbudge wrote: > This DCHECK isn't necessary. Done. https://codereview.chromium.org/704133005/diff/140001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:357: DCHECK(context); On 2015/03/02 16:03:08, bbudge wrote: > ditto Done. https://codereview.chromium.org/704133005/diff/140001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (right): https://codereview.chromium.org/704133005/diff/140001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:70: * This is only supported in version 1.2 of the API (Chrome 42) and later. On 2015/03/02 16:03:08, bbudge wrote: > s/42/43 > here and below Done.
LGTM https://codereview.chromium.org/704133005/diff/160001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h (right): https://codereview.chromium.org/704133005/diff/160001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:142: // Multicast options nit: // Multicast options, if socket hasn't been bound.
The CQ bit was checked by eduardo.lima@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/704133005/#ps180001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/704133005/180001
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...)
Pre submit bot is failing because I increased the milestone to M43. https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... File ppapi/api/ppb_udp_socket.idl (left): https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... ppapi/api/ppb_udp_socket.idl:13: }; It seems I am not allowed to change the milestone to M43, right? What should I do here?
On 2015/03/04 19:21:47, etrunko wrote: > Pre submit bot is failing because I increased the milestone to M43. > > https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... > File ppapi/api/ppb_udp_socket.idl (left): > > https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... > ppapi/api/ppb_udp_socket.idl:13: }; > It seems I am not allowed to change the milestone to M43, right? What should I > do here? You need an LGTM from security team for new pepper messages (join, leave). See ppapi/proxy/OWNERS for a list of reviewers. Then, add NOPRESUBMIT=true to the issue description: http://www.chromium.org/developers/testing/commit-queue
eduardo.lima@intel.com changed reviewers: + dcheng@chromium.org
On 2015/03/04 21:14:58, bbudge wrote: > On 2015/03/04 19:21:47, etrunko wrote: > > Pre submit bot is failing because I increased the milestone to M43. > > > > > https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... > > File ppapi/api/ppb_udp_socket.idl (left): > > > > > https://codereview.chromium.org/704133005/diff/180001/ppapi/api/ppb_udp_socke... > > ppapi/api/ppb_udp_socket.idl:13: }; > > It seems I am not allowed to change the milestone to M43, right? What should I > > do here? > > You need an LGTM from security team for new pepper messages (join, leave). See > ppapi/proxy/OWNERS for a list of reviewers. > > Then, add NOPRESUBMIT=true to the issue description: > > http://www.chromium.org/developers/testing/commit-queue +dcheng
bbudge, The *_rel bots are failing in those tests which I checked for the API version to compare the correct expected return value of the functions, but I removed the check as advised in comment #28. Is it possible to disable those or what could be done?
bbudge, The *_rel bots are failing in those tests which I checked for the API version to compare the correct expected return value of the functions, but I removed the check as advised in comment #28. Is it possible to disable those or what could be done?
https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:217: if (socket_.get()) No .get(). https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:236: if (socket_.get()) Ditto. https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:335: if (!socket_.get()) Same for .get() here and elsewhere. https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:344: // TODO(etrunko) Check that app has multicast permission. Nit: TODO(etrunko): (missing a colon) Also, this seems important. When is this going to be added? https://codereview.chromium.org/704133005/diff/180001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/704133005/diff/180001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:77: scoped_refptr<TrackedCallback> callback); As an aside, it's slightly more efficient to pass a scoped_refptr<T> as const scoped_refptr<T>&, as it avoids a copy and a refcount increment/decrement for the copied argument. That being said, Chromium code (by a 2:1) ratio uses scoped_refptr<T> instead of const scoped_refptr<T>& in function parameter lists, so it's probably not worth changing this (especially since there are some long-term plans to make scoped_refptr<T> moveable). https://codereview.chromium.org/704133005/diff/180001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:77: scoped_refptr<TrackedCallback> callback); As an aside, it's slightly more efficient to pass a scoped_refptr<T> as const scoped_refptr<T>&, as it avoids a copy and a refcount increment/decrement for the copied argument. That being said, Chromium code (by a 2:1) ratio uses scoped_refptr<T> instead of const scoped_refptr<T>& in function parameter lists, so it's probably not worth changing this (especially since there are some long-term plans to make scoped_refptr<T> moveable).
https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:344: // TODO(etrunko) Check that app has multicast permission. On 2015/03/05 15:34:49, dcheng wrote: > Nit: TODO(etrunko): (missing a colon) > > Also, this seems important. When is this going to be added? This check must be added before the API can be made stable (available to all apps on Stable channel.) Since this is currently a 'Dev' API, it is only available on Canary and, Dev channels, and with a command line flag. We could add this here, but there was some confusion about whether permissions checking was correct before, which is why we are deferring this. If you prefer, we can try to add it in this CL.
On 2015/03/05 at 16:42:55, bbudge wrote: > https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... > File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): > > https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:344: // TODO(etrunko) Check that app has multicast permission. > On 2015/03/05 15:34:49, dcheng wrote: > > Nit: TODO(etrunko): (missing a colon) > > > > Also, this seems important. When is this going to be added? > > This check must be added before the API can be made stable (available to all apps on Stable channel.) Since this is currently a 'Dev' API, it is only available on Canary and, Dev channels, and with a command line flag. > > We could add this here, but there was some confusion about whether permissions checking was correct before, which is why we are deferring this. If you prefer, we can try to add it in this CL. I'm OK with leaving it the way it currently is provided two things are true: - This is guarded by a command-line flag (which it sounds like it is) - We have a M43 release blocker bug to figure out what to do here so we don't lose track of this.
https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:217: if (socket_.get()) On 2015/03/05 15:34:49, dcheng wrote: > No .get(). Done. https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:236: if (socket_.get()) On 2015/03/05 15:34:49, dcheng wrote: > Ditto. Done. https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:335: if (!socket_.get()) On 2015/03/05 15:34:49, dcheng wrote: > Same for .get() here and elsewhere. Is this to be changed all over this file? For example the above snippet at line 322. https://codereview.chromium.org/704133005/diff/180001/ppapi/proxy/udp_socket_... File ppapi/proxy/udp_socket_resource_base.h (right): https://codereview.chromium.org/704133005/diff/180001/ppapi/proxy/udp_socket_... ppapi/proxy/udp_socket_resource_base.h:77: scoped_refptr<TrackedCallback> callback); On 2015/03/05 15:34:49, dcheng wrote: > As an aside, it's slightly more efficient to pass a scoped_refptr<T> as const > scoped_refptr<T>&, as it avoids a copy and a refcount increment/decrement for > the copied argument. > > That being said, Chromium code (by a 2:1) ratio uses scoped_refptr<T> instead of > const scoped_refptr<T>& in function parameter lists, so it's probably not worth > changing this (especially since there are some long-term plans to make > scoped_refptr<T> moveable). Acknowledged.
https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:335: if (!socket_.get()) On 2015/03/05 17:07:12, etrunko wrote: > On 2015/03/05 15:34:49, dcheng wrote: > > Same for .get() here and elsewhere. > > Is this to be changed all over this file? For example the above snippet at line > 322. No, let's just change the ones that were added. No need to introduce more churn at this time.
On 2015/03/05 17:08:29, dcheng wrote: > > No, let's just change the ones that were added. No need to introduce more churn > at this time. Thanks, new version is available.
On 2015/03/05 16:48:11, dcheng wrote: > On 2015/03/05 at 16:42:55, bbudge wrote: > > > https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... > > File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > (right): > > > > > https://codereview.chromium.org/704133005/diff/180001/content/browser/rendere... > > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:344: > // TODO(etrunko) Check that app has multicast permission. > > On 2015/03/05 15:34:49, dcheng wrote: > > > Nit: TODO(etrunko): (missing a colon) > > > > > > Also, this seems important. When is this going to be added? > > > > This check must be added before the API can be made stable (available to all > apps on Stable channel.) Since this is currently a 'Dev' API, it is only > available on Canary and, Dev channels, and with a command line flag. > > > > We could add this here, but there was some confusion about whether permissions > checking was correct before, which is why we are deferring this. If you prefer, > we can try to add it in this CL. > > I'm OK with leaving it the way it currently is provided two things are true: > - This is guarded by a command-line flag (which it sounds like it is) > - We have a M43 release blocker bug to figure out what to do here so we don't > lose track of this. Added a release blocker for M43 Stable https://code.google.com/p/chromium/issues/detail?id=464452
Eduardo, it turns out that we need to check that we actually are on 'Dev' channel before allowing Join and Leave to succeed. So you'll need to add checking for that and also before setting the new options. Please hold off on landing this until we've added those. I'll put together a comment on how you should go about that later today.
To add permission checks, follow the pattern used by PPB_Compositor, which is a dev only api: https://code.google.com/p/chromium/codesearch#search/&q=IsPluginAllowedToUseC... In short, you'll need to add an abstract method in src/content like 'IsPluginAllowedToUseMulticast'. The stub implementation in src/content would return false, and the real implementation in src/chrome would have the real check. You'd only call this before performing the new operations of the API. Here's compositor's implementation: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch... In your case, you can remove the whitelist check.
On 2015/03/05 19:09:45, bbudge wrote: > To add permission checks, follow the pattern used by PPB_Compositor, which is a > dev only api: > > https://code.google.com/p/chromium/codesearch#search/&q=IsPluginAllowedToUseC... > > In short, you'll need to add an abstract method in src/content like > 'IsPluginAllowedToUseMulticast'. The stub implementation in src/content would > return false, and the real implementation in src/chrome would have the real > check. You'd only call this before performing the new operations of the API. > Here's compositor's implementation: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch... > > In your case, you can remove the whitelist check. Done, though tests are still failing without the API version check.
On 2015/03/05 21:03:29, etrunko wrote: > On 2015/03/05 19:09:45, bbudge wrote: > > To add permission checks, follow the pattern used by PPB_Compositor, which is > a > > dev only api: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=IsPluginAllowedToUseC... > > > > In short, you'll need to add an abstract method in src/content like > > 'IsPluginAllowedToUseMulticast'. The stub implementation in src/content would > > return false, and the real implementation in src/chrome would have the real > > check. You'd only call this before performing the new operations of the API. > > Here's compositor's implementation: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch... > > > > In your case, you can remove the whitelist check. > > Done, though tests are still failing without the API version check. Your tests use the C++ wrappers. You'll have to update those to query for the new 1.2 API: ppapi/cpp/udp_socket.cc
https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:211: case PP_UDPSOCKET_OPTION_MULTICAST_LOOP: { check permissions https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:229: case PP_UDPSOCKET_OPTION_MULTICAST_TTL: { check permissions https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:337: SocketPermissionRequest::UDP_MULTICAST_MEMBERSHIP, addr); Nice. https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:344: } First, you should use your new IsPluginAllowedToUseMulticastAPIs method. Even if an app has permissions, this API is still restricted to Dev channel for now. Second, elsewhere in Chrome, the permission is also checked when leaving a group. So this code should be extracted into a separate method. I think you should make both checks (Dev, and app permissions) for SetOptions (new ones), Join, and Leave. https://codereview.chromium.org/704133005/diff/220001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:361: DCHECK_CURRENTLY_ON(BrowserThread::UI); check permissions https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... content/public/renderer/content_renderer_client.h:297: // Returns true if Pepper Multicast APIs are available for plugins. s/Multicast/UDP Multicast https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... content/public/renderer/content_renderer_client.h:298: virtual bool IsPluginAllowedToUseMulticastAPI(); On further thought, I think 'IsPluginAllowedToUseUDPMulticastAPI' might be a better name.
On 2015/03/05 22:02:18, bbudge wrote: > https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... > content/public/renderer/content_renderer_client.h:298: virtual bool > IsPluginAllowedToUseMulticastAPI(); > On further thought, I think 'IsPluginAllowedToUseUDPMulticastAPI' might be a > better name. What do you think about reusing IsPluginAllowedToUseDevChannelAPIs() method instead of creating this new one which basically duplicates code?
On 2015/03/06 17:54:31, etrunko wrote: > On 2015/03/05 22:02:18, bbudge wrote: > > > https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... > > content/public/renderer/content_renderer_client.h:298: virtual bool > > IsPluginAllowedToUseMulticastAPI(); > > On further thought, I think 'IsPluginAllowedToUseUDPMulticastAPI' might be a > > better name. > > What do you think about reusing IsPluginAllowedToUseDevChannelAPIs() method > instead of creating this new one which basically duplicates code? That occurred to me but I wasn't sure whether this might be whitelisted in the future. If you want, use IsPluginAllowedToUseDevChannelAPIs for now.
On 2015/03/06 18:10:46, bbudge wrote: > On 2015/03/06 17:54:31, etrunko wrote: > > On 2015/03/05 22:02:18, bbudge wrote: > > > > > > https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... > > > content/public/renderer/content_renderer_client.h:298: virtual bool > > > IsPluginAllowedToUseMulticastAPI(); > > > On further thought, I think 'IsPluginAllowedToUseUDPMulticastAPI' might be a > > > better name. > > > > What do you think about reusing IsPluginAllowedToUseDevChannelAPIs() method > > instead of creating this new one which basically duplicates code? > > That occurred to me but I wasn't sure whether this might be whitelisted in the > future. If you want, use IsPluginAllowedToUseDevChannelAPIs for now. bbudge, it seems I can't use those methos in pepper_udp_socket_message_filter.cc. My new version has the following snippet: http://pastebin.com/Jq3je2VY It builds but I get this error when uploading: ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc Illegal include: "content/public/renderer/content_renderer_client.h" Because of "-content" from content's include_rules.
On 2015/03/06 21:01:08, etrunko wrote: > On 2015/03/06 18:10:46, bbudge wrote: > > On 2015/03/06 17:54:31, etrunko wrote: > > > On 2015/03/05 22:02:18, bbudge wrote: > > > > > > > > > > https://codereview.chromium.org/704133005/diff/220001/content/public/renderer... > > > > content/public/renderer/content_renderer_client.h:298: virtual bool > > > > IsPluginAllowedToUseMulticastAPI(); > > > > On further thought, I think 'IsPluginAllowedToUseUDPMulticastAPI' might be > a > > > > better name. > > > > > > What do you think about reusing IsPluginAllowedToUseDevChannelAPIs() method > > > instead of creating this new one which basically duplicates code? > > > > That occurred to me but I wasn't sure whether this might be whitelisted in the > > future. If you want, use IsPluginAllowedToUseDevChannelAPIs for now. > > bbudge, it seems I can't use those methos in > pepper_udp_socket_message_filter.cc. My new version has the following snippet: > > http://pastebin.com/Jq3je2VY > > It builds but I get this error when uploading: > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > Illegal include: "content/public/renderer/content_renderer_client.h" > Because of "-content" from content's include_rules. Right, that's because the UDP socket message filter lives in the browser process, which can't depend on the renderer. Use this one instead: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
On 2015/03/06 21:05:08, bbudge wrote: > Right, that's because the UDP socket message filter lives in the browser > process, which can't depend on the renderer. Use this one instead: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Oh, I missed that one, sorry. New version is available. PTAL. Added a new static method: NetAddressPrivateImpl::CreateNetAddressPrivateFromAnyAddress().
https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:215: PP_FromBool(false), &any_addr); s/PP_FromBool(false)/PP_FALSE https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:239: PP_FromBool(false), &any_addr); PP_FALSE https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:686: ContentBrowserClient* content_renderer_client = GetContentClient()->browser(); nit: content_browser_client or content_client. https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:689: return false; It would be better if we could return PP_ERROR_FAILED when 'Dev' isn't available, and PP_ERROR_NOACCESS when the permissions aren't there. Could we change this method to return an int32_t and those two error codes, and PP_OK on success? https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h (right): https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:134: bool CanUseUDPMulticastAPI(const PP_NetAddress_Private& addr); nit: In this context, it would be OK to leave off the UDP in the name. https://codereview.chromium.org/704133005/diff/240001/ppapi/shared_impl/priva... File ppapi/shared_impl/private/net_address_private_impl.h (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/shared_impl/priva... ppapi/shared_impl/private/net_address_private_impl.h:51: PP_NetAddress_Private* addr); This shouldn't be with the conversion routines. How about moving it up and naming it GetAnyAddress? https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:516: PASS(); You don't need this check. The reason your tests fail is that you haven't updated the C++ wrappers to query for the 1.2 API. This check makes the bots green because it bypasses all of your 1.2 tests. See the ppapi/cpp/udp_socket.* ctor definition, which already handles 1.0 and 1.1. Just add similar boilerplate for 1.2.
https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:215: PP_FromBool(false), &any_addr); On 2015/03/07 00:52:21, bbudge wrote: > s/PP_FromBool(false)/PP_FALSE Done. https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:239: PP_FromBool(false), &any_addr); On 2015/03/07 00:52:21, bbudge wrote: > PP_FALSE Done. https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:686: ContentBrowserClient* content_renderer_client = GetContentClient()->browser(); On 2015/03/07 00:52:21, bbudge wrote: > nit: content_browser_client or content_client. Done. https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:689: return false; On 2015/03/07 00:52:21, bbudge wrote: > It would be better if we could return PP_ERROR_FAILED when 'Dev' isn't > available, and PP_ERROR_NOACCESS when the permissions aren't there. Could we > change this method to return an int32_t and those two error codes, and PP_OK on > success? Done. https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h (right): https://codereview.chromium.org/704133005/diff/240001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:134: bool CanUseUDPMulticastAPI(const PP_NetAddress_Private& addr); On 2015/03/07 00:52:21, bbudge wrote: > nit: In this context, it would be OK to leave off the UDP in the name. Done. https://codereview.chromium.org/704133005/diff/240001/ppapi/shared_impl/priva... File ppapi/shared_impl/private/net_address_private_impl.h (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/shared_impl/priva... ppapi/shared_impl/private/net_address_private_impl.h:51: PP_NetAddress_Private* addr); On 2015/03/07 00:52:21, bbudge wrote: > This shouldn't be with the conversion routines. How about moving it up and > naming it GetAnyAddress? Done. https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.cc (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.cc:516: PASS(); On 2015/03/07 00:52:21, bbudge wrote: > You don't need this check. The reason your tests fail is that you haven't > updated the C++ wrappers to query for the 1.2 API. This check makes the bots > green because it bypasses all of your 1.2 tests. See the ppapi/cpp/udp_socket.* > ctor definition, which already handles 1.0 and 1.1. Just add similar boilerplate > for 1.2. Done.
Again, LGTM
https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.h (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.h:16: typedef int32_t (*UDPSocketSetOption)(PP_Resource udp_socket, We prefer using foo = bar; to typedefs (http://chromium-cpp.appspot.com/) https://codereview.chromium.org/704133005/diff/280001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/704133005/diff/280001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:341: } \ Omit the \ on this line, since it's the last (real) line of the macro https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:689: if (!content_browser_client->IsPluginAllowedToUseDevChannelAPIs(NULL, nullptr
https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... File ppapi/tests/test_udp_socket.h (right): https://codereview.chromium.org/704133005/diff/240001/ppapi/tests/test_udp_so... ppapi/tests/test_udp_socket.h:16: typedef int32_t (*UDPSocketSetOption)(PP_Resource udp_socket, On 2015/03/09 21:14:33, dcheng wrote: > We prefer using foo = bar; to typedefs (http://chromium-cpp.appspot.com/) If I change it, the code fails to build. I think the nacl compiler does not support C++ 11 features. Here is the output: http://pastebin.com/XLyqY6E9 https://codereview.chromium.org/704133005/diff/280001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/704133005/diff/280001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_browsertest.cc:341: } \ On 2015/03/09 21:14:33, dcheng wrote: > Omit the \ on this line, since it's the last (real) line of the macro Done. https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:689: if (!content_browser_client->IsPluginAllowedToUseDevChannelAPIs(NULL, On 2015/03/09 21:14:33, dcheng wrote: > nullptr Done.
lgtm
https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:215: int32_t ret = CanUseMulticastAPI(any_addr); One last question, there CanUseSocketAPIs (content/browser/renderer_host/pepper/pepper_socket_utils.cc) checks for UI thread, but in this case we are in IO thread, do we need to keep this check here?
https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:215: int32_t ret = CanUseMulticastAPI(any_addr); On 2015/03/09 22:33:45, etrunko wrote: > One last question, there CanUseSocketAPIs > (content/browser/renderer_host/pepper/pepper_socket_utils.cc) checks for UI > thread, but in this case we are in IO thread, do we need to keep this check > here? Missed that, no, you shouldn't call this on the wrong thread though it probably works, except in Debug builds where it will assert. Instead, check CanUseSocketAPIs in Bind, which is run on the UI thread. Cache the result and then check that before setting the options here (and below) on socket_. If the socket_ isn't bound, just save the option values for later.
https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/280001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:215: int32_t ret = CanUseMulticastAPI(any_addr); On 2015/03/09 22:51:03, bbudge wrote: > On 2015/03/09 22:33:45, etrunko wrote: > > One last question, there CanUseSocketAPIs > > (content/browser/renderer_host/pepper/pepper_socket_utils.cc) checks for UI > > thread, but in this case we are in IO thread, do we need to keep this check > > here? > > Missed that, no, you shouldn't call this on the wrong thread though it probably > works, except in Debug builds where it will assert. > > Instead, check CanUseSocketAPIs in Bind, which is run on the UI thread. Cache > the result and then check that before setting the options here (and below) on > socket_. If the socket_ isn't bound, just save the option values for later. Done.
https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:218: if (socket_) See comment below: this should be: if (socket_ && can_use_multicast_api_) ... https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:266: } Sorry if I wasn't clear in my earlier comment. This is not correct. You should call CanUseMulticastAPI here even if these options aren't set, save the result in a field, say 'can_use_multicast_api', and then check that elsewhere. As it is, the plugin could call SetOption right after Bind but before DoBind runs, and avoid the permissions check.
On 2015/03/10 01:30:50, bbudge wrote: > https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... > File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc > (right): > > https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:218: if > (socket_) > See comment below: this should be: > if (socket_ && can_use_multicast_api_) ... > > https://codereview.chromium.org/704133005/diff/300001/content/browser/rendere... > content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:266: } > Sorry if I wasn't clear in my earlier comment. This is not correct. You should > call CanUseMulticastAPI here even if these options aren't set, save the result > in a field, say 'can_use_multicast_api', and then check that elsewhere. > > As it is, the plugin could call SetOption right after Bind but before DoBind > runs, and avoid the permissions check. That's alright, thank you for the comments (and overall patience). New version is available.
eduardo.lima@intel.com changed reviewers: + jwd@chromium.org
+jwd for histograms.xml
The CQ bit was checked by eduardo.lima@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/704133005/#ps360001 (title: "histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/704133005/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
https://codereview.chromium.org/704133005/diff/460001/content/browser/rendere... File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc (right): https://codereview.chromium.org/704133005/diff/460001/content/browser/rendere... content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:463: int net_result = socket->SetMulticastInterface(multicast_ttl_); This is causing the errors in mac, should be SetMulticastTimeToLive() not SetMulticastInterface.
The CQ bit was checked by eduardo.lima@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, dcheng@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/704133005/#ps470001 (title: "Fix typo: SetMulticastInterface -> SetMulticastTimeToLive")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/704133005/470001
Message was sent while issue was closed.
Committed patchset #25 (id:470001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/8171d46410b1ea937b0ae59577b9b36ed151149a Cr-Commit-Position: refs/heads/master@{#320504} |
