Added network components for blimp text input feature

blimp/client/feature/ime_feature.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <blimp/client/feature/ime_feature.h>
+#include <string>
+
+#include "blimp/common/create_blimp_message.h"
+#include "blimp/common/proto/blimp_message.pb.h"
+#include "blimp/net/input_message_converter.h"
+#include "net/base/net_errors.h"
+
+namespace blimp {
+namespace client {
+
+ImeFeature::ImeFeature() {}

[Wez, 2016/03/18 20:51:28]

As noted in the header, there are POD members of this class that need
initializing; you can do it in the header directly, otherwise you have to do it
here.

[shaktisahu, 2016/03/22 19:44:19]

Done.

+
+ImeFeature::~ImeFeature() {}
+
+void ImeFeature::set_outgoing_message_processor(
+    scoped_ptr<BlimpMessageProcessor> processor) {
+  outgoing_message_processor_ = std::move(processor);

[Wez, 2016/03/18 20:51:27]

nit: Since this is a "simple" setter you could implement it in-line in the
header.

[shaktisahu, 2016/03/22 19:44:19]

Done.

+}
+
+void ImeFeature::SetDelegate(ImeFeatureDelegate* delegate) {

[Wez, 2016/03/18 20:51:28]

Similarly, since this is a simple setter, it could be implemented in the header
directly.

[shaktisahu, 2016/03/22 19:44:19]

Done.

+  delegate_ = delegate;
+}
+
+void ImeFeature::SendImeTextToEngine(const std::string& text) {

[Wez, 2016/03/18 20:51:28]

I'd suggest DCHECK()ing that the tab & widget Ids are valid at this point.

[shaktisahu, 2016/03/22 19:44:19]

Done.

+  ImeMessage* ime_message;
+  scoped_ptr<BlimpMessage> blimp_message =
+      CreateBlimpMessage(&ime_message, tab_id_);
+  ime_message->set_render_widget_id(render_widget_id_);
+  ime_message->set_type(ImeMessage::SET_TEXT);
+  ime_message->set_ime_text(text);
+
+  outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
+                                              net::CompletionCallback());
+}
+
+void ImeFeature::ProcessMessage(scoped_ptr<BlimpMessage> message,
+                                const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+  DCHECK(message->type() == BlimpMessage::IME);

[Wez, 2016/03/18 20:51:27]

As noted below, if the Engine sends bad data, callback.Run(SOME_ERROR_CODE).

[shaktisahu, 2016/03/22 19:44:19]

Done.

[Wez, 2016/03/22 21:43:37]

Ah, sorry; forgot that the demultiplexer should never let us reach here!

nit: use DCHECK_EQ(BlimpMessage::IME, message->type())

[shaktisahu, 2016/03/22 23:45:42]

Done.

[shaktisahu, 2016/03/22 23:45:42]

Done.

+
+  DCHECK(delegate_) << "ImeFeatureDelegate not set";

[Wez, 2016/03/18 20:51:27]

nit: The message following the DCHECK seems redundant?

[shaktisahu, 2016/03/22 19:44:19]

Done.

+
+  const ImeMessage& ime_message = message->ime();
+  tab_id_ = message->target_tab_id();
+  render_widget_id_ = ime_message.render_widget_id();

[Wez, 2016/03/18 20:51:28]

When you receive a hide request you're basically switching off the IME, so
instead of caching the tab & widget Ids, surely you should clear them back to
invalid values?

[shaktisahu, 2016/03/22 19:44:19]

Done.

+
+  switch (ime_message.type()) {
+    case ImeMessage::SHOW_IME:

[Wez, 2016/03/18 20:51:28]

Suggest adding a check that the tab & widget Ids are invalid if we reach here,
before caching them and calling OnShowImeRequested().  The check should
Run(SOME_ERROR_CODE) rather than DCHECK or CHECK, since we're processing
protocol data from a remote Engine that has sent something bogus, implying it
may be compromise - we therefore want to disconnect rather than continue!

[shaktisahu, 2016/03/22 19:44:19]

Done.

[Wez, 2016/03/22 21:43:37]

Looks like you've added a check for the sender specifying valid-looking Ids, but
I think you also want a check that the IME is not already shown, i.e. that
|tab_id_| and |render_widget_id_| are not already set, here.

[shaktisahu, 2016/03/22 23:45:42]

Actually, there might be a case where we might have the ids already set, IME is
showing, user presses back button (which hides the IME) and then tries again for
text input. So we can have the valid ids at this point. The ids being set/unset
is not a one-to-one mapping with showing the IME or hiding it.

+      delegate_->OnShowImeRequested(
+          InputMessageConverter::TextInputTypeFromProto(
+              ime_message.text_input_type()),
+          ime_message.ime_text());
+      break;
+    case ImeMessage::HIDE_IME:
+      delegate_->OnHideImeRequested();

[Wez, 2016/03/18 20:51:27]

Suggesting DCHECKing that the tab & widget Ids are valid here, before clearing
them before calling on to delegate.

[shaktisahu, 2016/03/22 19:44:19]

Actually, tab and widget ids can be invalid here. It happens when the page is
loaded, and text input is out of focus.

[Wez, 2016/03/22 21:43:37]

So if the tab_id_ and render_widget_id_ are invalid should we also skip calling
out to the delegate, since it presumably isn't showing anything?

[shaktisahu, 2016/03/22 23:45:42]

Same as above comment. I would still prefer to make a call to hide IME.

+      break;
+    case ImeMessage::SET_TEXT:

[Wez, 2016/03/18 20:51:27]

nit: No need for a separate code block for this and UNKNOWN; you can just have

case SET_TEXT:
case UNKNOWN:
  NOTREACHED();
  callback.Run(SOME_KIND_OF_ERROR);
  break;

You want to return SOME_KIND_OF_ERROR in this case so that we'll terminate the
connection to the Engine, since it has actually sent bogus protocol data,
suggesting that it may be compromised.

[shaktisahu, 2016/03/22 19:44:19]

Done.

+      NOTREACHED();
+      break;
+    case ImeMessage::UNKNOWN:
+      NOTREACHED();
+  }
+
+  callback.Run(net::OK);
+}
+
+}  // namespace client
+}  // namespace blimp