Remove items on long press in keyboard accessory.

Remove items on long press in keyboard accessory.

BUG=428087
Committed: https://crrev.com/c752712ead6a6fbf2160786d29137bf3455bc091
Cr-Commit-Position: refs/heads/master@{#343764}

[please use gerrit instead, 2015-08-10 19:51:55]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2015-08-10 19:53:05]

rouslan@chromium.org changed reviewers:
+ newt@chromium.org

[please use gerrit instead, 2015-08-10 19:53:06]

Newton, PTAL Patch Set 1. The implementation is identical to autofill popup,
which will be going away eventually.

[newt (away), 2015-08-10 20:39:37]

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:10:
android:longClickable="true"
This isn't needed. setOnLongClickListener() automatically sets the view to be
long-clickable.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillDelegate.java (right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillDelegate.java:12: * Informs
the controller the AutofillKeyboardAccessory was hidden.
"AutofillKeyboardAccessory or AutofillPopup", right?  Same below.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:42:
WindowAndroid windowAndroid, AutofillDelegate autofillCallback) {
s/callback/delegate. Same for the member variable mAutofillCallback.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:151:
mAutofillCallback.deleteSuggestion(i);
If this LinearLayout ever contains child views other than the suggestion items
(e.g. an ImageButton to launch settings), then this logic will break.

Also, does showWithSuggestions() get called when a suggestion is deleted? If
not, interacting with the suggestion views after one has been deleted will be
broken.

[please use gerrit instead, 2015-08-11 19:09:48]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2015-08-11 19:09:53]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2015-08-11 20:42:20]

Newton, PTAL Patch Set 2.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/autofill_keyboard_accessory_item.xml (right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/autofill_keyboard_accessory_item.xml:10:
android:longClickable="true"
On 2015/08/10 20:39:36, newt wrote:
> This isn't needed. setOnLongClickListener() automatically sets the view to be
> long-clickable.

Done.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillDelegate.java (right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillDelegate.java:12: * Informs
the controller the AutofillKeyboardAccessory was hidden.
On 2015/08/10 20:39:36, newt wrote:
> "AutofillKeyboardAccessory or AutofillPopup", right?  Same below.

Done.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:42:
WindowAndroid windowAndroid, AutofillDelegate autofillCallback) {
On 2015/08/10 20:39:37, newt wrote:
> s/callback/delegate. Same for the member variable mAutofillCallback.

Done.

https://codereview.chromium.org/1281323003/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:151:
mAutofillCallback.deleteSuggestion(i);
On 2015/08/10 20:39:37, newt wrote:
> If this LinearLayout ever contains child views other than the suggestion items
> (e.g. an ImageButton to launch settings), then this logic will break.

This is prevented by calling setOnLongClickListener(this) only for deletable
items. The item that launches settings is not deletable.

> Also, does showWithSuggestions() get called when a suggestion is deleted? If
> not, interacting with the suggestion views after one has been deleted will be
> broken.

If more suggestions remain, the keyboard accessory is updated via
showWithSuggestions(), such that the removed suggestion is no longer showing. If
no suggestions remain, the keyboard accessory is hidden.

[newt (away), 2015-08-11 21:31:32]

lgtm after question

https://codereview.chromium.org/1281323003/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1281323003/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:151:
mAutofillDelegate.deleteSuggestion(i);
Consider this scenario:

  AutofillKeyboardAccessory
    - Child 0: settings button
    - Child 1: "Homer Simpson"
    - Child 2: "Fred Flintstone"

If the user long presses on "Fred Flintstone", this code will call
deleteSuggestion(2). Is 2 is the correct index to pass to deleteSuggestion() in
this case?

[please use gerrit instead, 2015-08-11 22:21:12]

Please see my answer clarifying question inline. Not submitting yet.

On 2015/08/11 21:31:32, newt wrote:
> Consider this scenario:
> 
>   AutofillKeyboardAccessory
>     - Child 0: settings button
>     - Child 1: "Homer Simpson"
>     - Child 2: "Fred Flintstone"
> 
> If the user long presses on "Fred Flintstone", this code will call
> deleteSuggestion(2). Is 2 is the correct index to pass to deleteSuggestion()
in
> this case?

You're right that this scenario will result in array-out-of-bounds condition,
because 2 is an index into this array

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

...which does not include utility items like "settings", "clear form", and "scan
card". The code for both autofill popup and keyboard accessory works because all
of the non deletable utility items go to the end of the list. I plan to put
"clear form" to the beginning of the list in one of the upcoming patches, but I
have not written it yet. Should I deal with this offset condition when "clear
form" is moved to the front or would you prefer me do it here in this patch?

[newt (away), 2015-08-11 22:55:20]

On 2015/08/11 22:21:12, Rouslan wrote:
> Please see my answer clarifying question inline. Not submitting yet.
> 
> On 2015/08/11 21:31:32, newt wrote:
> > Consider this scenario:
> > 
> >   AutofillKeyboardAccessory
> >     - Child 0: settings button
> >     - Child 1: "Homer Simpson"
> >     - Child 2: "Fred Flintstone"
> > 
> > If the user long presses on "Fred Flintstone", this code will call
> > deleteSuggestion(2). Is 2 is the correct index to pass to deleteSuggestion()
> in
> > this case?
> 
> You're right that this scenario will result in array-out-of-bounds condition,
> because 2 is an index into this array
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> ...which does not include utility items like "settings", "clear form", and
"scan
> card". The code for both autofill popup and keyboard accessory works because
all
> of the non deletable utility items go to the end of the list. I plan to put
> "clear form" to the beginning of the list in one of the upcoming patches, but
I
> have not written it yet. Should I deal with this offset condition when "clear
> form" is moved to the front or would you prefer me do it here in this patch?

Either way. Just don't forget :)

One easy way of fixing it is to attach the suggestion index associated with each
view using view.setTag().

  public void showWithSuggestions(...) {
      ...
      touchTarget.setTag(i);
      ...
  }

  public boolean onLongClick(View v) {
      mAutofillDelegate.deleteSuggestion((int) v.getTag());
  }

[please use gerrit instead, 2015-08-11 22:58:36]

Thank you for the suggestion. I will implement it when "clear form" moves to the
left side of the keyboard accessory. Sending out to a few more OWNERS now.

[please use gerrit instead, 2015-08-11 23:00:02]

rouslan@chromium.org changed reviewers:
+ boliu@chromium.org

[please use gerrit instead, 2015-08-11 23:00:03]

Bo, OWNERS PTAL in Patch Set 2:

android_webview/java/src/org/chromium/android_webview/AwAutofillClient.java

[please use gerrit instead, 2015-08-11 23:02:30]

rouslan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[please use gerrit instead, 2015-08-11 23:02:31]

Ted, OWNERS PTAL in Patch Set 2:

chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc

[please use gerrit instead, 2015-08-11 23:04:19]

rouslan@chromium.org changed reviewers:
+ yfriedman@chromium.org
- tedchoc@chromium.org

[please use gerrit instead, 2015-08-11 23:04:19]

Yaron, OWNERS PTAL in Patch Set 2:

chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc

(Ted and Aurimas are on vacation.)

[boliu, 2015-08-11 23:05:21]

android_webview rs lgtm

[Yaron, 2015-08-14 18:13:42]

sorry, somehow i missed this in my inbox

https://codereview.chromium.org/1281323003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1281323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:82:
mContext = windowAndroid.getActivity().get();
You're potentially leaking the activity.  Is this object guaranteed to be
deleted if the activity gets stopped?
Sorry, I'm not familiar with this code to know the owner/lifetime.

https://codereview.chromium.org/1281323003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1281323003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:103:
deleting_index_ = list_index;
i would much prefer it the way newt@ suggested of having it stored in the tag.
Also, is there not a better key that can be used (or is the index stable?). I
worry about the racey case of the backend getting updated in between the prompt
and ack.

[please use gerrit instead, 2015-08-14 20:23:31]

Yaron, PTAL Patch Set 3.

Newton, do you know whether it's OK to keep window's activity as a Context
inside of AutofillKeyboardAccessoryBridge?

https://codereview.chromium.org/1281323003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1281323003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:82:
mContext = windowAndroid.getActivity().get();
On 2015/08/14 18:13:42, Yaron wrote:
> You're potentially leaking the activity.  Is this object guaranteed to be
> deleted if the activity gets stopped?
> Sorry, I'm not familiar with this code to know the owner/lifetime.

Newton, do you know the answer to this question?

I am not familiar with how Chrome on Android handles its activity being stopped.
This object is owned by

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

...in case if that's helpful.

https://codereview.chromium.org/1281323003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1281323003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:103:
deleting_index_ = list_index;
On 2015/08/14 18:13:42, Yaron wrote:
> i would much prefer it the way newt@ suggested of having it stored in the tag.

Done.

> Also, is there not a better key that can be used (or is the index stable?). I
> worry about the racey case of the backend getting updated in between the
prompt
> and ack.

Webpage interaction is blocked after DeletionRequested(). Autofill updates only
on user gestures. Therefore, the suggestions array will remain unchanged between
DeletionRequested() and DeletionConfirmed(). Thus the index is stable.

[newt (away), 2015-08-14 20:50:11]

On 2015/08/14 20:23:31, Rouslan wrote:
> Yaron, PTAL Patch Set 3.
> 
> Newton, do you know whether it's OK to keep window's activity as a Context
> inside of AutofillKeyboardAccessoryBridge?

By the time the Activity (ChromeTabbedActivity, DocumentActivity,
WebAppActivity, etc) is destroyed, all the Tabs (and WebContents and
ContentViewCores) inside the activity will have been destroyed. Is it possible
that at that point, the AutofillKeyboardAccessoryBridge will still exist, or
will AutofillKeyboardAccessoryBridge also be destroyed when its tab is destroyed
(I would hope so)? If AutofillKeyboardAccessoryBridge can't live beyond the
activity's destruction, then it's OK to keep a reference to the activity in
AutofillKeyboardAccessoryBridge.

[please use gerrit instead, 2015-08-14 21:36:55]

On 2015/08/14 20:50:11, newt wrote:
> On 2015/08/14 20:23:31, Rouslan wrote:
> > Yaron, PTAL Patch Set 3.
> > 
> > Newton, do you know whether it's OK to keep window's activity as a Context
> > inside of AutofillKeyboardAccessoryBridge?
> 
> By the time the Activity (ChromeTabbedActivity, DocumentActivity,
> WebAppActivity, etc) is destroyed, all the Tabs (and WebContents and
> ContentViewCores) inside the activity will have been destroyed. Is it possible
> that at that point, the AutofillKeyboardAccessoryBridge will still exist, or
> will AutofillKeyboardAccessoryBridge also be destroyed when its tab is
destroyed
> (I would hope so)? If AutofillKeyboardAccessoryBridge can't live beyond the
> activity's destruction, then it's OK to keep a reference to the activity in
> AutofillKeyboardAccessoryBridge.

AutofillKeyboardAccessory (Java) lives inside of the window, so it's dismissed
when the window goes away:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

When AutofillKeyboardAccessory is dismissed, it notifies
AutofillKeyboardAccessoryView (C++) of its dismissal. At this point,
AutofillKeyboardAccessoryView deletes itself and the
AutofillKeyboardAccessoryBridge (Java) that it owns:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Therefore, AutofillKeyboardAccessoryBridge can't live beyond the activity's
destruction, right?

[Yaron, 2015-08-17 19:07:01]

lgtm

https://codereview.chromium.org/1281323003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1281323003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:98:
if (mAccessoryView != null) mAccessoryView.dismiss();
I think this should null out mContext to be certain. It mirrors the c++
destruction and should be safe to do so, while reducing the risk that this is
leaked.

[please use gerrit instead, 2015-08-17 21:50:35]

Sending to cq...

https://codereview.chromium.org/1281323003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1281323003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:98:
if (mAccessoryView != null) mAccessoryView.dismiss();
On 2015/08/17 19:07:01, Yaron (OOO 8.18-8.21) wrote:
> I think this should null out mContext to be certain. It mirrors the c++
> destruction and should be safe to do so, while reducing the risk that this is
> leaked.

Done.

[please use gerrit instead, 2015-08-17 21:50:54]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2015-08-17 21:50:54]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
newt@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1281323003/#ps120001
(title: "Null out context. Rebase.")

[commit-bot: I haz the power, 2015-08-17 21:52:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281323003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281323003/120001

[commit-bot: I haz the power, 2015-08-17 22:46:45]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-08-17 22:47:18]

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