Hooking up Blimp IME with BlimpContents

Hooking up Blimp IME with BlimpContents

This CL hooks up the ImeFeature and its delegate with BlimpContents.
The IME is an Android AlertDialog driven by ImeHelperDialog in native
which implements the ImeFeature's delegate interface and is owned in
native by the BlimpContentsView. The methods from ImeFeature for
sending text to engine is passed to the delegate as a callback which
binds the appropriate tab ID along with it.

BUG=611111
Committed: https://crrev.com/dbc30df32da8a742e26ac283114d8c72291e2347
Cr-Commit-Position: refs/heads/master@{#415867}

[shaktisahu, 2016-08-31 03:34:00]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[shaktisahu, 2016-08-31 03:34:02]

David, PTAL

[shaktisahu, 2016-08-31 03:45:04]

Description was changed from

==========
Hooking up Blimp IME with BlimpContents

This CL hooks up the ImeHelperDialog with BlimpContents.
ImeHelperDialog is owned in native by the BlimpContentsView
and is set as delegate for ImeFeature in BlimpContents constructor.

BUG=611111
==========

to

==========
Hooking up Blimp IME with BlimpContents

This CL hooks up the ImeFeature and its delegate with BlimpContents.
The IME is an Android AlertDialog driven by ImeHelperDialog in native
which implements the ImeFeature's delegate interface and is owned in
native by the BlimpContentsView. The methods from ImeFeature for
sending text to engine is passed to the delegate as a callback which
binds the appropriate tab ID along with it.

BUG=611111
==========

[David Trainor- moved to gerrit, 2016-08-31 05:59:10]

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.cc:70: std::string textInput =
base::android::ConvertJavaStringToUTF8(env, text);
text_input

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.cc:71:
textSubmitCallback_.Run(textInput);
drop the callback after calling it

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/ime_helper_dialog.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.h:20: class ImeHelperDialog : public
ImeFeature::Delegate {
Do we need two versions of this class?  Are they diverging that much?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.h:56: ShowImeCallback
textSubmitCallback_;
text_submit_callback_.  Could we also mention that it's the callback for the
current request?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.cc:53:
textSubmitCallback_.Run(textInput);
Drop the callback after calling it?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:17: }
// namespace ui

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:32: void
OnShowImeRequested(ui::TextInputType input_type,
We should look into grouping these params into a Request struct in the future. 
Doesn't have to be done in this patch though.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:46: ShowImeCallback
textSubmitCallback_;
text_submit_callback_

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java
(right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
private final Context mContext;
Move the final above the other parameters, but below the static.  Can we split
these by a few new lines in between types of variables?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:66: ImeFeature* GetImeFeature()
{ return ime_feature_; }
Not used?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:80: ImeFeature* ime_feature_;
Should we just pass this right into the BlimpContentsView?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_view_android.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:30:
ime_dialog_.reset(new ImeHelperDialog(view_.GetWindowAndroid()));
Let's put this in the constructor, we can access the WindowAndroid from
blimp_contents->GetNativeWindow()

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:33:
BlimpContentsViewAndroid::~BlimpContentsViewAndroid() {}
= default?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:55: ShowImeCallback showImeCallback =
base::Bind(
show_ime_callback

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:19: typedef base::Callback<void(const
std::string&)> ShowImeCallback;
- using instead of typedef?
- put this as a class member of the IMEFeature?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:59: void OnImeTextEntered(int tab_id,
Would it make sense to make this private if we're driving it from the callback?

[shaktisahu, 2016-08-31 17:28:22]

PTAL

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.cc:70: std::string textInput =
base::android::ConvertJavaStringToUTF8(env, text);
On 2016/08/31 05:59:09, David Trainor wrote:
> text_input

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.cc:71:
textSubmitCallback_.Run(textInput);
On 2016/08/31 05:59:09, David Trainor wrote:
> drop the callback after calling it

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/ime_helper_dialog.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.h:20: class ImeHelperDialog : public
ImeFeature::Delegate {
On 2016/08/31 05:59:09, David Trainor wrote:
> Do we need two versions of this class?  Are they diverging that much?

Not a lot though. The app version is driven by Java and doesn't understand
WindowAndroid stuff. I have to make some changes to BlimpClientSession to make
the contents version work. I think we can delete this stuff shortly.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.h:56: ShowImeCallback
textSubmitCallback_;
On 2016/08/31 05:59:09, David Trainor wrote:
> text_submit_callback_.  Could we also mention that it's the callback for the
> current request?

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.cc:53:
textSubmitCallback_.Run(textInput);
On 2016/08/31 05:59:09, David Trainor wrote:
> Drop the callback after calling it?

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:17: }
On 2016/08/31 05:59:09, David Trainor wrote:
> // namespace ui

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:32: void
OnShowImeRequested(ui::TextInputType input_type,
On 2016/08/31 05:59:09, David Trainor wrote:
> We should look into grouping these params into a Request struct in the future.

> Doesn't have to be done in this patch though.

Acknowledged.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.h:46: ShowImeCallback
textSubmitCallback_;
On 2016/08/31 05:59:09, David Trainor wrote:
> text_submit_callback_

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:66: ImeFeature* GetImeFeature()
{ return ime_feature_; }
On 2016/08/31 05:59:10, David Trainor wrote:
> Not used?

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:80: ImeFeature* ime_feature_;
On 2016/08/31 05:59:10, David Trainor wrote:
> Should we just pass this right into the BlimpContentsView?

In that case, we have to pass ImeFeature to BlimpContentsView::Create() and the
BlimpContentsView constructor.
BlimpContentsView would then have to hold the ime_feature_ as a member in order
to reset the delegate upon destruction.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_view_android.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:30:
ime_dialog_.reset(new ImeHelperDialog(view_.GetWindowAndroid()));
On 2016/08/31 05:59:10, David Trainor wrote:
> Let's put this in the constructor, we can access the WindowAndroid from
> blimp_contents->GetNativeWindow()

I had to do
blimp_contents->blimp_contents_impl()->GetNativeView()->GetWindowAndroid()

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:33:
BlimpContentsViewAndroid::~BlimpContentsViewAndroid() {}
On 2016/08/31 05:59:10, David Trainor wrote:
> = default?

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:33:
BlimpContentsViewAndroid::~BlimpContentsViewAndroid() {}
On 2016/08/31 05:59:10, David Trainor wrote:
> = default?

Doesn't work. "Complex destructor has an inline body."
I guess the view_ has non-trivial destructor.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:55: ShowImeCallback showImeCallback =
base::Bind(
On 2016/08/31 05:59:10, David Trainor wrote:
> show_ime_callback

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:19: typedef base::Callback<void(const
std::string&)> ShowImeCallback;
On 2016/08/31 05:59:10, David Trainor wrote:
> - using instead of typedef?
> - put this as a class member of the IMEFeature?

Done.

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:59: void OnImeTextEntered(int tab_id,
On 2016/08/31 05:59:10, David Trainor wrote:
> Would it make sense to make this private if we're driving it from the
callback?

Done.

[David Trainor- moved to gerrit, 2016-08-31 18:59:54]

lgtm % nits

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:80: ImeFeature* ime_feature_;
On 2016/08/31 17:28:21, shaktisahu wrote:
> On 2016/08/31 05:59:10, David Trainor wrote:
> > Should we just pass this right into the BlimpContentsView?
> 
> In that case, we have to pass ImeFeature to BlimpContentsView::Create() and
the
> BlimpContentsView constructor.
> BlimpContentsView would then have to hold the ime_feature_ as a member in
order
> to reset the delegate upon destruction. 

That's fine with me.  The ime feature feels like a view-specific thing anyway
:).

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_view_android.cc (right):

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:30:
ime_dialog_.reset(new ImeHelperDialog(view_.GetWindowAndroid()));
On 2016/08/31 17:28:21, shaktisahu wrote:
> On 2016/08/31 05:59:10, David Trainor wrote:
> > Let's put this in the constructor, we can access the WindowAndroid from
> > blimp_contents->GetNativeWindow()
> 
> I had to do
> blimp_contents->blimp_contents_impl()->GetNativeView()->GetWindowAndroid()

Ah make the constructor take a BlimpClientsImpl() and not the Android version?

https://codereview.chromium.org/2292343002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_android.cc:33:
BlimpContentsViewAndroid::~BlimpContentsViewAndroid() {}
On 2016/08/31 17:28:21, shaktisahu wrote:
> On 2016/08/31 05:59:10, David Trainor wrote:
> > = default?
> 
> Done.

= default; should be fine in the cc file though right?

https://codereview.chromium.org/2292343002/diff/40001/blimp/client/app/androi...
File blimp/client/app/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/40001/blimp/client/app/androi...
blimp/client/app/android/ime_helper_dialog.cc:72:
text_submit_callback_.Run(text_input);
try 

base::ResetAndReturn(&text_submit_callback_).Run(text_input);

instead of these two lines.

https://codereview.chromium.org/2292343002/diff/40001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2292343002/diff/40001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.cc:54:
text_submit_callback_.Run(text_input);
ResetAndReturn again?

[shaktisahu, 2016-08-31 20:25:12]

shaktisahu@chromium.org changed reviewers:
+ mikecase@chromium.org

[shaktisahu, 2016-08-31 20:25:12]

mikecase@chromium.org: Please review changes in lint

[shaktisahu, 2016-08-31 22:34:37]

shaktisahu@chromium.org changed reviewers:
+ nyquist@chromium.org

[shaktisahu, 2016-08-31 22:34:38]

@nyquist, please take a look at the last patch. I removed the duplicate files
for ImeHelperDialog from blimp/client/app

[David Trainor- moved to gerrit, 2016-08-31 23:19:58]

lgtm

[shaktisahu, 2016-09-01 00:29:18]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-09-01 00:29:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2292343002/#ps80001
(title: "merge origin/master")

[commit-bot: I haz the power, 2016-09-01 00:29:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 00:32:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 00:32:53]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shaktisahu, 2016-09-01 00:47:53]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 00:48:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 01:16:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 01:16:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-09-01 02:06:11]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 02:06:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2016-09-01 03:01:55]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2016-09-01 03:02:02]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-09-01 03:02:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2292343002/#ps120001
(title: "Fixed linux client")

[commit-bot: I haz the power, 2016-09-01 03:02:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 03:08:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-01 03:10:10]

Description was changed from

==========
Hooking up Blimp IME with BlimpContents

This CL hooks up the ImeFeature and its delegate with BlimpContents.
The IME is an Android AlertDialog driven by ImeHelperDialog in native
which implements the ImeFeature's delegate interface and is owned in
native by the BlimpContentsView. The methods from ImeFeature for
sending text to engine is passed to the delegate as a callback which
binds the appropriate tab ID along with it.

BUG=611111
==========

to

==========
Hooking up Blimp IME with BlimpContents

This CL hooks up the ImeFeature and its delegate with BlimpContents.
The IME is an Android AlertDialog driven by ImeHelperDialog in native
which implements the ImeFeature's delegate interface and is owned in
native by the BlimpContentsView. The methods from ImeFeature for
sending text to engine is passed to the delegate as a callback which
binds the appropriate tab ID along with it.

BUG=611111

Committed: https://crrev.com/dbc30df32da8a742e26ac283114d8c72291e2347
Cr-Commit-Position: refs/heads/master@{#415867}
==========

[commit-bot: I haz the power, 2016-09-01 03:10:12]

Patchset 7 (id:??) landed as
https://crrev.com/dbc30df32da8a742e26ac283114d8c72291e2347
Cr-Commit-Position: refs/heads/master@{#415867}