Used oneof in blimp_message.proto

Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
Committed: https://crrev.com/eb552d3ed748c438f7861ec9b75ec30d404a7ac3
Cr-Commit-Position: refs/heads/master@{#395243}

[shaktisahu, 2016-04-29 18:45:23]

shaktisahu@chromium.org changed reviewers:
+ nyquist@chromium.org

[shaktisahu, 2016-04-29 18:45:23]

Hi,

Please take a look if this is in the right direction. I haven't added oneof to
the other .proto files such as input.proto, layer.proto.

Thanks
Shakti

[Kevin M, 2016-05-02 17:47:46]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-05-02 17:47:46]

kmarshall@chromium.org changed required reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-05-02 17:47:48]

Cool!

Adding myself as a mandatory reviewer because this heavily impacts code that the
engine team is responsible for.

In general, comparing the "case" field for oneofs is better than checking
has_xxx(), because the assertion errors will dump the actual value. Simply
checking the value of has_xxx() is uninformative - you only know that the field
wasn't there; you don't know what type it was.

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/ime_fe...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/ime_fe...
blimp/client/feature/ime_feature.cc:38: DCHECK(message->has_ime());
Check case

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/naviga...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/naviga...
blimp/client/feature/navigation_feature.cc:105:
DCHECK(message->has_navigation());
Check case

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/render...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/render...
blimp/client/feature/render_widget_feature.cc:86:
DCHECK(message->has_render_widget() || message->has_compositor());
Check case

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.cc:120: void SetBlimpMessageType(BlimpMessage*
message,
The body of this function is trivial enough that I'm not sure if this is saving
the caller any effort. I would recommend not using a helper function.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.h (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.h:70: BLIMP_COMMON_EXPORT void
SetBlimpMessageType(BlimpMessage* message,
I don't think we should have a function like this, but anyways, it doesn't fit
with the "create message" theme of this helper library.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message_unittest.cc:85:
EXPECT_TRUE(message->has_protocol_control());
EXPECT_EQ(kCase, ...)

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging.cc#new...
blimp/common/logging.cc:279: BlimpMessage::FeatureCase type,
I'm no naming savant but it seems like a mishmash to have a "case" called
"type". We might want to switch away from the "type" parameter name in favor of
something else? feature_case...?

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
File blimp/common/logging_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:104: base_msg.mutable_navigation();
Why do this? |navigation| is already being created on line 108.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:179: base_msg.mutable_protocol_control();
Ditto

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:209:
base_msg.mutable_render_widget()->set_render_widget_id(123);
Ditto

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:446: if
(message->has_tab_control()) {
Check the case instead

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:463: } else if
(message->has_navigation() && web_contents_) {
ditto

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_che...
File blimp/net/blimp_message_checkpointer.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_che...
blimp/net/blimp_message_checkpointer.cc:38: if (message->has_protocol_control()
&&
Check the case

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_out...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_out...
blimp/net/blimp_message_output_buffer.cc:137: DVLOG(3) << "Writing message (id="
Just log *message_to_write to the stream

https://codereview.chromium.org/1933053003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:114: if
(message->has_protocol_control() &&
Check case

https://codereview.chromium.org/1933053003/diff/1/blimp/net/thread_pipe_manag...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/thread_pipe_manag...
blimp/net/thread_pipe_manager_unittest.cc:5: #include
"blimp/common/create_blimp_message.h"
Move this to the second block of includes; this top include is reserved
exclusively for thread_pipe_manager.h

[shaktisahu, 2016-05-16 18:26:39]

Description was changed from

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
==========

to

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
==========

[shaktisahu, 2016-05-16 18:26:39]

shaktisahu@chromium.org changed reviewers:
+ wez@chromium.org

[shaktisahu, 2016-05-16 20:19:03]

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/ime_fe...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/ime_fe...
blimp/client/feature/ime_feature.cc:38: DCHECK(message->has_ime());
On 2016/05/02 17:47:47, Kevin M wrote:
> Check case

I agree, it gives more information. Although it looks cleaner than 
  DCHECK_EQ(BlimpMessage::kCompositor, message->feature_case());

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/naviga...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/naviga...
blimp/client/feature/navigation_feature.cc:105:
DCHECK(message->has_navigation());
On 2016/05/02 17:47:47, Kevin M wrote:
> Check case

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/render...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/client/feature/render...
blimp/client/feature/render_widget_feature.cc:86:
DCHECK(message->has_render_widget() || message->has_compositor());
On 2016/05/02 17:47:47, Kevin M wrote:
> Check case

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.cc:120: void SetBlimpMessageType(BlimpMessage*
message,
On 2016/05/02 17:47:48, Kevin M wrote:
> The body of this function is trivial enough that I'm not sure if this is
saving
> the caller any effort. I would recommend not using a helper function.

I agree that it looks a little weird. However, I still need this function in two
places, (blimp/net/blimp_message_multiplexer.cc and
blimp/net/thread_pipe_manager_unittest.cc)
Let me know if you strongly feel about it.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.h (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.h:70: BLIMP_COMMON_EXPORT void
SetBlimpMessageType(BlimpMessage* message,
On 2016/05/02 17:47:48, Kevin M wrote:
> I don't think we should have a function like this, but anyways, it doesn't fit
> with the "create message" theme of this helper library.

See my comment in the .cc file

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message_unittest.cc:85:
EXPECT_TRUE(message->has_protocol_control());
On 2016/05/02 17:47:48, Kevin M wrote:
> EXPECT_EQ(kCase, ...)

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging.cc#new...
blimp/common/logging.cc:279: BlimpMessage::FeatureCase type,
On 2016/05/02 17:47:48, Kevin M wrote:
> I'm no naming savant but it seems like a mishmash to have a "case" called
> "type". We might want to switch away from the "type" parameter name in favor
of
> something else? feature_case...? 

hmm.. I am open for suggestions. ++wez@
By the way, the 'Case' suffix of FeatureCase was automatically added by the
proto complier to the union name which I named as 'feature'. We can rename
'feature', but cant rid of 'case'

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
File blimp/common/logging_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:104: base_msg.mutable_navigation();
On 2016/05/02 17:47:48, Kevin M wrote:
> Why do this? |navigation| is already being created on line 108.

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:179: base_msg.mutable_protocol_control();
On 2016/05/02 17:47:48, Kevin M wrote:
> Ditto

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/common/logging_unitte...
blimp/common/logging_unittest.cc:209:
base_msg.mutable_render_widget()->set_render_widget_id(123);
On 2016/05/02 17:47:48, Kevin M wrote:
> Ditto

Not here I think.

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:446: if
(message->has_tab_control()) {
On 2016/05/02 17:47:48, Kevin M wrote:
> Check the case instead

I added it in the DCHECK above

https://codereview.chromium.org/1933053003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:463: } else if
(message->has_navigation() && web_contents_) {
On 2016/05/02 17:47:48, Kevin M wrote:
> ditto

Same as above.

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_che...
File blimp/net/blimp_message_checkpointer.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_che...
blimp/net/blimp_message_checkpointer.cc:38: if (message->has_protocol_control()
&&
On 2016/05/02 17:47:48, Kevin M wrote:
> Check the case

I am checking the case it only in DCHECK

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_out...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/blimp_message_out...
blimp/net/blimp_message_output_buffer.cc:137: DVLOG(3) << "Writing message (id="
On 2016/05/02 17:47:48, Kevin M wrote:
> Just log *message_to_write to the stream

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/net/engine_authentica...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/engine_authentica...
blimp/net/engine_authentication_handler.cc:114: if
(message->has_protocol_control() &&
On 2016/05/02 17:47:48, Kevin M wrote:
> Check case

Done.

https://codereview.chromium.org/1933053003/diff/1/blimp/net/thread_pipe_manag...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/1/blimp/net/thread_pipe_manag...
blimp/net/thread_pipe_manager_unittest.cc:5: #include
"blimp/common/create_blimp_message.h"
On 2016/05/02 17:47:48, Kevin M wrote:
> Move this to the second block of includes; this top include is reserved
> exclusively for thread_pipe_manager.h

Done.

[Kevin M, 2016-05-16 22:58:02]

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:47: SetBlimpMessageType(message.get(),
type_);
We should probably standardize on what message format is acceptable. Instead of
coercing a message to the appropriate type, can we just DCHECK if the
message->feature_case() == type_ and require that callers only provide well
formed messages?

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_pump_unittest.cc:34: : message1_(new BlimpMessage),
message2_(new BlimpMessage) {
Use CreateBlimpMessage() here to create valid input/tab control messages.

[shaktisahu, 2016-05-17 01:21:32]

PTAL

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:47: SetBlimpMessageType(message.get(),
type_);
On 2016/05/16 22:58:02, Kevin M wrote:
> We should probably standardize on what message format is acceptable. Instead
of
> coercing a message to the appropriate type, can we just DCHECK if the
> message->feature_case() == type_ and require that callers only provide well
> formed messages?

Done.

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/20001/blimp/net/blimp_message...
blimp/net/blimp_message_pump_unittest.cc:34: : message1_(new BlimpMessage),
message2_(new BlimpMessage) {
On 2016/05/16 22:58:02, Kevin M wrote:
> Use CreateBlimpMessage() here to create valid input/tab control messages.

Done.

[Wez, 2016-05-18 01:53:32]

Why are we not updating tab-control and protocol-control?

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/im...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/im...
blimp/client/feature/ime_feature.cc:39: DCHECK_EQ(BlimpMessage::kIme,
message->feature_case());
Wouldn't it be cleaner to DCHECK(message->has_ime()), and similarly across the
other files?

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/re...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/re...
blimp/client/feature/render_widget_feature.cc:87: BlimpMessage::kCompositor ==
message->feature_case());
Suggest DCHECK(has_render_widget() || has_compositor()) here.

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/create_bli...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/create_bli...
blimp/common/create_blimp_message.cc:22: DCHECK(compositor_message);
nit: While you're here, can we remove these DCHECKs, please, since these
functions will crash when we try to assign to *foo_message if foo_message is
null?

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/blim...
File blimp/common/proto/blimp_message.proto (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/blim...
blimp/common/proto/blimp_message.proto:56: TabControlMessage tab_control = 1000;
nit: While we're here, we may as well reduce these from the 1000s, since having
such large ordinals just bloats the protobuf for no good reason - we might start
them at e.g. 40, since we're unlikely to have >40 other fields at this level!

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
File blimp/common/proto/tab_control.proto (left):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
blimp/common/proto/tab_control.proto:38: // crbug.com/570371.
Why are we removing this comment, since this proto still doesn't use oneof yet?

https://codereview.chromium.org/1933053003/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_settings_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_settings_feature.cc:28:
DCHECK_EQ(BlimpMessage::kSettings, message->feature_case());
nit: DCHECK(has_settings())

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/blimp_message...
File blimp/net/blimp_message_demultiplexer_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer_unittest.cc:32: CompositorMessage*
compositor;
nit: Move this to appear immediately before CreateBlimpMessage.

Here and elsewhere, you should also be initializing these bare-pointers to
nullptr, even though they are about to get overwritten by the call.

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager_unittest.cc:156: TabControlMessage* tab_control;
nit: Move this to come immediately before tab_control_message.

[shaktisahu, 2016-05-18 03:55:42]

PTAL

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/im...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/im...
blimp/client/feature/ime_feature.cc:39: DCHECK_EQ(BlimpMessage::kIme,
message->feature_case());
On 2016/05/18 01:53:31, Wez wrote:
> Wouldn't it be cleaner to DCHECK(message->has_ime()), and similarly across the
> other files?


As we discussed, it is better to keep the DCHECK_EQ since it provides extra
information.

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/re...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/client/feature/re...
blimp/client/feature/render_widget_feature.cc:87: BlimpMessage::kCompositor ==
message->feature_case());
On 2016/05/18 01:53:31, Wez wrote:
> Suggest DCHECK(has_render_widget() || has_compositor()) here.

ditto

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/create_bli...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/create_bli...
blimp/common/create_blimp_message.cc:22: DCHECK(compositor_message);
On 2016/05/18 01:53:31, Wez wrote:
> nit: While you're here, can we remove these DCHECKs, please, since these
> functions will crash when we try to assign to *foo_message if foo_message is
> null?

Done.

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/blim...
File blimp/common/proto/blimp_message.proto (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/blim...
blimp/common/proto/blimp_message.proto:56: TabControlMessage tab_control = 1000;
On 2016/05/18 01:53:31, Wez wrote:
> nit: While we're here, we may as well reduce these from the 1000s, since
having
> such large ordinals just bloats the protobuf for no good reason - we might
start
> them at e.g. 40, since we're unlikely to have >40 other fields at this level!

Done.

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
File blimp/common/proto/tab_control.proto (left):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
blimp/common/proto/tab_control.proto:38: // crbug.com/570371.
On 2016/05/18 01:53:31, Wez wrote:
> Why are we removing this comment, since this proto still doesn't use oneof
yet?

There is only one field here. Do we still need a oneof?

https://codereview.chromium.org/1933053003/diff/40001/blimp/engine/feature/en...
File blimp/engine/feature/engine_settings_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/engine/feature/en...
blimp/engine/feature/engine_settings_feature.cc:28:
DCHECK_EQ(BlimpMessage::kSettings, message->feature_case());
On 2016/05/18 01:53:31, Wez wrote:
> nit: DCHECK(has_settings())

As we discussed, it is better to keep the DCHECK_EQ since it provides extra
information.

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/blimp_message...
File blimp/net/blimp_message_demultiplexer_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer_unittest.cc:32: CompositorMessage*
compositor;
On 2016/05/18 01:53:31, Wez wrote:
> nit: Move this to appear immediately before CreateBlimpMessage.
> 
> Here and elsewhere, you should also be initializing these bare-pointers to
> nullptr, even though they are about to get overwritten by the call.

Done.

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/40001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager_unittest.cc:156: TabControlMessage* tab_control;
On 2016/05/18 01:53:31, Wez wrote:
> nit: Move this to come immediately before tab_control_message.

Done.

[Kevin M, 2016-05-20 18:28:16]

There are still quite a few obsolete references to "type" and "|type|",
especially in parameter names and code comments. In addition to the items I
called out, please go through the affected files and find and fix any lingering
references to "type".

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/feature/re...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/feature/re...
blimp/client/feature/render_widget_feature.cc:102: NOTIMPLEMENTED();
NOTREACHED()

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:244: // Client will not send send
any RenderWidget messages, so don't save the
Move the HasSwitch code to the end of the function, so that it doesn't break up
the RegisterFeature() flow

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:461: if
(message->has_tab_control()) {
Check the feature case

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:478: } else if
(message->has_navigation() && web_contents_) {
Check the feature case

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_checkpointer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_checkpointer.cc:38: if (message->has_protocol_control())
{
Check the feature case

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_checkpointer.cc:39: if
(message->protocol_control().has_checkpoint_ack() &&
Check the case here too

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_demultiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer.cc:27: DLOG(FATAL) << "Handler already
registered for type=" << feature_case
"feature="

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:19: BlimpMessage::FeatureCase type);
update name

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:23: // |message.type|, if set, must match
the sender's type.
update comment

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:29: BlimpMessage::FeatureCase type_;
update name

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:36: BlimpMessage::FeatureCase type)
update param name

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.h:32: // Creates a BlimpMessageProcessor
object for sending messages of type |type|.
update documentation to reflect new naming

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.h:34: std::unique_ptr<BlimpMessageProcessor>
CreateSenderForType(
The name "ForType" doesn't work anymore, since we don't have a "type".

How about just CreateSender()?

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_output_buffer.cc:138: <<
write_buffer_.front()->message->message_id() << ", "
Nit: move the message_id logging portion into the common field extractors of
logging.cc so we are just logging *message_to_write.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:40: // specified. Only one handler may be
added per type.
"type" - update documentation

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:44: // Returns a BlimpMessageProcessor
object for sending messages of type
"of type feature_case" doesn't parse...

how about "sending messages for a given feature"?

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler_unittest.cc:51:
FakeFeature(BlimpMessage::FeatureCase type,
update param name

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/engine_authen...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/engine_authen...
blimp/net/engine_authentication_handler.cc:114: if
(message->has_protocol_control() &&
check cases, not has()

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager.h:44: // for sending messages of type |type|.
"type"... update comment

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager_unittest.cc:31:
FakeFeature(BlimpMessage::FeatureCase type,
"type" - here and elsewhere

[Wez, 2016-05-20 21:10:50]

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
File blimp/common/proto/tab_control.proto (left):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
blimp/common/proto/tab_control.proto:38: // crbug.com/570371.
On 2016/05/18 03:55:42, shaktisahu wrote:
> On 2016/05/18 01:53:31, Wez wrote:
> > Why are we removing this comment, since this proto still doesn't use oneof
> yet?
> 
> There is only one field here. Do we still need a oneof? 

Good question; I'd suggest that we have some sort of EmptyMessage, with no
fields, and have something like:

oneof {
  EmptyMessage create_tab
  EmptyMessage close_tab
  SizeMessage size
}

since there are actually three control messages in here, it's just that two of
them have no parameters!

[shaktisahu, 2016-05-20 22:29:42]

PTAL

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
File blimp/common/proto/tab_control.proto (left):

https://codereview.chromium.org/1933053003/diff/40001/blimp/common/proto/tab_...
blimp/common/proto/tab_control.proto:38: // crbug.com/570371.
On 2016/05/20 21:10:49, Wez wrote:
> On 2016/05/18 03:55:42, shaktisahu wrote:
> > On 2016/05/18 01:53:31, Wez wrote:
> > > Why are we removing this comment, since this proto still doesn't use oneof
> > yet?
> > 
> > There is only one field here. Do we still need a oneof? 
> 
> Good question; I'd suggest that we have some sort of EmptyMessage, with no
> fields, and have something like:
> 
> oneof {
>   EmptyMessage create_tab
>   EmptyMessage close_tab
>   SizeMessage size
> }
> 
> since there are actually three control messages in here, it's just that two of
> them have no parameters!

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/feature/re...
File blimp/client/feature/render_widget_feature.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/feature/re...
blimp/client/feature/render_widget_feature.cc:102: NOTIMPLEMENTED();
On 2016/05/20 18:28:15, Kevin M wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:244: // Client will not send send
any RenderWidget messages, so don't save the
On 2016/05/20 18:28:15, Kevin M wrote:
> Move the HasSwitch code to the end of the function, so that it doesn't break
up
> the RegisterFeature() flow

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:461: if
(message->has_tab_control()) {
On 2016/05/20 18:28:15, Kevin M wrote:
> Check the feature case

Why? We are already doing that in the DCHECK above. And it is much cleaner.

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:478: } else if
(message->has_navigation() && web_contents_) {
On 2016/05/20 18:28:15, Kevin M wrote:
> Check the feature case

Ditto

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_checkpointer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_checkpointer.cc:38: if (message->has_protocol_control())
{
On 2016/05/20 18:28:15, Kevin M wrote:
> Check the feature case

Why? We are already doing that in the DCHECK above. And it is much cleaner.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_checkpointer.cc:39: if
(message->protocol_control().has_checkpoint_ack() &&
On 2016/05/20 18:28:16, Kevin M wrote:
> Check the case here too

Ditto

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_demultiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer.cc:27: DLOG(FATAL) << "Handler already
registered for type=" << feature_case
On 2016/05/20 18:28:16, Kevin M wrote:
> "feature="

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:19: BlimpMessage::FeatureCase type);
On 2016/05/20 18:28:16, Kevin M wrote:
> update name

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:23: // |message.type|, if set, must match
the sender's type.
On 2016/05/20 18:28:16, Kevin M wrote:
> update comment

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:29: BlimpMessage::FeatureCase type_;
On 2016/05/20 18:28:16, Kevin M wrote:
> update name

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:36: BlimpMessage::FeatureCase type)
On 2016/05/20 18:28:16, Kevin M wrote:
> update param name

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.h:34: std::unique_ptr<BlimpMessageProcessor>
CreateSenderForType(
On 2016/05/20 18:28:16, Kevin M wrote:
> The name "ForType" doesn't work anymore, since we don't have a "type".
> 
> How about just CreateSender()?

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
File blimp/net/blimp_message_output_buffer.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/blimp_message...
blimp/net/blimp_message_output_buffer.cc:138: <<
write_buffer_.front()->message->message_id() << ", "
On 2016/05/20 18:28:16, Kevin M wrote:
> Nit: move the message_id logging portion into the common field extractors of
> logging.cc so we are just logging *message_to_write.

It might be too verbose in that case. I think we don't need the id everywhere to
be printed.
This is what it prints currently
BlimpMessage type=PROTOCOL_CONTROL subtype=CHECKPOINT_ACK checkpoint_id=1221117
byte_size=20

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:40: // specified. Only one handler may be
added per type.
On 2016/05/20 18:28:16, Kevin M wrote:
> "type" - update documentation

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:44: // Returns a BlimpMessageProcessor
object for sending messages of type
On 2016/05/20 18:28:16, Kevin M wrote:
> "of type feature_case" doesn't parse...
> 
> how about "sending messages for a given feature"?

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/browser_conne...
blimp/net/browser_connection_handler_unittest.cc:51:
FakeFeature(BlimpMessage::FeatureCase type,
On 2016/05/20 18:28:16, Kevin M wrote:
> update param name

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/engine_authen...
File blimp/net/engine_authentication_handler.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/engine_authen...
blimp/net/engine_authentication_handler.cc:114: if
(message->has_protocol_control() &&
On 2016/05/20 18:28:16, Kevin M wrote:
> check cases, not has()

See my previous comments.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager.h (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager.h:44: // for sending messages of type |type|.
On 2016/05/20 18:28:16, Kevin M wrote:
> "type"... update comment

Done.

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager_unittest.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager_unittest.cc:31:
FakeFeature(BlimpMessage::FeatureCase type,
On 2016/05/20 18:28:16, Kevin M wrote:
> "type" - here and elsewhere

Done.

[Kevin M, 2016-05-21 00:32:06]

lgtm % nits

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1933053003/diff/60001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:461: if
(message->has_tab_control()) {
On 2016/05/20 22:29:41, shaktisahu wrote:
> On 2016/05/20 18:28:15, Kevin M wrote:
> > Check the feature case
> 
> Why? We are already doing that in the DCHECK above. And it is much cleaner.

Consistency in how we are checking the type. In some places, we check
feature_case(), and in others, we check has_x(). I don't feel strongly about
this point, however.

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/blimp_message...
File blimp/net/blimp_message_demultiplexer.h (right):

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer.h:20: // processors based on
|message.type|.
fix reference to type

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_demultiplexer.h:30: // of the |type| specified.
Fix "type" here and on teh next line

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/blimp_message...
File blimp/net/blimp_message_multiplexer.cc (right):

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_multiplexer.cc:23: // |message.feature_case|, if set,
must match the sender's feature_case.
sender's feature_case_ (trailing _)

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/browser_conne...
File blimp/net/browser_connection_handler.h (right):

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/browser_conne...
blimp/net/browser_connection_handler.h:39: // |feature_case|
Premature newline here

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/thread_pipe_m...
File blimp/net/thread_pipe_manager.cc (right):

https://codereview.chromium.org/1933053003/diff/80001/blimp/net/thread_pipe_m...
blimp/net/thread_pipe_manager.cc:62: // as the
unnecessary newline

[shaktisahu, 2016-05-21 01:34:26]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-05-21 01:34:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1933053003/#ps100001
(title: "Nits")

[commit-bot: I haz the power, 2016-05-21 01:34:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1933053003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1933053003/100001

[commit-bot: I haz the power, 2016-05-21 03:24:00]

Description was changed from

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
==========

to

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
==========

[commit-bot: I haz the power, 2016-05-21 03:24:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-21 03:25:59]

Description was changed from

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371
==========

to

==========
Used oneof in blimp_message.proto

oneof is a way to set zero or at most one field out of a set of optional
fields in a proto. Used oneof in blimp_message.proto for the feature
specific messages. Removed the enum that specifies which feature is
currently in use as this is not required any more.

BUG=570371

Committed: https://crrev.com/eb552d3ed748c438f7861ec9b75ec30d404a7ac3
Cr-Commit-Position: refs/heads/master@{#395243}
==========

[commit-bot: I haz the power, 2016-05-21 03:26:00]

Patchset 6 (id:??) landed as
https://crrev.com/eb552d3ed748c438f7861ec9b75ec30d404a7ac3
Cr-Commit-Position: refs/heads/master@{#395243}