Blimp: Added spinner to text input dialog

Blimp: Added spinner to text input dialog

Added a spinner (ProgressBar with indeterminate style) to the text input
AlertDialog. The spinner is shown when user hits enter/OK, and is
dismissed on receiving a hide IME request or if the user hasn't changed
the text or cancelling the dialog through back button/touching outside.
During submission, the EditText background changes to a dotted line.

In an unrelated change, fixed the IME editor options (following similar
structure as ImeUtils.java). This enables multi-line input for text area.

BUG=652959
Committed: https://crrev.com/3d1c669b5c8b03dc232859960adb5d571c4c76db
Cr-Commit-Position: refs/heads/master@{#430105}

[shaktisahu, 2016-10-14 03:21:07]

Description was changed from

==========
Blimp: Added spinner to text input dialog

Added a spinner (ProgressBar with indeterminate style) to the text input
AlertDialog. The spinner is shown when user hits enter/OK, and is
dismissed on receiving a hide IME request or if the user hasn't changed
the text or cancelling the dialog through back button/touching outside.

BUG=652959
==========

to

==========
Blimp: Added spinner to text input dialog

Added a spinner (ProgressBar with indeterminate style) to the text input
AlertDialog. The spinner is shown when user hits enter/OK, and is
dismissed on receiving a hide IME request or if the user hasn't changed
the text or cancelling the dialog through back button/touching outside.
During submission, the EditText background changes to a dotted line.

In an unrelated change, fixed the IME editor options (following similar
structure as ImeUtils.java). This enables multi-line input for text area.

BUG=652959
==========

[shaktisahu, 2016-10-14 03:22:03]

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

[David Trainor- moved to gerrit, 2016-10-19 16:40:31]

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/drawable/dotted_line.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:1: <?xml
version="1.0" encoding="UTF-8"?>
Is this the right place for this file?  Have we moved our resources yet?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:16: <padding
Odd indenting

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/layout/text_input_popup.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:9: <TextView
Are we adding the label here as well?  Or is this for a different kind of label?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:15:
android:textSize="12sp"/>
Should we be setting color/font/etc?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:27: <ProgressBar
Are we relying on the OK/CANCEL buttons from the alert dialog, but hiding them
for this?  Would it be cleaner to keep both in the xml so we can eventually do
the slide animations?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/values/colors.xml:10: <color
name="disabled_text_color">#808080</color>
Doesn't look like this is ever used for text?

https://codereview.chromium.org/2393443004/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/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
public class ImeHelperDialog {
We should probably rename this or look at restructuring the code in a bit.  This
class isn't actually a dialog :).

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:40:
private ProgressBar mProgressDialog;
Dialog seems like an incorrect name

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:41:
private long mNativeImeHelperDialog;
Can you pull the pointer out separately from the java-specific members?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:72:
if (text.equals(mExistingText)) {
We still need to tell the engine that we're dismissing the IME, we just don't
need to send text to it or trigger the spinner.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:82:
mAlertDialog.getButton(DialogInterface.BUTTON_POSITIVE).setVisibility(View.GONE);
Let's talk about this today.  It might be clean to write a simple widget that
understands how to swap those buttons with a progress bar, that way we can reuse
it for all types of input/eventual UI if we need to?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:87:
mEditText.setBackgroundResource(R.drawable.dotted_line);
Do we have a blue line for normal entry?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:112:
final TextView tvLabel = (TextView) viewPopup.findViewById(R.id.label);
Can we have this and the set text lines close together?

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:126:
tvLabel.setText(R.string.blimp_ime_dialog_title);
Should this be called "web_input_default_label"?  This will be overridden by the
info coming from the site if available right?

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:237:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Actually I think it's cleaner if we send this delayed from the client.  Can we
delay how we hit the callback by 0.5 seconds or something?  That already has a
callback_.Run(), now it'll be PostDelayedTask(callback_, blah blah,
base::TimeDelta::FromMilliseconds(500))?

[shaktisahu, 2016-10-27 02:42:56]

dtrainor@ - PTAL

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/drawable/dotted_line.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:1: <?xml
version="1.0" encoding="UTF-8"?>
On 2016/10/19 16:40:30, David Trainor wrote:
> Is this the right place for this file?  Have we moved our resources yet?

It seems so. Chrome keeps its XML drawable files in
chrome/android/java/res/drawable/. 
I think all our resources are still in blimp/client/app.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:16: <padding
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Odd indenting

Done.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/layout/text_input_popup.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:9: <TextView
On 2016/10/19 16:40:30, David Trainor wrote:
> Are we adding the label here as well?  Or is this for a different kind of
label?

Yea, this is the same label used above the edit text. The text on this label
will be set from Java.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:15:
android:textSize="12sp"/>
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Should we be setting color/font/etc?

I think we should clean these up to use a few defined styles. Also the roboto
fonts are supported only in newer versions. Need to define different styles for
different versions.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:27: <ProgressBar
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Are we relying on the OK/CANCEL buttons from the alert dialog, but hiding them
> for this?  Would it be cleaner to keep both in the xml so we can eventually do
> the slide animations?

Done. Changed to a generic dialog and moved them to a different panel.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/values/colors.xml:10: <color
name="disabled_text_color">#808080</color>
On 2016/10/19 16:40:30, David Trainor wrote:
> Doesn't look like this is ever used for text?

It is used in OnImeTextEntered().

mEditText.setTextColor(R.color.disabled_text_color);

https://codereview.chromium.org/2393443004/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/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
public class ImeHelperDialog {
On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> We should probably rename this or look at restructuring the code in a bit. 
This
> class isn't actually a dialog :).

Yea.. Probably rename to something else, but this class is still all about
driving a dialog. :)

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:40:
private ProgressBar mProgressDialog;
On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> Dialog seems like an incorrect name

Done. Removed field.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:41:
private long mNativeImeHelperDialog;
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Can you pull the pointer out separately from the java-specific members?

Done.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:72:
if (text.equals(mExistingText)) {
On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> We still need to tell the engine that we're dismissing the IME, we just don't
> need to send text to it or trigger the spinner.

Why? Is it for the state sync? Anyway, I think handling this condition is
unnecessary. I will remove this.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:82:
mAlertDialog.getButton(DialogInterface.BUTTON_POSITIVE).setVisibility(View.GONE);
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Let's talk about this today.  It might be clean to write a simple widget that
> understands how to swap those buttons with a progress bar, that way we can
reuse
> it for all types of input/eventual UI if we need to?

Done.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:87:
mEditText.setBackgroundResource(R.drawable.dotted_line);
On 2016/10/19 16:40:31, David Trainor wrote:
> Do we have a blue line for normal entry?

Yes, the blue line comes with the default theme.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:112:
final TextView tvLabel = (TextView) viewPopup.findViewById(R.id.label);
On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> Can we have this and the set text lines close together?

Done.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:126:
tvLabel.setText(R.string.blimp_ime_dialog_title);
On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> Should this be called "web_input_default_label"?  This will be overridden by
the
> info coming from the site if available right?

Done. Yes.

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:237:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2016/10/19 16:40:31, David Trainor wrote:
> Actually I think it's cleaner if we send this delayed from the client.  Can we
> delay how we hit the callback by 0.5 seconds or something?  That already has a
> callback_.Run(), now it'll be PostDelayedTask(callback_, blah blah,
> base::TimeDelta::FromMilliseconds(500))?

Actually, today, engine is not sending a HIDE_IME request which will cause the
spinner to spin indefinitely.

[David Trainor- moved to gerrit, 2016-10-28 05:08:09]

This is looking really good!  Some more nits but it's almost ready.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/drawable/dotted_line.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:1: <?xml
version="1.0" encoding="UTF-8"?>
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:30, David Trainor wrote:
> > Is this the right place for this file?  Have we moved our resources yet?
> 
> It seems so. Chrome keeps its XML drawable files in
> chrome/android/java/res/drawable/. 
> I think all our resources are still in blimp/client/app.

Ok.  Confirm with xingliu@ as he's been moving some stuff around.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/layout/text_input_popup.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:15:
android:textSize="12sp"/>
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> > Should we be setting color/font/etc?
> 
> I think we should clean these up to use a few defined styles. Also the roboto
> fonts are supported only in newer versions. Need to define different styles
for
> different versions.

Sounds good!

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/text_input_popup.xml:27: <ProgressBar
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> > Are we relying on the OK/CANCEL buttons from the alert dialog, but hiding
them
> > for this?  Would it be cleaner to keep both in the xml so we can eventually
do
> > the slide animations?
> 
> Done. Changed to a generic dialog and moved them to a different panel.

I like the name :).

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/res/values/colors.xml:10: <color
name="disabled_text_color">#808080</color>
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:30, David Trainor wrote:
> > Doesn't look like this is ever used for text?
> 
> It is used in OnImeTextEntered().
> 
> mEditText.setTextColor(R.color.disabled_text_color);

Acknowledged.

https://codereview.chromium.org/2393443004/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/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
public class ImeHelperDialog {
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> > We should probably rename this or look at restructuring the code in a bit. 
> This
> > class isn't actually a dialog :).
> 
> Yea.. Probably rename to something else, but this class is still all about
> driving a dialog. :)

Yeah good point.  Although I'd be fine with naming it something that describes
what it does to someone who doesn't know the internals.  But I'm okay if you
want to call it WebFormDialogDriver as well ;).

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:72:
if (text.equals(mExistingText)) {
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:31, David Trainor (OOO to oct 31) wrote:
> > We still need to tell the engine that we're dismissing the IME, we just
don't
> > need to send text to it or trigger the spinner.
> 
> Why? Is it for the state sync? Anyway, I think handling this condition is
> unnecessary. I will remove this.

Yeah we'll have to do it for state sync, but that's a good point I don't know
what the implications are in the blink layer.  But for state sync we'll have to
mark the web form UI as hidden.

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:82:
mAlertDialog.getButton(DialogInterface.BUTTON_POSITIVE).setVisibility(View.GONE);
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:30, David Trainor (OOO to oct 31) wrote:
> > Let's talk about this today.  It might be clean to write a simple widget
that
> > understands how to swap those buttons with a progress bar, that way we can
> reuse
> > it for all types of input/eventual UI if we need to?
> 
> Done.

w00t!

https://codereview.chromium.org/2393443004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:87:
mEditText.setBackgroundResource(R.drawable.dotted_line);
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:31, David Trainor wrote:
> > Do we have a blue line for normal entry?
> 
> Yes, the blue line comes with the default theme.

Acknowledged.

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:237:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2016/10/27 02:42:56, shaktisahu wrote:
> On 2016/10/19 16:40:31, David Trainor wrote:
> > Actually I think it's cleaner if we send this delayed from the client.  Can
we
> > delay how we hit the callback by 0.5 seconds or something?  That already has
a
> > callback_.Run(), now it'll be PostDelayedTask(callback_, blah blah,
> > base::TimeDelta::FromMilliseconds(500))?
> 
> Actually, today, engine is not sending a HIDE_IME request which will cause the
> spinner to spin indefinitely.

Yeah I was assuming we'd just send the hide right away, but it's a good point
that we can just add the delay here since we already have to call the hide
method.

Can you file a bug and tag it here?

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/app/androi...
File blimp/client/app/android/java/res/layout/web_input_bottom_panel.xml
(right):

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/web_input_bottom_panel.xml:6: <merge
xmlns:android="http://schemas.android.com/apk/res/android"
org.chromium.blimp.core.contents.input.WebInputConfirmationPanel?

https://codereview.chromium.org/2393443004/diff/60001/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/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
private String mExistingText;
Put the static finals first.  Then the finals second.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:26:
private boolean mSubmitForm;
Do we need this here?  This class shouldn't really concern itself with the web
form stuff.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:31:
public interface Listener {
Put this above all variables

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:53:
inflate(context, R.layout.web_input_bottom_panel, this);
If you address the class in the xml file you shouldn't need to inflate here.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:67:
private void intializeAnimators() {
If you make the suggested comment above, put this in "onFinishInflate()"

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:77:
ObjectAnimator okTranslateAnim = ObjectAnimator.ofFloat(ok, View.TRANSLATION_X,
0.f, 105.f);
IIRC this is in pixels.  Convert 105 -> dp?  Pull this out and the durations to
constants up top so they're easy to tweak?

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:96:
mListener.onCancel();
Check if listener is null?

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:120:
mSubmitForm = submitForm;
Can we decouple the animation from the button?  Let's just do something like:

(no clue on the name)

moveToWaitState() {
  animatorSet.start();
}

Then have the ok button call moveToWaitState().

[shaktisahu, 2016-11-01 21:35:34]

dtrainor@ - PTAL

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/20001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:237:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> On 2016/10/27 02:42:56, shaktisahu wrote:
> > On 2016/10/19 16:40:31, David Trainor wrote:
> > > Actually I think it's cleaner if we send this delayed from the client. 
Can
> we
> > > delay how we hit the callback by 0.5 seconds or something?  That already
has
> a
> > > callback_.Run(), now it'll be PostDelayedTask(callback_, blah blah,
> > > base::TimeDelta::FromMilliseconds(500))?
> > 
> > Actually, today, engine is not sending a HIDE_IME request which will cause
the
> > spinner to spin indefinitely.
> 
> Yeah I was assuming we'd just send the hide right away, but it's a good point
> that we can just add the delay here since we already have to call the hide
> method.
> 
> Can you file a bug and tag it here?

Done.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/app/androi...
File blimp/client/app/android/java/res/layout/web_input_bottom_panel.xml
(right):

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/app/androi...
blimp/client/app/android/java/res/layout/web_input_bottom_panel.xml:6: <merge
xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> org.chromium.blimp.core.contents.input.WebInputConfirmationPanel?

Somehow, I crash during inflating the parent layout if I include the full class
here.

https://codereview.chromium.org/2393443004/diff/60001/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/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:34:
private String mExistingText;
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> Put the static finals first.  Then the finals second.

Done.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:26:
private boolean mSubmitForm;
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> Do we need this here?  This class shouldn't really concern itself with the web
> form stuff.

Removed.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:31:
public interface Listener {
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> Put this above all variables

Done.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:53:
inflate(context, R.layout.web_input_bottom_panel, this);
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> If you address the class in the xml file you shouldn't need to inflate here.

Somehow, the LayoutInflater is unable to inflate it if directly included from
xml file.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:67:
private void intializeAnimators() {
On 2016/10/28 05:08:09, David Trainor (OOO to oct 31) wrote:
> If you make the suggested comment above, put this in "onFinishInflate()"

Changed this to onFinishInflate().

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:77:
ObjectAnimator okTranslateAnim = ObjectAnimator.ofFloat(ok, View.TRANSLATION_X,
0.f, 105.f);
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> IIRC this is in pixels.  Convert 105 -> dp?  Pull this out and the durations
to
> constants up top so they're easy to tweak?

Moving this to dimens.xml file.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:96:
mListener.onCancel();
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> Check if listener is null?

Done.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:120:
mSubmitForm = submitForm;
On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> Can we decouple the animation from the button?  Let's just do something like:
> 
> (no clue on the name)
> 
> moveToWaitState() {
>   animatorSet.start();
> }
> 
> Then have the ok button call moveToWaitState().

Done. Added a public method startAnimations() which can be called to show only
animations.
Now the form submission logic is removed from this class.

[David Trainor- moved to gerrit, 2016-11-03 03:07:55]

looks pretty good.  last few nits i think.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:53:
inflate(context, R.layout.web_input_bottom_panel, this);
On 2016/11/01 21:35:33, shaktisahu wrote:
> On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> > If you address the class in the xml file you shouldn't need to inflate here.
> 
> Somehow, the LayoutInflater is unable to inflate it if directly included from
> xml file.

Acknowledged.

https://codereview.chromium.org/2393443004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:77:
ObjectAnimator okTranslateAnim = ObjectAnimator.ofFloat(ok, View.TRANSLATION_X,
0.f, 105.f);
On 2016/11/01 21:35:33, shaktisahu wrote:
> On 2016/10/28 05:08:08, David Trainor (OOO to oct 31) wrote:
> > IIRC this is in pixels.  Convert 105 -> dp?  Pull this out and the durations
> to
> > constants up top so they're easy to tweak?
> 
> Moving this to dimens.xml file.

Keeping the constants here is ok, but you just need to pull the screen density
and multiply it.

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/app/androi...
File blimp/client/app/android/java/res/drawable/dotted_line.xml (right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:8: <item
android:top="-2dp" android:right="-2dp" android:left="-2dp">
indent in?

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:65:
final Button ok = (Button) findViewById(R.id.btn_ok);
Should we just do all of this inside "startAnimation"?  wdyt?

https://codereview.chromium.org/2393443004/diff/80001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:242:
base::Unretained(this), target_tab_id,
Unretained is unsafe here.  Would be good to use a weakptr just for safety
reasons, even if we're going to delete this eventually.

[shaktisahu, 2016-11-03 18:10:04]

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/app/androi...
File blimp/client/app/android/java/res/drawable/dotted_line.xml (right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/app/androi...
blimp/client/app/android/java/res/drawable/dotted_line.xml:8: <item
android:top="-2dp" android:right="-2dp" android:left="-2dp">
On 2016/11/03 03:07:54, David Trainor wrote:
> indent in?

Done.

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:65:
final Button ok = (Button) findViewById(R.id.btn_ok);
On 2016/11/03 03:07:54, David Trainor wrote:
> Should we just do all of this inside "startAnimation"?  wdyt?

We would still have to setup the OnClickListeners for ok/cancel buttons here.
I am okay either way.

https://codereview.chromium.org/2393443004/diff/80001/blimp/engine/feature/en...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/80001/blimp/engine/feature/en...
blimp/engine/feature/engine_render_widget_feature.cc:242:
base::Unretained(this), target_tab_id,
On 2016/11/03 03:07:55, David Trainor wrote:
> Unretained is unsafe here.  Would be good to use a weakptr just for safety
> reasons, even if we're going to delete this eventually.

Done.

[David Trainor- moved to gerrit, 2016-11-04 21:10:20]

lgtm % nit

https://codereview.chromium.org/2393443004/diff/100001/blimp/client/core/cont...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java
(right):

https://codereview.chromium.org/2393443004/diff/100001/blimp/client/core/cont...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/WebInputConfirmationPanel.java:100:
int translateX =
getResources().getDimensionPixelSize(R.dimen.dialog_button_animation_x);
Not sure if it makes sense to move this to dimens.  You could just pull the
density and multiply by it.  If you want to keep it in dimen pick a more
specific name IMO.

[shaktisahu, 2016-11-04 23:34:51]

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

[shaktisahu, 2016-11-04 23:34:52]

mikecase@chromium.org: PTAL at suppressions.xml

[mikecase (-- gone --), 2016-11-04 23:39:11]

suppressions.xml lgtm with nit

https://codereview.chromium.org/2393443004/diff/100001/build/android/lint/sup...
File build/android/lint/suppressions.xml (right):

https://codereview.chromium.org/2393443004/diff/100001/build/android/lint/sup...
build/android/lint/suppressions.xml:293: <ignore
regexp="blimp/client/app/android/java/res/drawable/dotted_line.xml" />
nit: alphabetize

[shaktisahu, 2016-11-04 23:49:44]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-11-04 23:49:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mikecase@chromium.org
Link to the patchset: https://codereview.chromium.org/2393443004/#ps120001
(title: "nits")

[commit-bot: I haz the power, 2016-11-04 23:50:29]

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-11-05 00:53:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-05 00:56:10]

Description was changed from

==========
Blimp: Added spinner to text input dialog

Added a spinner (ProgressBar with indeterminate style) to the text input
AlertDialog. The spinner is shown when user hits enter/OK, and is
dismissed on receiving a hide IME request or if the user hasn't changed
the text or cancelling the dialog through back button/touching outside.
During submission, the EditText background changes to a dotted line.

In an unrelated change, fixed the IME editor options (following similar
structure as ImeUtils.java). This enables multi-line input for text area.

BUG=652959
==========

to

==========
Blimp: Added spinner to text input dialog

Added a spinner (ProgressBar with indeterminate style) to the text input
AlertDialog. The spinner is shown when user hits enter/OK, and is
dismissed on receiving a hide IME request or if the user hasn't changed
the text or cancelling the dialog through back button/touching outside.
During submission, the EditText background changes to a dotted line.

In an unrelated change, fixed the IME editor options (following similar
structure as ImeUtils.java). This enables multi-line input for text area.

BUG=652959

Committed: https://crrev.com/3d1c669b5c8b03dc232859960adb5d571c4c76db
Cr-Commit-Position: refs/heads/master@{#430105}
==========

[commit-bot: I haz the power, 2016-11-05 00:56:12]

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

[Wez, 2016-11-07 22:51:51]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2016-11-07 22:51:52]

https://codereview.chromium.org/2393443004/diff/120001/blimp/engine/feature/e...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393443004/diff/120001/blimp/engine/feature/e...
blimp/engine/feature/engine_render_widget_feature.cc:244:
base::TimeDelta::FromMilliseconds(1500));
What's the rationale for the delay here?