Blimp: IME should submit form with text

blimp/client/core/contents/ime_feature.cc

Index: blimp/client/core/contents/ime_feature.cc
diff --git a/blimp/client/core/contents/ime_feature.cc b/blimp/client/core/contents/ime_feature.cc
index 4a4bcdbfe331165cdd44125d5d47c0e95aab0ac9..3e0012d00bd679f80a6b902af5d7f2f56c0bd1f7 100644
--- a/blimp/client/core/contents/ime_feature.cc
+++ b/blimp/client/core/contents/ime_feature.cc
@@ -14,22 +14,38 @@
 namespace blimp {
 namespace client {
 
+ImeFeature::WebInputRequest::WebInputRequest()
+    : tab_id(-1),
+      render_widget_id(0),

[David Trainor- moved to gerrit, 2016/10/14 16:57:40]

Is 0 the correct invalid id value?  Should we just make the request take these
in the constructor?  If we bake them into the callbcak we can remove them, which
would be nice too :).

[shaktisahu, 2016/10/14 18:08:18]

Done.

+      input_type(ui::TEXT_INPUT_TYPE_NONE),
+      text(""),

[David Trainor- moved to gerrit, 2016/10/14 16:57:40]

Don't need.  Class has a default constructor.

[shaktisahu, 2016/10/14 18:08:18]

Done.

+      feature(nullptr) {}
+
+ImeFeature::WebInputResponse::WebInputResponse()
+    : tab_id(-1), render_widget_id(0), text(""), submit(false) {}
+
+ImeFeature::WebInputResponse::WebInputResponse(
+    const ImeFeature::WebInputRequest& request)
+    : WebInputResponse() {
+  tab_id = request.tab_id;
+  render_widget_id = request.render_widget_id;
+}
+
 ImeFeature::ImeFeature() {}
 
 ImeFeature::~ImeFeature() {}
 
-void ImeFeature::OnImeTextEntered(int tab_id,
-                                  int render_widget_id,
-                                  const std::string& text) {
-  DCHECK_LE(0, tab_id);
-  DCHECK_LT(0, render_widget_id);
+void ImeFeature::OnImeTextEntered(const WebInputResponse& response) {
+  DCHECK_LE(0, response.tab_id);

[David Trainor- moved to gerrit, 2016/10/14 16:57:40]

I think generally we put the checked value first and the expected value as the
response?  This feels like it reads a bit cleaner:

DCHECK_GE(response.tab_id, 0);
DCHECK_GT(response.render_wiget_id, 0);

[shaktisahu, 2016/10/14 18:08:18]

I see usages both way. I remember Wez once commented to write this way. Don't
remember exactly though.

[David Trainor- moved to gerrit, 2016/10/18 16:53:45]

sg

+  DCHECK_LT(0, response.render_widget_id);
 
   ImeMessage* ime_message;
   std::unique_ptr<BlimpMessage> blimp_message =
-      CreateBlimpMessage(&ime_message, tab_id);
-  ime_message->set_render_widget_id(render_widget_id);
+      CreateBlimpMessage(&ime_message, response.tab_id);
+  ime_message->set_render_widget_id(response.render_widget_id);
   ime_message->set_type(ImeMessage::SET_TEXT);
-  ime_message->set_ime_text(text);
+  ime_message->set_ime_text(response.text);
+  ime_message->set_auto_submit(response.submit);
 
   outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
                                               net::CompletionCallback());
@@ -52,13 +68,15 @@ void ImeFeature::ProcessMessage(std::unique_ptr<BlimpMessage> message,
         return;
       }
       {
-        ShowImeCallback show_ime_callback = base::Bind(
-            &ImeFeature::OnImeTextEntered, base::Unretained(this),
-            message->target_tab_id(), ime_message.render_widget_id());
-        delegate_->OnShowImeRequested(
-            InputMessageConverter::TextInputTypeFromProto(
-                ime_message.text_input_type()),
-            ime_message.ime_text(), show_ime_callback);
+        WebInputRequest request;
+        request.tab_id = message->target_tab_id();
+        request.render_widget_id = ime_message.render_widget_id();
+        request.input_type = InputMessageConverter::TextInputTypeFromProto(
+            ime_message.text_input_type());
+        request.text = ime_message.ime_text();
+        request.feature = this;
+
+        delegate_->OnShowImeRequested(request);
       }
       break;
     case ImeMessage::HIDE_IME: