Issue 7745005: Initial work for UDP Pepper API

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1271 lines, -44 lines)			Patch
M	AUTHORS	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/browser/renderer_host/pepper_message_filter.h	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+5 lines, -1 line	0 comments	Download
M	content/browser/renderer_host/pepper_message_filter.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	8 chunks	+362 lines, -43 lines	0 comments	Download
M	content/ppapi_plugin/ppapi_thread.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+6 lines, -0 lines	0 comments	Download
A	ppapi/api/private/ppb_flash_udp_socket.idl	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+59 lines, -0 lines	0 comments	Download
A	ppapi/c/private/ppb_flash_udp_socket.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+76 lines, -0 lines	0 comments	Download
A	ppapi/cpp/private/flash_udp_socket.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+40 lines, -0 lines	0 comments	Download
A	ppapi/cpp/private/flash_udp_socket.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+72 lines, -0 lines	0 comments	Download
M	ppapi/ppapi_cpp.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/ppapi_proxy.gypi	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ppapi/ppapi_shared.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ppapi/proxy/interface_id.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/proxy/interface_list.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	3 chunks	+3 lines, -0 lines	0 comments	Download
M	ppapi/proxy/ppapi_messages.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+35 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_flash_udp_socket_proxy.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+60 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_flash_udp_socket_proxy.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+390 lines, -0 lines	0 comments	Download
M	ppapi/proxy/resource_creation_proxy.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/proxy/resource_creation_proxy.cc	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+6 lines, -0 lines	0 comments	Download
M	ppapi/shared_impl/resource.h	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_flash_udp_socket_api.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+34 lines, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_flash_udp_socket_thunk.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+98 lines, -0 lines	0 comments	Download
M	ppapi/thunk/resource_creation_api.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/thunk/thunk.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+2 lines, -0 lines	0 comments	Download
M	webkit/plugins/ppapi/plugin_module.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	2 chunks	+3 lines, -0 lines	0 comments	Download
M	webkit/plugins/ppapi/resource_creation_impl.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	webkit/plugins/ppapi/resource_creation_impl.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 37 (0 generated)

Expand Messages | Collapse Messages

viettrungluu

Here's a start. Also, note that, confusingly, you have to send a message (a blank ...

9 years, 3 months ago (2011-08-29 19:20:05 UTC) #1

mtilburg

fixed everything except the refactoring of the Socket Manager... which I am working on currently... ...

9 years, 3 months ago (2011-08-30 16:56:14 UTC) #2

mtilburg

refactoring of TCP and UDP socket managers to share code, created base class FlashSocketManager that ...

9 years, 3 months ago (2011-09-06 22:06:57 UTC) #3

yzshen1

Hi, This is the first part of review comments. http://codereview.chromium.org/7745005/diff/14001/ppapi/c/private/ppb_flash_udp_socket.h File ppapi/c/private/ppb_flash_udp_socket.h (right): http://codereview.chromium.org/7745005/diff/14001/ppapi/c/private/ppb_flash_udp_socket.h#newcode19 ppapi/c/private/ppb_flash_udp_socket.h:19: ...

9 years, 3 months ago (2011-09-07 18:00:36 UTC) #4

mtilburg

This should address all the items from the last code review, with the exception of ...

9 years, 3 months ago (2011-09-15 23:13:32 UTC) #5

brettw

Just some comments I noticed. http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right): http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_socket_proxy.cc#newcode36 ppapi/proxy/ppb_flash_udp_socket_proxy.cc:36: IDToSocketMap* g_id_to_socket = NULL; ...

9 years, 3 months ago (2011-09-16 17:50:16 UTC) #6

yzshen1

http://codereview.chromium.org/7745005/diff/37004/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/37004/content/browser/renderer_host/pepper_message_filter.cc#newcode42 content/browser/renderer_host/pepper_message_filter.cc:42: #include "net/udp/udp_socket.h" You probably don't need this include. http://codereview.chromium.org/7745005/diff/37004/content/browser/renderer_host/pepper_message_filter.cc#newcode248 ...

9 years, 3 months ago (2011-09-17 02:10:06 UTC) #7

mtilburg

* change Disconnect to Close * remove family from UDPSocket, since it can be calculated ...

9 years, 3 months ago (2011-09-21 01:46:42 UTC) #8

yzshen1

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right): http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_socket_proxy.cc#newcode36 ppapi/proxy/ppb_flash_udp_socket_proxy.cc:36: IDToSocketMap* g_id_to_socket = NULL; On 2011/09/21 01:46:43, mtilburg wrote: ...

9 years, 3 months ago (2011-09-21 18:46:01 UTC) #9

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:36: IDToSocketMap* g_id_to_socket =
NULL;
On 2011/09/21 01:46:43, mtilburg wrote:
> On 2011/09/16 17:50:17, brettw wrote:
> > I was looking at this and I think we can avoid this global map. Yuzhu is
going
> > to also update the TCP socket code which you based this on.
> > 
> > We already have IDs in the plugin for this object, which are the PP_Resource
> > IDs. If the plugin sends the PP_Resource to the browser and then the browser
> > echoes it back when necessary, we can use the PP_Resource to get back to the
> > object using the regular EnterResource template stuff.
> > 
> > I think this will simplify things a bit and remove the extra global
treacking,
> > which would be nice.
> 
> Brett- this sounds good... can I recommend I change this on the next check-in.
> this change is already fairly large, and getting kind of stale... let me know
> what you think...

I could do it if you would like me to. I have to change the TCP socket.

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:164:
GetDispatcher()->SendToBrowser(msg_deletor.release());
On 2011/09/21 01:46:43, mtilburg wrote:
> On 2011/09/17 02:10:07, yzshen1 wrote:
> > You could remove msg_deletor.
> 
> I don't understand what you mean here, doesn't msg_deletor hold the message
that
> we are sending to the Browser?

I mean, you can directly use 
GetDispatcher()->SendToBrowser(new PpapiHostMsg...);

msg_deletor is not very useful here -- it gives away the ownership of the
message object in line 155; and never does the job of a "deletor".

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:147: if (bind_in_progress_)
If you use this boolean value to indicate "it has been binded", it might be
better to:
1) rename it to binded_;
2) only set it to true when a bind operation has succeeded;
3) don't allow a second bind request if the previous one hasn't finished. You
can know whether there is a unfinished Bind operation by looking at the
completion callback:
if (bind_callback_.func)
  return PP_ERROR_INPROGRESS;

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:207: sendto_pending_ = true;
1) You haven't actually checked the value of sendto_pending_.

2) Whenever we have a non-NULL completion callback, we have a pending sendto, so
you can use the following instead of defining |sendto_pending_|.
if (sendto_callback_.func)
  return PP_ERROR_INPROGRESS;

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:213: if (!bind_in_progress_)
As I said, you should remember whether it has been closed and prevent from doing
multiple close operations. |bind_in_progress_| seems irrelevant here. Am I
missing anything?

mtilburg

* change bind_in_progress_ to binded_ * add check to sendto to make sure it's not ...

9 years, 3 months ago (2011-09-22 13:53:22 UTC) #10

* change bind_in_progress_ to binded_
* add check to sendto to make sure it's not already in progress

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:36: IDToSocketMap* g_id_to_socket =
NULL;
On 2011/09/21 18:46:03, yzshen1 wrote:
> On 2011/09/21 01:46:43, mtilburg wrote:
> > On 2011/09/16 17:50:17, brettw wrote:
> > > I was looking at this and I think we can avoid this global map. Yuzhu is
> going
> > > to also update the TCP socket code which you based this on.
> > > 
> > > We already have IDs in the plugin for this object, which are the
PP_Resource
> > > IDs. If the plugin sends the PP_Resource to the browser and then the
browser
> > > echoes it back when necessary, we can use the PP_Resource to get back to
the
> > > object using the regular EnterResource template stuff.
> > > 
> > > I think this will simplify things a bit and remove the extra global
> treacking,
> > > which would be nice.
> > 
> > Brett- this sounds good... can I recommend I change this on the next
check-in.
> > this change is already fairly large, and getting kind of stale... let me
know
> > what you think...
> 
> I could do it if you would like me to. I have to change the TCP socket.

Doesn't matter to me, if you have the change already pending, I don't mind if
you handle it.  Thanks!

http://codereview.chromium.org/7745005/diff/37004/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:164:
GetDispatcher()->SendToBrowser(msg_deletor.release());
On 2011/09/21 18:46:03, yzshen1 wrote:
> On 2011/09/21 01:46:43, mtilburg wrote:
> > On 2011/09/17 02:10:07, yzshen1 wrote:
> > > You could remove msg_deletor.
> > 
> > I don't understand what you mean here, doesn't msg_deletor hold the message
> that
> > we are sending to the Browser?
> 
> I mean, you can directly use 
> GetDispatcher()->SendToBrowser(new PpapiHostMsg...);
> 
> msg_deletor is not very useful here -- it gives away the ownership of the
> message object in line 155; and never does the job of a "deletor".

Done.

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:147: if (bind_in_progress_)
On 2011/09/21 18:46:03, yzshen1 wrote:
> If you use this boolean value to indicate "it has been binded", it might be
> better to:
> 1) rename it to binded_;
> 2) only set it to true when a bind operation has succeeded;
> 3) don't allow a second bind request if the previous one hasn't finished. You
> can know whether there is a unfinished Bind operation by looking at the
> completion callback:
> if (bind_callback_.func)
>   return PP_ERROR_INPROGRESS;

Done.

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:207: sendto_pending_ = true;
On 2011/09/21 18:46:03, yzshen1 wrote:
> 1) You haven't actually checked the value of sendto_pending_.
> 
> 2) Whenever we have a non-NULL completion callback, we have a pending sendto,
so
> you can use the following instead of defining |sendto_pending_|.
> if (sendto_callback_.func)
>   return PP_ERROR_INPROGRESS;
> 

Done.

http://codereview.chromium.org/7745005/diff/43001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:213: if (!bind_in_progress_)
On 2011/09/21 18:46:03, yzshen1 wrote:
> As I said, you should remember whether it has been closed and prevent from
doing
> multiple close operations. |bind_in_progress_| seems irrelevant here. Am I
> missing anything?

so |binded_| (previously bind_in_progress_| will keep weather or not the socked
has been binded(or 'opened')  The logic as it works will

1) just return if Bind was never called(and thus, there is nothing to close)
2) close the socket if binded_ is true ('opened') and then set binded_ to false,
which will make any further calls to Close just return, unless Bind is called
again.

I've looked over this a bunch of times, this really seems right to me.

viettrungluu

Sorry, I got distracted. http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc#newcode211 content/browser/renderer_host/pepper_message_filter.cc:211: // Base class for TCP ...

9 years, 3 months ago (2011-09-22 16:45:34 UTC) #11

brettw

I think it's probably a good idea to land the global tracking thing later as ...

9 years, 3 months ago (2011-09-22 20:03:30 UTC) #12

yzshen1

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right): http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_socket_proxy.cc#newcode209 ppapi/proxy/ppb_flash_udp_socket_proxy.cc:209: if (!binded_) It is not right to use binded_ ...

9 years, 3 months ago (2011-09-22 20:07:04 UTC) #13

yzshen1

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc#newcode870 content/browser/renderer_host/pepper_message_filter.cc:870: void PepperMessageFilter::FlashUDPSocketManager::OnMsgBind( On 2011/09/22 16:45:34, viettrungluu wrote: > Why ...

9 years, 3 months ago (2011-09-22 20:14:27 UTC) #14

mtilburg

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc#newcode211 content/browser/renderer_host/pepper_message_filter.cc:211: // Base class for TCP and UDP socket managers ...

9 years, 3 months ago (2011-09-22 23:44:34 UTC) #15

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:211: // Base class for
TCP and UDP socket managers
On 2011/09/22 16:45:34, viettrungluu wrote:
> Needs punctation.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:215: explicit
FlashSocketManager(PepperMessageFilter* pepper_message_filter);
On 2011/09/22 16:45:34, viettrungluu wrote:
> You may as well inline the definitions of these methods, seeing as how it's in
a
> .cc file and not a header file.

this might change if we move all this to another file

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:242: // unique managers,
so the socket ID can be the same in this case
On 2011/09/22 16:45:34, viettrungluu wrote:
> Missing period.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:247: //
FlashTCPSocketManager and a FlashUDPSocketManager in this filter)
On 2011/09/22 16:45:34, viettrungluu wrote:
> The parenthetical comment is probably unnecessary.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:248: // talking to it
On 2011/09/22 16:45:34, viettrungluu wrote:
> "

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:250: DCHECK(socket_id);
On 2011/09/22 16:45:34, viettrungluu wrote:
> This DCHECK() isn't necessary; we're sure to crash anyway if socket_id is
null.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:251: if (sockets_.size()
>= std::numeric_limits<uint32>::max()) {
On 2011/09/22 16:45:34, viettrungluu wrote:
> It seems silly to do a check like this, since it'll never be hit on 32-bit
> systems, and you'll almost certainly have died before hitting it on 64-bit
> systems.
> 
> It *would* be reasonable to limit the number of sockets, so perhaps a check
> should be made against some other number (|kMaxSockets| or some such).

I don't mind fixing this, if it's incorrect, but I should note, this is not my
code.  Remember this is the code from TCPSocketManager that I moved into the
common parent class so that UDPSocketManager could share it.  We may want to run
these changes by Yuzhu

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:271: explicit
FlashTCPSocketManager(PepperMessageFilter* pepper_message_filter);
On 2011/09/22 16:45:34, viettrungluu wrote:
> Ditto for inlining methods, here and elsewhere.

Again... this is not my code... furthermore, inlining all these functions causes
problems because FlashTCPSocket and FlashTCPSocketManager reference each
other... I think we should keep this review in the scope of UDP

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:709: else
On 2011/09/22 20:03:30, brettw wrote:
> We always use {} if there's more than one line (add to the "if" as well to
make
> it match).

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:731:
DCHECK(pepper_message_filter);
On 2011/09/22 16:45:34, viettrungluu wrote:
> This DCHECK() is unnecessary. This class doesn't do anything with
> |pepper_message_filter| -- its superclass does (and it already DCHECKs it).

I'll remove this, but again, this is the TCP code, I didn't write this...

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:870: void
PepperMessageFilter::FlashUDPSocketManager::OnMsgBind(
On 2011/09/22 16:45:34, viettrungluu wrote:
> Why do we have to separate Create and Bind anyway?

Create and Bind are separate system calls, so it's reasonable to assume that any
implementation of UDP will have these call separate(as we do in the Flash
Player)

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:870: void
PepperMessageFilter::FlashUDPSocketManager::OnMsgBind(
On 2011/09/22 20:14:27, yzshen1 wrote:
> On 2011/09/22 16:45:34, viettrungluu wrote:
> > Why do we have to separate Create and Bind anyway?
> 
> I think the current implementation doesn't enforce that Bind is the first
method
> to be called on a socket object.
> For example, SendTo can be called before Bind.

That's true, and sendto will return an error, which will be returned to the
user.  The same behavior if sendto is called after ::close is called

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:874: if (iter ==
sockets_.end()) {
On 2011/09/22 16:45:34, viettrungluu wrote:
> Let's just |CHECK(iter != sockets_.end());|.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:886: if (iter ==
sockets_.end()) {
On 2011/09/22 16:45:34, viettrungluu wrote:
> "

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:899: if (iter ==
sockets_.end()) {
On 2011/09/22 16:45:34, viettrungluu wrote:
> "

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:980: // UDP
On 2011/09/22 16:45:34, viettrungluu wrote:
> If this deserves a comment, so too does TCP. Also, punctuation.

Done.

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:1257: }
On 2011/09/22 20:03:30, brettw wrote:
> On 2011/09/22 16:45:34, viettrungluu wrote:
> > This file is too big. Stuff needs to be moved out.
> 
> Let's put FlashUDPSocket in its own flash_udp_socket.h/cc file. I think it's
OK
> to move the class name out of the PepperMessageFilter scope.

I'm guessing we should do something similar with the tcp part?  I'm guessing
that Yuzhu has some changes pending(based on his comments) to the tcp code, so
I'll just do the udp for now, and let him handle the tcp part...

btw- we already have a file called flash_udp_socket.h, besides, should it be a
more descriptive name, like udp_socket_filter.cc/h ?

What about FlashUDPSocketManager?  Same file?

yzshen1

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc#newcode251 content/browser/renderer_host/pepper_message_filter.cc:251: if (sockets_.size() >= std::numeric_limits<uint32>::max()) { This is a good ...

9 years, 3 months ago (2011-09-23 00:00:44 UTC) #16

mtilburg

http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc File content/browser/renderer_host/pepper_message_filter.cc (right): http://codereview.chromium.org/7745005/diff/51001/content/browser/renderer_host/pepper_message_filter.cc#newcode1257 content/browser/renderer_host/pepper_message_filter.cc:1257: } On 2011/09/22 23:44:34, mtilburg wrote: > On 2011/09/22 ...

9 years, 3 months ago (2011-09-23 13:39:24 UTC) #17

mtilburg

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right): http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_socket_proxy.cc#newcode58 ppapi/proxy/ppb_flash_udp_socket_proxy.cc:58: class FlashUDPSocket : public PPB_Flash_UDPSocket_API, On 2011/09/22 20:03:30, brettw ...

9 years, 3 months ago (2011-09-23 17:30:27 UTC) #18

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:58: class FlashUDPSocket : public
PPB_Flash_UDPSocket_API,
On 2011/09/22 20:03:30, brettw wrote:
> I think this class can be in the anonymous namespace since it's only used in
> this .cc file.

Done.

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:209: if (!binded_)
I've added checks at the beginning of Bind/SendTo/RecvFrom to check the binded_
bool.  for Bind, binded_ must be false(so not to allow a second bind on the same
socket) For SendTo and RecvFrom, binded_ must be true, so these operations will
not be allowed unless bind has been called and succeeded.

As for Close, I will not check the binded_ bool, but I will set it to false, so
that no more operations can occur.

I think this should satisfy all the requirements?
On 2011/09/22 20:07:04, yzshen1 wrote:
> It is not right to use binded_ to decide whether Close should be executed or
> not. According to the current implementation, Close line 214 and below needs
to
> be run exactly once for an FlashUDPSocket.
> 
> Let's consider the following examples:
> 1) create a FlashUDPSocket object, and then destruct it directly: you leave an
> invalid entry in g_id_to_socket.
> 2) create a FlashUDPSocket object, call Bind, before the bind finishes you
> destruct the object -> you leave an invalid entry in g_id_to_socket and an
> uncalled completion callback.
> 
> Something else to consider:
> What happens if
> 1) the user calls Bind(), and it succeeds;
> 2) and then the user calls Close();
> 3) and then the user calls other operations (Bind/SendTo/RecvFrom).
> Do you need to fail those operations directly?
> According to the current logic, the browser process will delete the socket
stuff
> at its side directly when it receives PpapiHostMsg_PPBFlashUDPSocket_Close.
> 
> If you allow other operations after Close(), they will never finish. And you
> will get assertion in the browser process because it has no idea about the
> socket_id any more.

yzshen1

9 years, 3 months ago (2011-09-23 18:06:57 UTC) #19

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:209: if (!binded_)
On 2011/09/23 17:30:28, mtilburg wrote:
> I've added checks at the beginning of Bind/SendTo/RecvFrom to check the
binded_
> bool.  for Bind, binded_ must be false(so not to allow a second bind on the
same
> socket) For SendTo and RecvFrom, binded_ must be true, so these operations
will
> not be allowed unless bind has been called and succeeded.
> 
> As for Close, I will not check the binded_ bool, but I will set it to false,
so
> that no more operations can occur.
> 
> I think this should satisfy all the requirements?
> On 2011/09/22 20:07:04, yzshen1 wrote:
> > It is not right to use binded_ to decide whether Close should be executed or
> > not. According to the current implementation, Close line 214 and below needs
> to
> > be run exactly once for an FlashUDPSocket.
> > 
> > Let's consider the following examples:
> > 1) create a FlashUDPSocket object, and then destruct it directly: you leave
an
> > invalid entry in g_id_to_socket.
> > 2) create a FlashUDPSocket object, call Bind, before the bind finishes you
> > destruct the object -> you leave an invalid entry in g_id_to_socket and an
> > uncalled completion callback.
> > 
> > Something else to consider:
> > What happens if
> > 1) the user calls Bind(), and it succeeds;
> > 2) and then the user calls Close();
> > 3) and then the user calls other operations (Bind/SendTo/RecvFrom).
> > Do you need to fail those operations directly?
> > According to the current logic, the browser process will delete the socket
> stuff
> > at its side directly when it receives PpapiHostMsg_PPBFlashUDPSocket_Close.
> > 
> > If you allow other operations after Close(), they will never finish. And you
> > will get assertion in the browser process because it has no idea about the
> > socket_id any more.
> 

No. Please note that line 212-223 is teardown code that must be executed
*exactly once* for the object. And after that point, no operation
(Bind/SendTo/RecvFrom) will succeed, because the browser has forgotten this
socket_id_. (The code will hit NOTREACHED in pepper_message_filter.cc)

As I said, you could have a |closed_| boolean to guard that the logic is
execuated exactly once. And you need to test |closed_| in Bind() so that we will
not try to store the completion callback of Bind if Close has been called.
(Otherwise, that completion callback will never be called. This should be avoid,
because Pepper requires that if we have returned PP_OK_COMPLETIONPENDING, the
callback must be called exactly once later.)

This is one of the feasible approaches, but seems to be the one with minimal
change.

If you would like to, we could have a dicussion over phone. Hopefully that will
be more clear. I think this change is very close to completion now.

mtilburg

9 years, 3 months ago (2011-09-23 18:36:55 UTC) #20

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right):

http://codereview.chromium.org/7745005/diff/51001/ppapi/proxy/ppb_flash_udp_s...
ppapi/proxy/ppb_flash_udp_socket_proxy.cc:209: if (!binded_)
Okay, I added closed_, and check it before I execute this code so that it is
only executed once...  I also added a check in Bind so that we will not re-bind
the socket if it has already been closed.  The binded_ bool is still necessary
to make sure that sendto and recvfrom are not executed unless bind has been
successful
On 2011/09/23 18:06:58, yzshen1 wrote:
> On 2011/09/23 17:30:28, mtilburg wrote:
> > I've added checks at the beginning of Bind/SendTo/RecvFrom to check the
> binded_
> > bool.  for Bind, binded_ must be false(so not to allow a second bind on the
> same
> > socket) For SendTo and RecvFrom, binded_ must be true, so these operations
> will
> > not be allowed unless bind has been called and succeeded.
> > 
> > As for Close, I will not check the binded_ bool, but I will set it to false,
> so
> > that no more operations can occur.
> > 
> > I think this should satisfy all the requirements?
> > On 2011/09/22 20:07:04, yzshen1 wrote:
> > > It is not right to use binded_ to decide whether Close should be executed
or
> > > not. According to the current implementation, Close line 214 and below
needs
> > to
> > > be run exactly once for an FlashUDPSocket.
> > > 
> > > Let's consider the following examples:
> > > 1) create a FlashUDPSocket object, and then destruct it directly: you
leave
> an
> > > invalid entry in g_id_to_socket.
> > > 2) create a FlashUDPSocket object, call Bind, before the bind finishes you
> > > destruct the object -> you leave an invalid entry in g_id_to_socket and an
> > > uncalled completion callback.
> > > 
> > > Something else to consider:
> > > What happens if
> > > 1) the user calls Bind(), and it succeeds;
> > > 2) and then the user calls Close();
> > > 3) and then the user calls other operations (Bind/SendTo/RecvFrom).
> > > Do you need to fail those operations directly?
> > > According to the current logic, the browser process will delete the socket
> > stuff
> > > at its side directly when it receives
PpapiHostMsg_PPBFlashUDPSocket_Close.
> > > 
> > > If you allow other operations after Close(), they will never finish. And
you
> > > will get assertion in the browser process because it has no idea about the
> > > socket_id any more.
> > 
> 
> No. Please note that line 212-223 is teardown code that must be executed
> *exactly once* for the object. And after that point, no operation
> (Bind/SendTo/RecvFrom) will succeed, because the browser has forgotten this
> socket_id_. (The code will hit NOTREACHED in pepper_message_filter.cc)
> 
> As I said, you could have a |closed_| boolean to guard that the logic is
> execuated exactly once. And you need to test |closed_| in Bind() so that we
will
> not try to store the completion callback of Bind if Close has been called.
> (Otherwise, that completion callback will never be called. This should be
avoid,
> because Pepper requires that if we have returned PP_OK_COMPLETIONPENDING, the
> callback must be called exactly once later.)
> 
> This is one of the feasible approaches, but seems to be the one with minimal
> change.
> 
> If you would like to, we could have a dicussion over phone. Hopefully that
will
> be more clear. I think this change is very close to completion now.

brettw

I'm happy with this from a high level, so once Yuzhu is happy I think ...

9 years, 3 months ago (2011-09-23 19:08:22 UTC) #21

mtilburg

I've added the .idl file for udp socket... I still have to move it to ...

9 years, 2 months ago (2011-09-26 16:23:12 UTC) #22

yzshen1

LGTM Thanks! You could use the commit queue to submit this change: http://www.chromium.org/developers/testing/commit-queue http://codereview.chromium.org/7745005/diff/61027/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ...

9 years, 2 months ago (2011-09-26 17:03:27 UTC) #23

mtilburg

http://codereview.chromium.org/7745005/diff/61027/ppapi/proxy/ppb_flash_udp_socket_proxy.cc File ppapi/proxy/ppb_flash_udp_socket_proxy.cc (right): http://codereview.chromium.org/7745005/diff/61027/ppapi/proxy/ppb_flash_udp_socket_proxy.cc#newcode212 ppapi/proxy/ppb_flash_udp_socket_proxy.cc:212: if( closed_ ) On 2011/09/26 17:03:27, yzshen1 wrote: > ...

9 years, 2 months ago (2011-09-26 17:16:18 UTC) #24

commit-bot: I haz the power

CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/mtilburg@gmail.com/7745005/72001

9 years, 2 months ago (2011-09-26 17:56:16 UTC) #25

commit-bot: I haz the power

Presubmit check for 7745005-72001 failed and returned exit status 1. Running presubmit commit checks ...

9 years, 2 months ago (2011-09-26 17:56:30 UTC) #26

commit-bot: I haz the power

CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/mtilburg@gmail.com/7745005/72001

9 years, 2 months ago (2011-09-26 17:58:29 UTC) #28

commit-bot: I haz the power

Presubmit check for 7745005-72001 failed and returned exit status 1. Running presubmit commit checks ...

9 years, 2 months ago (2011-09-26 17:58:43 UTC) #29

brettw

We should add Adobe to the src/AUTHORS file since you guys have signed the CLA ...

9 years, 2 months ago (2011-09-26 18:11:33 UTC) #30

mtilburg

I spoke with our Legal, our agreement only includes these four engineers, and they do ...

9 years, 2 months ago (2011-09-26 19:13:26 UTC) #31

commit-bot: I haz the power

CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/mtilburg@gmail.com/7745005/72003

9 years, 2 months ago (2011-09-26 19:23:33 UTC) #33

commit-bot: I haz the power

Can't apply patch for file AUTHORS. While running patch -p1 --forward --force; patching file AUTHORS ...

9 years, 2 months ago (2011-09-26 19:23:43 UTC) #34

M-A Ruel

On 2011/09/26 19:23:43, I haz the power (commit-bot) wrote: > Can't apply patch for file ...

9 years, 2 months ago (2011-09-26 19:24:50 UTC) #35

M-A Ruel

9 years, 2 months ago (2011-09-26 19:40:31 UTC) #37

On 2011/09/26 19:24:50, Marc-Antoine Ruel wrote:
> Please sync past revision 102622 first.

(But even with that, the patch will fail since the email address you used to
upload it not the one listed in AUTHORS. The check is strict about that.

Choices are:
- You upload a new issue with a Google credential with the address specified in
AUTHORS. You can create a Google account is an arbitrary email address as long
as you are not using Google Apps. Since you are using git, it's "git cl issue 0;
rm ~/codereview_upload_cookies; git cl upload".
- You let Brett commit it on your behalf.

Expand Messages | Collapse Messages