Blimp: IME should submit form with text

blimp/client/core/contents/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/core/contents/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::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());
 }
 
 void ImeFeature::ProcessMessage(std::unique_ptr<BlimpMessage> message,
                                 const net::CompletionCallback& callback) {
   DCHECK(!callback.is_null());
   DCHECK_EQ(BlimpMessage::kIme, message->feature_case());
 
   DCHECK(delegate_);
 
   const ImeMessage& ime_message = message->ime();
 
   switch (ime_message.type()) {
     case ImeMessage::SHOW_IME:
       if (!message->has_target_tab_id() || message->target_tab_id() < 0 ||
           ime_message.render_widget_id() <= 0) {
         callback.Run(net::ERR_INVALID_ARGUMENT);
         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:
       delegate_->OnHideImeRequested();
       break;
     case ImeMessage::SET_TEXT:
     case ImeMessage::UNKNOWN:
       NOTREACHED();
       callback.Run(net::ERR_UNEXPECTED);
       return;
   }
 
   callback.Run(net::OK);
 }
 
 }  // namespace client
 }  // namespace blimp