Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3352013:
Modify Chromotocol to have a new RectangleUpdatePacket. (Closed)
|
Created:
10 years, 3 months ago by awong Modified:
9 years, 7 months ago CC:
chromium-reviews, Sergey Ulanov, dmac, awong, garykac Visibility:
Public. |
DescriptionModify Chromotocol to have a new RectangleUpdatePacket.
This is a less nested version of UpdateStreamMessage. It will replace after
a few more CLs are checked in.
BUG=52833
TEST=none. adding new messages doesn't change functionality.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60360
Patch Set 1 #
Total comments: 18
Patch Set 2 : fix comments #Messages
Total messages: 7 (0 generated)
Breaking apart the huge CL into smaller chunks.
Nits. LGTM http://codereview.chromium.org/3352013/diff/1/2 File remoting/base/protocol/chromotocol.proto (right): http://codereview.chromium.org/3352013/diff/1/2#newcode99 remoting/base/protocol/chromotocol.proto:99: // TODO(ajwong): Determine if these fields should be optional or required. Having x,y,width,height required and the rest optional seems right to me. Also, a "// NEXT ID: 7" would be nice here. http://codereview.chromium.org/3352013/diff/1/2#newcode121 remoting/base/protocol/chromotocol.proto:121: optional bool is_rect_end = 2 [default = false]; Either renumber the fields to be '1' and '2', or change the NEXT ID comment to be 4 http://codereview.chromium.org/3352013/diff/1/2#newcode128 remoting/base/protocol/chromotocol.proto:128: message RectangleUpdatePacket { // NEXT ID: 6. Unless you didn't mean to skip ID 4 below. http://codereview.chromium.org/3352013/diff/1/2#newcode142 remoting/base/protocol/chromotocol.proto:142: // packets may not be skipped. Nice comment! http://codereview.chromium.org/3352013/diff/1/2#newcode156 remoting/base/protocol/chromotocol.proto:156: optional bytes encoded_rect = 2; Does the 'encoded_rect' need to always be the last field? If so, can we have a comment to that effect so that new fields are added before this?
http://codereview.chromium.org/3352013/diff/1/2 File remoting/base/protocol/chromotocol.proto (right): http://codereview.chromium.org/3352013/diff/1/2#newcode119 remoting/base/protocol/chromotocol.proto:119: message RectangleData { why do we have this and RectangleUpdatePacket? Seems redundant. http://codereview.chromium.org/3352013/diff/1/2#newcode121 remoting/base/protocol/chromotocol.proto:121: optional bool is_rect_end = 2 [default = false]; this should be 1. http://codereview.chromium.org/3352013/diff/1/2#newcode124 remoting/base/protocol/chromotocol.proto:124: required bytes encoded_data = 3; this should be 2. http://codereview.chromium.org/3352013/diff/1/2#newcode147 remoting/base/protocol/chromotocol.proto:147: optional int32 flags = 1 [default = 0]; Why don't we just use bool instead of a int32? http://codereview.chromium.org/3352013/diff/1/2#newcode156 remoting/base/protocol/chromotocol.proto:156: optional bytes encoded_rect = 2; Do you mean RectangleData?
fixed. http://codereview.chromium.org/3352013/diff/1/2 File remoting/base/protocol/chromotocol.proto (right): http://codereview.chromium.org/3352013/diff/1/2#newcode99 remoting/base/protocol/chromotocol.proto:99: // TODO(ajwong): Determine if these fields should be optional or required. On 2010/09/08 18:21:32, garykac wrote: > Having x,y,width,height required and the rest optional seems right to me. > > Also, a "// NEXT ID: 7" would be nice here. I'm just scared about having any fields be required inside the network protocol because then the protocol buffer decode logic just aborts on a failure. This is not cool. As for the NEXT ID things, I actually think we should avoid doing that...it's easy to get out of sync and add revision control noise. http://codereview.chromium.org/3352013/diff/1/2#newcode119 remoting/base/protocol/chromotocol.proto:119: message RectangleData { On 2010/09/08 19:06:55, Alpha wrote: > why do we have this and RectangleUpdatePacket? Seems redundant. > Eek...I meant for this to be deleted. It's gone. http://codereview.chromium.org/3352013/diff/1/2#newcode128 remoting/base/protocol/chromotocol.proto:128: message RectangleUpdatePacket { On 2010/09/08 18:21:32, garykac wrote: > // NEXT ID: 6. > > Unless you didn't mean to skip ID 4 below. I'm not being strict with the ID numbers for now since we'll re tag all the fields after the protocol stablizes. But I fixed the number since I was looking at it again. http://codereview.chromium.org/3352013/diff/1/2#newcode142 remoting/base/protocol/chromotocol.proto:142: // packets may not be skipped. On 2010/09/08 18:21:32, garykac wrote: > Nice comment! Thanks :) http://codereview.chromium.org/3352013/diff/1/2#newcode147 remoting/base/protocol/chromotocol.proto:147: optional int32 flags = 1 [default = 0]; On 2010/09/08 19:06:55, Alpha wrote: > Why don't we just use bool instead of a int32? Because we can have multiple flags, and a bool is one byte on the wire where what we're doing is really closer to a bunch of options. A bitfield felt more appropriate. If you disagree, let me know. http://codereview.chromium.org/3352013/diff/1/2#newcode156 remoting/base/protocol/chromotocol.proto:156: optional bytes encoded_rect = 2; On 2010/09/08 18:21:32, garykac wrote: > Does the 'encoded_rect' need to always be the last field? If so, can we have a > comment to that effect so that new fields are added before this? No it doesn't need to be the last field. Protocol buffers are unordered. This just ended up here. :) http://codereview.chromium.org/3352013/diff/1/2#newcode156 remoting/base/protocol/chromotocol.proto:156: optional bytes encoded_rect = 2; On 2010/09/08 19:06:55, Alpha wrote: > Do you mean RectangleData? I got rid of RectangleData.
http://codereview.chromium.org/3352013/diff/1/2 File remoting/base/protocol/chromotocol.proto (right): http://codereview.chromium.org/3352013/diff/1/2#newcode99 remoting/base/protocol/chromotocol.proto:99: // TODO(ajwong): Determine if these fields should be optional or required. On 2010/09/08 19:39:14, awong wrote: > On 2010/09/08 18:21:32, garykac wrote: > > Having x,y,width,height required and the rest optional seems right to me. > > > > Also, a "// NEXT ID: 7" would be nice here. > > I'm just scared about having any fields be required inside the network protocol > because then the protocol buffer decode logic just aborts on a failure. This is > not cool. The network code should handle proto decode failures in any case. Missing 'required' params isn't the only failure case. I think it's fine the way it is, but you can make everything 'optional' if you prefer that. > As for the NEXT ID things, I actually think we should avoid doing that...it's > easy to get out of sync and add revision control noise. The NEXT ID info is *incredibly* useful for large protocol buffers, since people often add new fields near related fields rather than at the end. But these protobufs are small, so it's not a big deal. We can add them when the messages get large.
On Wed, Sep 8, 2010 at 1:31 PM, <garykac@chromium.org> wrote: > > http://codereview.chromium.org/3352013/diff/1/2 > File remoting/base/protocol/chromotocol.proto (right): > > http://codereview.chromium.org/3352013/diff/1/2#newcode99 > remoting/base/protocol/chromotocol.proto:99: // TODO(ajwong): Determine > if these fields should be optional or required. > On 2010/09/08 19:39:14, awong wrote: > >> On 2010/09/08 18:21:32, garykac wrote: >> > Having x,y,width,height required and the rest optional seems right >> > to me. > >> > >> > Also, a "// NEXT ID: 7" would be nice here. >> > > I'm just scared about having any fields be required inside the network >> > protocol > >> because then the protocol buffer decode logic just aborts on a >> > failure. This is > >> not cool. >> > > The network code should handle proto decode failures in any case. > Missing 'required' params isn't the only failure case. > > I think it's fine the way it is, but you can make everything 'optional' > if you prefer that. The thing with required that I've hit before is that the network layer ends up being hamstrung at trying to detect and gracefully handle a corrupt protocol buffer because the actual generated parsing code errors out hard (by killing the process) rather than returning "oh, something went wrong" if a require field is missing. Also, it makes it hard to handle multiple versions of the protocol if somehow in the future, we end up not needing these fields. But I haven't really thought it all the way through...hence the TODO. > As for the NEXT ID things, I actually think we should avoid doing >> > that...it's > >> easy to get out of sync and add revision control noise. >> > > The NEXT ID info is *incredibly* useful for large protocol buffers, > since people often add new fields near related fields rather than at the > end. > > But these protobufs are small, so it's not a big deal. We can add them > when the messages get large. Ah. Good point. I'd rather leave them out for now until we need them. There's enough stuff fluxing that we're just end up changing these comments a lot.
LGTM. |
