Added network components for blimp text input feature

blimp/client/feature/ime_feature.h

+// 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.
+
+#ifndef BLIMP_CLIENT_FEATURE_IME_FEATURE_H_
+#define BLIMP_CLIENT_FEATURE_IME_FEATURE_H_
+
+#include <map>
+#include <string>
+
+#include "base/macros.h"
+#include "blimp/client/blimp_client_export.h"
+#include "blimp/net/blimp_message_processor.h"
+#include "ui/base/ime/text_input_type.h"
+
+namespace blimp {
+namespace client {
+
+// Handles all incoming and outgoing protobuf messages for text input of type
+// BlimpMessage::IME.
+// Upon receiving a text input request from the engine, the ImeFeature caches
+// the |tab_id_| and |render_widget_id_| for the request and
+// delegates the request to ImeFeatureDelegate which then opens up the IME.
+// After user is done typing, the text is passed back to ImeFeature, which then
+// sends the text to the engine over network along with the same |tab_id_| and
+// |render_widget_id_|.

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

nit: What happens if the Engine sends a new request while the previous one is
outstanding?

+class BLIMP_CLIENT_EXPORT ImeFeature : public BlimpMessageProcessor {

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

This should be ImeClientFeature, since there will be a corresponding IME feature
object for the Engine.

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

Hmm, not sure if this sounds good. By the way, on the server side, IME requests
are handled in EngineRenderWidgetFeature. 

+ public:
+  // A delegate to be notified of text input events.

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

nit: I think you mean "text input requests"?

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

Done.

+  class ImeFeatureDelegate {
+   public:
+    virtual void OnShowImeRequested(ui::TextInputType input_type,
+                                    const std::string& text) = 0;
+    virtual void OnHideImeRequested() = 0;
+  };
+
+  ImeFeature();
+  ~ImeFeature() override;
+
+  // Set the BlimpMessageProcessor that will be used to send BlimpMessage::IME
+  // messages to the engine.
+  void set_outgoing_message_processor(
+      scoped_ptr<BlimpMessageProcessor> processor);
+
+  // Sets a ImeFeatureDelegate to be notified of all text input messages.
+  void SetDelegate(ImeFeatureDelegate* delegate);

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

Could/should this be a constructor parameter?

nit: Be explicit re the lifetime expectation of the |delegate|, e.g. it must
remain valid until after the last message has been passed to ImeFeature, or
until ImeFeature is actually deleted?

Also, since this is a simple setter it could be named in unix_style rather than
CamelCase, and since the preceding setter is unix_style, this should also be, to
match.

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

Can't be a constructor param since ImeFeature outlives the |delegate|. 

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

IIUC you're saying it's valid to pass a NULL |delegate| parameter - in which
case best to mention that in the comment, e.g. "Passing a null |delegate| causes
IME events to be ignored."

+
+  // Sends text from IME to the blimp engine.
+  void SendImeTextToEngine(const std::string& text);

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

This class doesn't really have a concept of the "Engine", just of incoming and
outgoing messages; you might rename this OnImeTextEntered() or just
SendImeText().

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

Done.

+
+ private:
+  // BlimpMessageProcessor implementation.
+  void ProcessMessage(scoped_ptr<BlimpMessage> message,
+                      const net::CompletionCallback& callback) override;
+
+  ImeFeatureDelegate* delegate_;

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

nit: You can in-line initialize this to nullptr.

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

Done.

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

Doesn't look done!

+
+  // Tab id and render widget id for the input field for which user input is
+  // being requested.
+  // The values are cached from the ImeMessage::SHOW_IME message and sent back
+  // to engine in ImeMessage::SET_TEXT message.
+  // The cached values are not cleared, since they are overwritten with the new
+  // value at the beginning of every text input request.
+  int tab_id_;
+  int render_widget_id_;

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

nit: Similarly, suggest inline initializing these to invalid values.

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

Done.

+
+  // Used to send BlimpMessage::IME messages to the engine.
+  scoped_ptr<BlimpMessageProcessor> outgoing_message_processor_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImeFeature);
+};
+
+}  // namespace client
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_FEATURE_IME_FEATURE_H_