Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880613005:
De-Clientize UDPSocket service (Closed)
|
Created:
5 years, 11 months ago by Ken Rockot(use gerrit already) Modified:
5 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@rollin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDe-Clientize UDPSocket service
BUG=451321
Committed: https://crrev.com/2b400c191f78c67e75e84a4bee81e41c0e9eb6b1
Cr-Commit-Position: refs/heads/master@{#313984}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 3
Patch Set 3 : so long, SetReceiver #Patch Set 4 : docs #Patch Set 5 : recovered a lost word #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 42 (4 generated)
rockot@chromium.org changed reviewers: + yzshen@chromium.org
Hi Yuzhu, could you please take a look? This is basically the interface we discussed earlier. I adopted your naming (i.e. Receiver instead of Client).
https://codereview.chromium.org/880613005/diff/1/mojo/services/network/networ... File mojo/services/network/network_service_impl.cc (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/networ... mojo/services/network/network_service_impl.cc:70: // The lifetime of this UDPSocketImpl is bound to that of the underlying pipe. It is nice to add this comment to udp_socket_impl.h, too. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... File mojo/services/network/public/cpp/udp_socket_wrapper.h (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/cpp/udp_socket_wrapper.h:133: Binding<UDPSocketReceiver> binding_; Please add include for Binding<>. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... File mojo/services/network/public/interfaces/udp_socket.mojom (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/interfaces/udp_socket.mojom:11: // typical flow of using the interfaces is: Could you please update the comment to: - add a step of SetReceiver - update the name of UDPSocketClient https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/interfaces/udp_socket.mojom:67: // Correspondingly, OnReceived() of the UDPSocketClient interface will be Please update the comment accordingly https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... File mojo/services/network/udp_socket_impl.cc (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.cc:221: receiver_ = receiver.Pass(); I believe this line works even if |receiver| is null. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.cc:348: if (receiver_) { Before |receiver_| is set or when it is reset to null, I think we should hold off receiving packets, instead of receiving and discarding, right? https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... File mojo/services/network/udp_socket_impl.h (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.h:27: UDPSocketImpl(InterfaceRequest<UDPSocket> request); explicit, please.
Thanks! https://codereview.chromium.org/880613005/diff/1/mojo/services/network/networ... File mojo/services/network/network_service_impl.cc (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/networ... mojo/services/network/network_service_impl.cc:70: // The lifetime of this UDPSocketImpl is bound to that of the underlying pipe. On 2015/01/27 22:18:24, yzshen1 wrote: > It is nice to add this comment to udp_socket_impl.h, too. Done. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... File mojo/services/network/public/cpp/udp_socket_wrapper.h (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/cpp/udp_socket_wrapper.h:133: Binding<UDPSocketReceiver> binding_; On 2015/01/27 22:18:24, yzshen1 wrote: > Please add include for Binding<>. Done. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... File mojo/services/network/public/interfaces/udp_socket.mojom (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/interfaces/udp_socket.mojom:11: // typical flow of using the interfaces is: On 2015/01/27 22:18:24, yzshen1 wrote: > Could you please update the comment to: > - add a step of SetReceiver > - update the name of UDPSocketClient Done. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/public... mojo/services/network/public/interfaces/udp_socket.mojom:67: // Correspondingly, OnReceived() of the UDPSocketClient interface will be On 2015/01/27 22:18:24, yzshen1 wrote: > Please update the comment accordingly Done. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... File mojo/services/network/udp_socket_impl.cc (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.cc:221: receiver_ = receiver.Pass(); On 2015/01/27 22:18:24, yzshen1 wrote: > I believe this line works even if |receiver| is null. Ahhhh. I was interpreting the Pass implementation incorrectly. You're right of course, this is fine when null. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.cc:348: if (receiver_) { On 2015/01/27 22:18:24, yzshen1 wrote: > Before |receiver_| is set or when it is reset to null, I think we should hold > off receiving packets, instead of receiving and discarding, right? Yeah I agree. I convinced myself otherwise earlier, but retaining a pending recvfrom until a receiver is available does not really add much complexity. Done. https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... File mojo/services/network/udp_socket_impl.h (right): https://codereview.chromium.org/880613005/diff/1/mojo/services/network/udp_so... mojo/services/network/udp_socket_impl.h:27: UDPSocketImpl(InterfaceRequest<UDPSocket> request); On 2015/01/27 22:18:24, yzshen1 wrote: > explicit, please. Done.
LGTM Thanks! https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/ud... File mojo/services/network/udp_socket_impl.cc (right): https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/ud... mojo/services/network/udp_socket_impl.cc:337: // state, preventing additional RecvFroms as well. This is a pretty clever way. :)
rockot@chromium.org changed reviewers: + jamesr@chromium.org
James could you take a look as owner? Also, do we have a bug for tracking Client= removal in chromium? I found lots of candidates, but it's unclear to me if they were intended to track mojo changes only.
Does the receiver connection need to be this flexible, or should it be tied to the socket itself? https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/pu... File mojo/services/network/public/cpp/udp_socket_wrapper.cc (right): https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/pu... mojo/services/network/public/cpp/udp_socket_wrapper.cc:165: binding_.Bind(&receiver_proxy); this isn't wrong, but it's a bit confusing (imho). i prefer always associating Binding<>s with InterfaceRequest<>s i.e. UDPSocketReceiverPtr receiver; binding_.Bind(GetProxy(&receiver)); // <-- this creates the message pipe etc and returns an InterfaceRequest<UDPSocketReciever> socket_->SetReceiver(receiver.Pass()); https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/pu... File mojo/services/network/public/interfaces/udp_socket.mojom (right): https://codereview.chromium.org/880613005/diff/20001/mojo/services/network/pu... mojo/services/network/public/interfaces/udp_socket.mojom:62: // be set to null. Be aware that pending receive requests which occur between what does a null receiver mean?
On Wed, Jan 28, 2015 at 12:48 PM, <jamesr@chromium.org> wrote: > Does the receiver connection need to be this flexible, or should it be > tied to > the socket itself? > This way it is possible to: - use a socket interface to receive data - reset the receiver - forward the socket to someone else - [someone else] set a new receiver. It has more similar to what [Client=] can achieve. What do you think? > > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > services/network/public/cpp/udp_socket_wrapper.cc > File mojo/services/network/public/cpp/udp_socket_wrapper.cc (right): > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > services/network/public/cpp/udp_socket_wrapper.cc#newcode165 > mojo/services/network/public/cpp/udp_socket_wrapper.cc:165: > binding_.Bind(&receiver_proxy); > this isn't wrong, but it's a bit confusing (imho). i prefer always > associating Binding<>s with InterfaceRequest<>s i.e. > > UDPSocketReceiverPtr receiver; > binding_.Bind(GetProxy(&receiver)); // <-- this creates the message > pipe etc and returns an InterfaceRequest<UDPSocketReciever> > socket_->SetReceiver(receiver.Pass()); > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > services/network/public/interfaces/udp_socket.mojom > File mojo/services/network/public/interfaces/udp_socket.mojom (right): > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > services/network/public/interfaces/udp_socket.mojom#newcode62 > mojo/services/network/public/interfaces/udp_socket.mojom:62: // be set > to null. Be aware that pending receive requests which occur between > what does a null receiver mean? > > https://codereview.chromium.org/880613005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Couldn't you just pass the receiver around if you want to transfer it to someone else, or close the pipe if you aren't interested in new data? I can't figure out when passing a 'null' over would make sense.
On 2015/01/28 20:59:51, yzshen1 wrote: > On Wed, Jan 28, 2015 at 12:48 PM, <mailto:jamesr@chromium.org> wrote: > > > Does the receiver connection need to be this flexible, or should it be > > tied to > > the socket itself? > > > > This way it is possible to: > - use a socket interface to receive data > - reset the receiver > - forward the socket to someone else > - [someone else] set a new receiver. > > It has more similar to what [Client=] can achieve. > > What do you think? > > > > > > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > > services/network/public/cpp/udp_socket_wrapper.cc > > File mojo/services/network/public/cpp/udp_socket_wrapper.cc (right): > > > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > > services/network/public/cpp/udp_socket_wrapper.cc#newcode165 > > mojo/services/network/public/cpp/udp_socket_wrapper.cc:165: > > binding_.Bind(&receiver_proxy); > > this isn't wrong, but it's a bit confusing (imho). i prefer always > > associating Binding<>s with InterfaceRequest<>s i.e. > > > > UDPSocketReceiverPtr receiver; > > binding_.Bind(GetProxy(&receiver)); // <-- this creates the message > > pipe etc and returns an InterfaceRequest<UDPSocketReciever> > > socket_->SetReceiver(receiver.Pass()); > > > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > > services/network/public/interfaces/udp_socket.mojom > > File mojo/services/network/public/interfaces/udp_socket.mojom (right): > > > > https://codereview.chromium.org/880613005/diff/20001/mojo/ > > services/network/public/interfaces/udp_socket.mojom#newcode62 > > mojo/services/network/public/interfaces/udp_socket.mojom:62: // be set > > to null. Be aware that pending receive requests which occur between > > what does a null receiver mean? > > > > https://codereview.chromium.org/880613005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I don't see how we can avoid making the consumer provide a receiver impl if we want to avoid a round trip for every datagram received. Supporting null in SetReceiver seemed reasonable because it didn't add much complexity, and implementations would have already had to tolerate a receiverless existence due to their starting state. We could just roll the receiver provision into the args for ReceiveMore. Rather than: socket->SetReceiver(receiver); socket->ReceiveMore(1000); you write socket->ReceiveMore(1000, receiver); This would prevent swapping in a new receiver in the middle of a recv batch, but I can't think of a practical use for that ability anyway.
Setting 'null' and closing the receive pipe mean the same thing, don't they? Better to provide one way to do things - simple implementation and simpler interface for callers to use. Callers are expected to call ReceiveMore() many times on the same socket, right?
On 2015/01/28 21:56:45, jamesr wrote: > Setting 'null' and closing the receive pipe mean the same thing, don't they? > Better to provide one way to do things - simple implementation and simpler > interface for callers to use. > > Callers are expected to call ReceiveMore() many times on the same socket, right? Yes, they are
On 2015/01/28 21:44:34, jamesr wrote: > Couldn't you just pass the receiver around if you want to transfer it to someone > else, or close the pipe if you aren't interested in new data? I can't figure > out when passing a 'null' over would make sense. True that passing <socket, receiver> together works, too. I don't really have a strong opinion.
If you pass the receiver with the ReadMore() then you have to have a new pipe for each call, but maybe that's OK.
On 2015/01/28 22:15:55, jamesr wrote: > If you pass the receiver with the ReadMore() then you have to have a new pipe > for each call, but maybe that's OK. Right, that's something I'm not sure about. I'd rather not require a new pipe for each call if we can avoid it. I'm not seeing a good alternative to either this approach or the SetReceiver-style interface.
SetReceiver is OK if the same pipe needs to be reused across multiple reads, but it's a bit complicated. Passing a null value in just flat out does not make any sense. Changing the receiver seems pretty dubious as well - why not just pass the receiver handle around if someone wants to change what receives the stream. I'd say have SetReceiver be callable exactly once and not be nullable. If we need to set a different receiver (why?) then say that it's callable multiple times, but never with null. If a particular receiver doesn't want to receive bytes any more it can close its pipe.
On 2015/01/28 22:37:06, jamesr wrote: > SetReceiver is OK if the same pipe needs to be reused across multiple reads, but > it's a bit complicated. Passing a null value in just flat out does not make any > sense. Changing the receiver seems pretty dubious as well - why not just pass > the receiver handle around if someone wants to change what receives the stream. > > I'd say have SetReceiver be callable exactly once and not be nullable. If we > need to set a different receiver (why?) then say that it's callable multiple > times, but never with null. If a particular receiver doesn't want to receive > bytes any more it can close its pipe. I am fine with this. I think the idea of passing a new pipe for each ReceiveMore() is costly and hard to use. I hope we don't go down that direction. :/ (The current udp wrapper calls ReceiveMore(1) to ask for one more packet everytime it has a slot to store it.)
OK. I agree nullability is essentially useless so I'll at least remove that. One more consideration here: What if Bind and Connect both replied with a UDPSocketReceiver& that the consumer could bind On Wed, Jan 28, 2015 at 2:42 PM, <yzshen@chromium.org> wrote: > On 2015/01/28 22:37:06, jamesr wrote: > >> SetReceiver is OK if the same pipe needs to be reused across multiple >> reads, >> > but > >> it's a bit complicated. Passing a null value in just flat out does not >> make >> > any > >> sense. Changing the receiver seems pretty dubious as well - why not just >> pass >> the receiver handle around if someone wants to change what receives the >> > stream. > > I'd say have SetReceiver be callable exactly once and not be nullable. >> If we >> need to set a different receiver (why?) then say that it's callable >> multiple >> times, but never with null. If a particular receiver doesn't want to >> receive >> bytes any more it can close its pipe. >> > > I am fine with this. > > I think the idea of passing a new pipe for each ReceiveMore() is costly > and hard > to use. I hope we don't go down that direction. :/ > (The current udp wrapper calls ReceiveMore(1) to ask for one more packet > everytime it has a slot to store it.) > > > https://codereview.chromium.org/880613005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(and then SetReceiver goes away too) On Wed, Jan 28, 2015 at 2:44 PM, Ken Rockot <rockot@chromium.org> wrote: > OK. I agree nullability is essentially useless so I'll at least remove > that. > > One more consideration here: What if Bind and Connect both replied with a > UDPSocketReceiver& that the consumer could bind > > On Wed, Jan 28, 2015 at 2:42 PM, <yzshen@chromium.org> wrote: > >> On 2015/01/28 22:37:06, jamesr wrote: >> >>> SetReceiver is OK if the same pipe needs to be reused across multiple >>> reads, >>> >> but >> >>> it's a bit complicated. Passing a null value in just flat out does not >>> make >>> >> any >> >>> sense. Changing the receiver seems pretty dubious as well - why not >>> just pass >>> the receiver handle around if someone wants to change what receives the >>> >> stream. >> >> I'd say have SetReceiver be callable exactly once and not be nullable. >>> If we >>> need to set a different receiver (why?) then say that it's callable >>> multiple >>> times, but never with null. If a particular receiver doesn't want to >>> receive >>> bytes any more it can close its pipe. >>> >> >> I am fine with this. >> >> I think the idea of passing a new pipe for each ReceiveMore() is costly >> and hard >> to use. I hope we don't go down that direction. :/ >> (The current udp wrapper calls ReceiveMore(1) to ask for one more packet >> everytime it has a slot to store it.) >> >> >> https://codereview.chromium.org/880613005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ooo, there's an idea. So the usage would be you get a UDPSocket interface then call either Bind() or Connect(), get a Receiver, then call ReceiveMore() to get more bytes? I like that best of all. You could close the pipe if you didn't want to receive anything more.
On 2015/01/28 23:04:55, jamesr wrote: > Ooo, there's an idea. So the usage would be you get a UDPSocket interface then > call either Bind() or Connect(), get a Receiver, then call ReceiveMore() to get > more bytes? I like that best of all. You could close the pipe if you didn't > want to receive anything more. SGTM too
There we go.
Update the interface comment in the mojom too (can't figure out how to content inline on mobile) looks really nice to me
Done On Wed, Jan 28, 2015 at 6:25 PM, <jamesr@chromium.org> wrote: > Update the interface comment in the mojom too (can't figure out how to > content > inline on mobile) looks really nice to me > > https://codereview.chromium.org/880613005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/29 02:32:10, Ken Rockot wrote: > Done > > On Wed, Jan 28, 2015 at 6:25 PM, <mailto:jamesr@chromium.org> wrote: > > > Update the interface comment in the mojom too (can't figure out how to > > content > > inline on mobile) looks really nice to me > > > > https://codereview.chromium.org/880613005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. at the risk of burning a few more cycles here, now that i've done this i'm starting to think it'd be a nicer interface if Bind and Connect just took receiver ptrs rather than returning receiver requests. -_- what do you guys think?
Hmm, means that a pipe is allocated even if the bind/connect calls are doomed but it might be nice for callers. They'll have to handle the failure car anyway, though.
True, you do have to handle failure anyway so the difference in usage would be marginal. Current implementation has the bonus of delayed pipe allocation. OK, leaving as is. On Wed, Jan 28, 2015 at 7:44 PM, <jamesr@chromium.org> wrote: > Hmm, means that a pipe is allocated even if the bind/connect calls are > doomed > but it might be nice for callers. They'll have to handle the failure car > anyway, > though. > > https://codereview.chromium.org/880613005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/29 05:04:10, Ken Rockot wrote: > True, you do have to handle failure anyway so the difference in usage would > be marginal. Current implementation has the bonus of delayed pipe > allocation. > > OK, leaving as is. > > On Wed, Jan 28, 2015 at 7:44 PM, <mailto:jamesr@chromium.org> wrote: > > > Hmm, means that a pipe is allocated even if the bind/connect calls are > > doomed > > but it might be nice for callers. They'll have to handle the failure car > > anyway, > > though. > > > > https://codereview.chromium.org/880613005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. For the record, I like this new idea better. It feels more natural. (Actually, I probably have misread your previous proposal. I didn't realize that you were proposing to return an interface request in the callback.)
mojom lgtm. I have some questions about the impl, but I may just be out of date on this code. https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... File mojo/services/network/public/cpp/udp_socket_wrapper.h (right): https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... mojo/services/network/public/cpp/udp_socket_wrapper.h:27: typedef Callback< the bindings now generate a typedef for these callbacks that matches the mojom (thanks to Yuzhu), although I'm not sure if we've rolled that in yet. Maybe leave a TODO here to use those? either way, use the 'using ...Callback = Callback<...>;' syntax since that's recommended for new type aliases and (imo) easier to read https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... mojo/services/network/public/cpp/udp_socket_wrapper.h:105: class ReceiverBindingCallback : public BindOrConnectCallback::Runnable { why don't you use base::Bind to generate this type?
On 2015/01/29 19:30:39, jamesr wrote: > mojom lgtm. I have some questions about the impl, but I may just be out of date > on this code. > > https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... > File mojo/services/network/public/cpp/udp_socket_wrapper.h (right): > > https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... > mojo/services/network/public/cpp/udp_socket_wrapper.h:27: typedef Callback< > the bindings now generate a typedef for these callbacks that matches the mojom > (thanks to Yuzhu), although I'm not sure if we've rolled that in yet. Maybe > leave a TODO here to use those? > > either way, use the 'using ...Callback = Callback<...>;' syntax since that's > recommended for new type aliases and (imo) easier to read > > https://codereview.chromium.org/880613005/diff/80001/mojo/services/network/pu... > mojo/services/network/public/cpp/udp_socket_wrapper.h:105: class > ReceiverBindingCallback : public BindOrConnectCallback::Runnable { > why don't you use base::Bind to generate this type? services/network/public forbids dependencies on base
Ah right, good point.
In any case, updated the type aliases. Yuzhu, I am sympathetic to your preference for having the client pass receivers to Bind()/Connect(), but I'm ambivalent on the decision to change it. How strongly do you feel about this?
Patchset #6 (id:100001) has been deleted
(Discussed with Darin, James and Trung.) I think I agree with Darin's suggestion to: - eliminate Receiver and ReceiveMore(); - have a Receive() method that returns one packet, and allow multiple inflight requests. Ken: I am fine if you would like to land this patch as-is and let me take over to make further changes. (Of course, you are welcomed to continue the work if you want to.) On Thu, Jan 29, 2015 at 3:13 PM, <rockot@chromium.org> wrote: > In any case, updated the type aliases. > > Yuzhu, I am sympathetic to your preference for having the client pass > receivers > to Bind()/Connect(), but I'm ambivalent on the decision to change it. How > strongly do you feel about this? > > https://codereview.chromium.org/880613005/ > -- Best regards, Yuzhu Shen. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Send again using correct account... (Discussed with Darin, James and Trung.) I think I agree with Darin's suggestion to: - eliminate Receiver and ReceiveMore(); - have a Receive() method that returns one packet, and allow multiple inflight requests. Ken: I am fine if you would like to land this patch as-is and let me take over to make further changes. (Of course, you are welcomed to continue the work if you want to.) On Thu, Jan 29, 2015 at 3:13 PM, <rockot@chromium.org> wrote: > In any case, updated the type aliases. > > Yuzhu, I am sympathetic to your preference for having the client pass > receivers > to Bind()/Connect(), but I'm ambivalent on the decision to change it. How > strongly do you feel about this? > > https://codereview.chromium.org/880613005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah, I see. We'd have a full round trip per packet, but that's really fine if requests can be batched. SGTM. I'll land this as-is and defer to you from here. Thanks! On Fri, Jan 30, 2015 at 11:52 AM, Yuzhu Shen <yzshen@chromium.org> wrote: > Send again using correct account... > > (Discussed with Darin, James and Trung.) > > I think I agree with Darin's suggestion to: > - eliminate Receiver and ReceiveMore(); > - have a Receive() method that returns one packet, and allow multiple > inflight requests. > > Ken: I am fine if you would like to land this patch as-is and let me take > over to make further changes. (Of course, you are welcomed to continue the > work if you want to.) > > On Thu, Jan 29, 2015 at 3:13 PM, <rockot@chromium.org> wrote: > >> In any case, updated the type aliases. >> >> Yuzhu, I am sympathetic to your preference for having the client pass >> receivers >> to Bind()/Connect(), but I'm ambivalent on the decision to change it. How >> strongly do you feel about this? >> >> https://codereview.chromium.org/880613005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880613005/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2b400c191f78c67e75e84a4bee81e41c0e9eb6b1 Cr-Commit-Position: refs/heads/master@{#313984} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
