Address post commit comments on Blimp Feature CL

- Address comments after
  https://chromiumcodereview.appspot.com/1496133002/ landed.
- Specifically fix some nits and update the unit tests to use gmock
  properly.

Review URL: https://codereview.chromium.org/1518753002

Cr-Commit-Position: refs/heads/master@{#365033}

4 files changed, 71 insertions, 62 deletions

diff --git a/blimp/client/android/toolbar.h b/blimp/client/android/toolbar.h
index b72ce1b..df4203b 100644
--- a/blimp/client/android/toolbar.h
+++ b/blimp/client/android/toolbar.h
@@ -41,6 +41,9 @@ class Toolbar : public NavigationFeature::NavigationFeatureDelegate {
  private:
   virtual ~Toolbar();
 
+  // A bridge to the network layer which does the work of (de)serializing the
+  // outgoing and incoming navigation messages from the engine. Toolbar does not
+  // own this and it is expected to outlive this Toolbar instance.
   NavigationFeature* navigation_feature_;
 
   // Reference to the Java object which owns this class.
diff --git a/blimp/client/compositor/blimp_compositor.h b/blimp/client/compositor/blimp_compositor.h
index 18280e0..05401c1 100644
--- a/blimp/client/compositor/blimp_compositor.h
+++ b/blimp/client/compositor/blimp_compositor.h
@@ -81,8 +81,7 @@ class BLIMP_CLIENT_EXPORT BlimpCompositor
   // |dp_to_px| is the scale factor required to move from dp (device pixels) to
   // px.  See https://developer.android.com/guide/practices/screens_support.html
   // for more details.
-  explicit BlimpCompositor(float dp_to_px,
-                           RenderWidgetFeature* render_widget_feature);
+  BlimpCompositor(float dp_to_px, RenderWidgetFeature* render_widget_feature);
 
   // Populates the cc::LayerTreeSettings used by the cc::LayerTreeHost.  Can be
   // overridden to provide custom settings parameters.
@@ -162,6 +161,8 @@ class BLIMP_CLIENT_EXPORT BlimpCompositor
   cc::RemoteProtoChannel::ProtoReceiver* remote_proto_channel_receiver_;
 
   // The bridge to the network layer that does the proto/RenderWidget id work.
+  // BlimpCompositor does not own this and it is expected to outlive this
+  // BlimpCompositor instance.
   // TODO(dtrainor): Move this to a higher level once we start dealing with
   // multiple tabs.
   RenderWidgetFeature* render_widget_feature_;
diff --git a/blimp/client/session/render_widget_feature_unittest.cc b/blimp/client/session/render_widget_feature_unittest.cc
index 3e7752d..aeb4eb0 100644
--- a/blimp/client/session/render_widget_feature_unittest.cc
+++ b/blimp/client/session/render_widget_feature_unittest.cc
@@ -17,6 +17,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::InSequence;
+using testing::Sequence;
 
 namespace blimp {
 
@@ -96,37 +98,31 @@ class RenderWidgetFeatureTest : public testing::Test {
 
 TEST_F(RenderWidgetFeatureTest, DelegateCallsOK) {
   EXPECT_CALL(delegate1_, MockableOnRenderWidgetInitialized()).Times(1);
-  SendRenderWidgetMessage(&feature_, 1, 1U);
+  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(_)).Times(2);
+  EXPECT_CALL(delegate2_, MockableOnRenderWidgetInitialized()).Times(1);
+  EXPECT_CALL(delegate2_, MockableOnCompositorMessageReceived(_)).Times(0);
 
-  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(_)).Times(1);
+  SendRenderWidgetMessage(&feature_, 1, 1U);
+  SendCompositorMessage(&feature_, 1, 1U);
   SendCompositorMessage(&feature_, 1, 1U);
-
-  EXPECT_CALL(delegate2_, MockableOnRenderWidgetInitialized()).Times(1);
   SendRenderWidgetMessage(&feature_, 2, 2U);
-
-  EXPECT_CALL(delegate2_, MockableOnCompositorMessageReceived(_)).Times(1);
-  SendCompositorMessage(&feature_, 2, 2U);
 }
 
 TEST_F(RenderWidgetFeatureTest, RepliesHaveCorrectRenderWidgetId) {
-  SendRenderWidgetMessage(&feature_, 1, 2U);
-  SendRenderWidgetMessage(&feature_, 2, 1U);
+  InSequence sequence;
 
   EXPECT_CALL(*out_compositor_processor_,
-              MockableProcessMessage(CompMsgEquals(1, 2U), _))
-      .Times(1);
-  feature_.SendCompositorMessage(1, cc::proto::CompositorMessage());
+              MockableProcessMessage(CompMsgEquals(1, 2U), _));
+  EXPECT_CALL(*out_compositor_processor_,
+              MockableProcessMessage(CompMsgEquals(1, 3U), _));
+  EXPECT_CALL(*out_compositor_processor_,
+              MockableProcessMessage(CompMsgEquals(2, 1U), _));
 
+  SendRenderWidgetMessage(&feature_, 1, 2U);
+  feature_.SendCompositorMessage(1, cc::proto::CompositorMessage());
   SendRenderWidgetMessage(&feature_, 1, 3U);
-
-  EXPECT_CALL(*out_compositor_processor_,
-              MockableProcessMessage(CompMsgEquals(1, 3U), _))
-      .Times(1);
   feature_.SendCompositorMessage(1, cc::proto::CompositorMessage());
-
-  EXPECT_CALL(*out_compositor_processor_,
-              MockableProcessMessage(CompMsgEquals(2, 1U), _))
-      .Times(1);
+  SendRenderWidgetMessage(&feature_, 2, 1U);
   feature_.SendCompositorMessage(2, cc::proto::CompositorMessage());
 }
 
diff --git a/blimp/engine/browser/engine_render_widget_message_processor_unittest.cc b/blimp/engine/browser/engine_render_widget_message_processor_unittest.cc
index 70d78fd..82a06df 100644
--- a/blimp/engine/browser/engine_render_widget_message_processor_unittest.cc
+++ b/blimp/engine/browser/engine_render_widget_message_processor_unittest.cc
@@ -19,10 +19,8 @@
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 
 using testing::_;
-using testing::InvokeArgument;
-using testing::Ref;
-using testing::Return;
-using testing::SaveArg;
+using testing::InSequence;
+using testing::Sequence;
 
 namespace blimp {
 
@@ -110,9 +108,6 @@ class EngineRenderWidgetMessageProcessorTest : public testing::Test {
   void SetUp() override {
     processor_.SetDelegate(1, &delegate1_);
     processor_.SetDelegate(2, &delegate2_);
-
-    processor_.OnRenderWidgetInitialized(1);
-    processor_.OnRenderWidgetInitialized(2);
   }
 
  protected:
@@ -125,63 +120,77 @@ class EngineRenderWidgetMessageProcessorTest : public testing::Test {
 TEST_F(EngineRenderWidgetMessageProcessorTest, DelegateCallsOK) {
   std::vector<uint8_t> payload = { 'd', 'a', 'v', 'i', 'd' };
 
-  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(
-      CompMsgEquals(payload))).Times(1);
-  SendCompositorMessage(&processor_, 1, 1U, payload);
-
+  EXPECT_CALL(out_processor_, MockableProcessMessage(_, _)).Times(2);
+  EXPECT_CALL(delegate1_,
+              MockableOnCompositorMessageReceived(CompMsgEquals(payload)))
+      .Times(1);
   EXPECT_CALL(delegate1_, MockableOnWebInputEvent()).Times(1);
-  SendInputMessage(&processor_, 1, 1U);
+  EXPECT_CALL(delegate2_,
+              MockableOnCompositorMessageReceived(CompMsgEquals(payload)))
+      .Times(1);
+  EXPECT_CALL(delegate2_, MockableOnWebInputEvent()).Times(0);
 
-  EXPECT_CALL(delegate2_, MockableOnCompositorMessageReceived(
-      CompMsgEquals(payload))).Times(1);
+  processor_.OnRenderWidgetInitialized(1);
+  processor_.OnRenderWidgetInitialized(2);
+  SendCompositorMessage(&processor_, 1, 1U, payload);
+  SendInputMessage(&processor_, 1, 1U);
   SendCompositorMessage(&processor_, 2, 1U, payload);
-
-  EXPECT_CALL(delegate2_, MockableOnWebInputEvent()).Times(1);
-  SendInputMessage(&processor_, 2, 1U);
 }
 
 TEST_F(EngineRenderWidgetMessageProcessorTest, DropsStaleMessages) {
+  InSequence sequence;
   std::vector<uint8_t> payload = { 'f', 'u', 'n' };
-
-  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(
-      CompMsgEquals(payload))).Times(1);
-  SendCompositorMessage(&processor_, 1, 1U, payload);
+  std::vector<uint8_t> new_payload = {'n', 'o', ' ', 'f', 'u', 'n'};
 
   EXPECT_CALL(out_processor_,
-              MockableProcessMessage(BlimpRWMsgEquals(1, 2U), _)).Times(1);
-  processor_.OnRenderWidgetInitialized(1);
+              MockableProcessMessage(BlimpRWMsgEquals(1, 1U), _));
+  EXPECT_CALL(delegate1_,
+              MockableOnCompositorMessageReceived(CompMsgEquals(payload)));
+  EXPECT_CALL(out_processor_,
+              MockableProcessMessage(BlimpRWMsgEquals(1, 2U), _));
+  EXPECT_CALL(delegate1_,
+              MockableOnCompositorMessageReceived(CompMsgEquals(new_payload)));
+  EXPECT_CALL(delegate1_, MockableOnWebInputEvent());
 
-  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(
-      CompMsgEquals(payload))).Times(0);
-  payload[0] = 'a';
+  processor_.OnRenderWidgetInitialized(1);
   SendCompositorMessage(&processor_, 1, 1U, payload);
+  processor_.OnRenderWidgetInitialized(1);
 
-  EXPECT_CALL(delegate1_, MockableOnWebInputEvent()).Times(0);
+  // These next three calls should be dropped.
+  SendCompositorMessage(&processor_, 1, 1U, payload);
+  SendCompositorMessage(&processor_, 1, 1U, payload);
   SendInputMessage(&processor_, 1, 1U);
 
-  EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived(
-      CompMsgEquals(payload))).Times(1);
-  SendCompositorMessage(&processor_, 1, 2U, payload);
-
-  EXPECT_CALL(delegate1_, MockableOnWebInputEvent()).Times(1);
+  SendCompositorMessage(&processor_, 1, 2U, new_payload);
   SendInputMessage(&processor_, 1, 2U);
 }
 
 TEST_F(EngineRenderWidgetMessageProcessorTest,
        RepliesHaveCorrectRenderWidgetId) {
+  Sequence delegate1_sequence;
+  Sequence delegate2_sequence;
   std::vector<uint8_t> payload = { 'a', 'b', 'c', 'd' };
 
   EXPECT_CALL(out_processor_,
-              MockableProcessMessage(BlimpRWMsgEquals(1, 2U), _)).Times(1);
-  processor_.OnRenderWidgetInitialized(1);
-
+              MockableProcessMessage(BlimpRWMsgEquals(1, 1U), _))
+      .InSequence(delegate1_sequence);
   EXPECT_CALL(out_processor_,
-              MockableProcessMessage(BlimpRWMsgEquals(2, 2U), _)).Times(1);
-  processor_.OnRenderWidgetInitialized(2);
-
-  EXPECT_CALL(out_processor_, MockableProcessMessage(
-      BlimpCompMsgEquals(1, 2U, payload), _)).Times(1);
+              MockableProcessMessage(BlimpRWMsgEquals(1, 2U), _))
+      .InSequence(delegate1_sequence);
+  EXPECT_CALL(out_processor_,
+              MockableProcessMessage(BlimpCompMsgEquals(1, 2U, payload), _))
+      .InSequence(delegate1_sequence);
+  EXPECT_CALL(out_processor_,
+              MockableProcessMessage(BlimpRWMsgEquals(2, 1U), _))
+      .InSequence(delegate2_sequence);
+  EXPECT_CALL(out_processor_,
+              MockableProcessMessage(BlimpRWMsgEquals(2, 2U), _))
+      .InSequence(delegate2_sequence);
 
+  processor_.OnRenderWidgetInitialized(1);
+  processor_.OnRenderWidgetInitialized(2);
+  processor_.OnRenderWidgetInitialized(1);
+  processor_.OnRenderWidgetInitialized(2);
   processor_.SendCompositorMessage(1, payload);
 }