Autofill item deletion on android

Autofill item deletion on android

You can now delete things by long pressing suggestions in the dropdown (assuming it's the right kind of suggestion).

BUG=486153
Committed: https://crrev.com/c806c71f4a9e704d40b14df026a6ebfa0c2fcac1
Cr-Commit-Position: refs/heads/master@{#330413}

[Evan Stade, 2015-05-13 23:32:58]

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

[Ted C, 2015-05-14 21:56:40]

overall seems good to me...didn't think this would be implemented so quickly :-)

That's what I get for typing something stupid into a form and having no way to
delete it.  Progress!

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
(right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:32:
private AlertDialog mDeletionDialog = null;
No need for = null; as it is the default

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:74:
public void deleteSuggestion(int listIndex) {
from my comment in the c++ side, I think it would be better to just keep the
index here and ask for the deletion text, save the index in this side since it
seems a bit unnecessary making native track it.

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:120:
.setNegativeButton(R.string.cancel, this)
do you know if the listener can be null in this case since we don't actually
need to handle it?

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:106: void
AutofillPopupViewAndroid::DeleteSuggestion(JNIEnv* env,
Why does it need to call down to this for deletion?  Can't the index be tracked
purely in javaland?

Seems like all we really need to expose is a method to get the removal
confirmation text body.

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:56: public void
deleteSuggestion(int listIndex);
javadoc

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:113:
//Log.e("estade", "long click found");
:-)

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java
(right):

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java:17: private
boolean mDeletable;
final

[Evan Stade, 2015-05-14 23:12:06]

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
(right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:32:
private AlertDialog mDeletionDialog = null;
On 2015/05/14 21:56:39, Ted C wrote:
> No need for = null; as it is the default

Done.

https://codereview.chromium.org/1134793004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:120:
.setNegativeButton(R.string.cancel, this)
On 2015/05/14 21:56:39, Ted C wrote:
> do you know if the listener can be null in this case since we don't actually
> need to handle it?

seems to work

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:106: void
AutofillPopupViewAndroid::DeleteSuggestion(JNIEnv* env,
On 2015/05/14 21:56:39, Ted C wrote:
> Why does it need to call down to this for deletion?  Can't the index be
tracked
> purely in javaland?
> 
> Seems like all we really need to expose is a method to get the removal
> confirmation text body.

The title as well as the body, but sure, that's a possible alternative. What is
the main advantage? It seems functionally pretty similar.

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:56: public void
deleteSuggestion(int listIndex);
On 2015/05/14 21:56:40, Ted C wrote:
> javadoc

Done.

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:113:
//Log.e("estade", "long click found");
On 2015/05/14 21:56:40, Ted C wrote:
> :-)

ah, good thing I didn't write a more embarrassing debug message

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java
(right):

https://codereview.chromium.org/1134793004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java:17: private
boolean mDeletable;
On 2015/05/14 21:56:40, Ted C wrote:
> final

Done.

[Ted C, 2015-05-14 23:21:19]

lgtm

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:106: void
AutofillPopupViewAndroid::DeleteSuggestion(JNIEnv* env,
On 2015/05/14 23:12:05, Evan Stade wrote:
> On 2015/05/14 21:56:39, Ted C wrote:
> > Why does it need to call down to this for deletion?  Can't the index be
> tracked
> > purely in javaland?
> > 
> > Seems like all we really need to expose is a method to get the removal
> > confirmation text body.
> 
> The title as well as the body, but sure, that's a possible alternative. What
is
> the main advantage? It seems functionally pretty similar.

To me it's just a bit less jumping around.  In most cases, I would just keep the
id local to the callback that will handle the delete.

The fact that you need both the title and body and it is a complex method, I
think this is ok.

I might rename this method to something like triggerDeleteConfirmation or
something since deleteSuggestion makes me think it's actually doing the delete.

[Evan Stade, 2015-05-15 16:49:59]

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/1134793004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:106: void
AutofillPopupViewAndroid::DeleteSuggestion(JNIEnv* env,
On 2015/05/14 23:21:18, Ted C wrote:
> On 2015/05/14 23:12:05, Evan Stade wrote:
> > On 2015/05/14 21:56:39, Ted C wrote:
> > > Why does it need to call down to this for deletion?  Can't the index be
> > tracked
> > > purely in javaland?
> > > 
> > > Seems like all we really need to expose is a method to get the removal
> > > confirmation text body.
> > 
> > The title as well as the body, but sure, that's a possible alternative. What
> is
> > the main advantage? It seems functionally pretty similar.
> 
> To me it's just a bit less jumping around.  In most cases, I would just keep
the
> id local to the callback that will handle the delete.
> 
> The fact that you need both the title and body and it is a complex method, I
> think this is ok.
> 
> I might rename this method to something like triggerDeleteConfirmation or
> something since deleteSuggestion makes me think it's actually doing the
delete.

changed to DeletionRequested. Thanks!

[Evan Stade, 2015-05-15 16:50:06]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-05-15 16:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1134793004/#ps60001
(title: "DeletionRequested")

[commit-bot: I haz the power, 2015-05-15 16:51:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134793004/60001

[commit-bot: I haz the power, 2015-05-15 17:00:11]

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

[commit-bot: I haz the power, 2015-05-15 17:00:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2015-05-15 17:48:08]

estade@chromium.org changed reviewers:
+ aurimas@chromium.org, gcasto@chromium.org, sgurun@chromium.org

[Evan Stade, 2015-05-15 17:48:09]

OWNERS reviews please

sgurun:
  android_webview/java/src/org/chromium/android_webview/AwAutofillClient.java
aurimas:
 
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
 
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
  chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
  chrome/browser/ui/android/autofill/autofill_popup_view_android.h
  ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java
  ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java
gcasto:
  components/password_manager/core/browser/password_autofill_manager.cc
  components/password_manager/core/browser/password_autofill_manager.h

[Garrett Casto, 2015-05-15 17:51:01]

components/password_manager/* LGTM

[sgurun-gerrit only, 2015-05-15 17:52:08]

On 2015/05/15 17:51:01, Garrett Casto wrote:
> components/password_manager/* LGTM

lgtm

[aurimas (slooooooooow), 2015-05-15 18:02:38]

LGTM for 

chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
chrome/browser/ui/android/autofill/autofill_popup_view_android.h
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java
ui/android/java/src/org/chromium/ui/autofill/AutofillSuggestion.java

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

https://codereview.chromium.org/1134793004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:29:
DialogInterface.OnClickListener {
I think this fits on the line above?

[Evan Stade, 2015-05-15 18:41:06]

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

https://codereview.chromium.org/1134793004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:29:
DialogInterface.OnClickListener {
On 2015/05/15 18:02:37, aurimas wrote:
> I think this fits on the line above?

Done.

[Evan Stade, 2015-05-15 18:41:10]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-05-15 18:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sgurun@chromium.org, aurimas@chromium.org,
gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1134793004/#ps100001
(title: "grit whitelist")

[commit-bot: I haz the power, 2015-05-15 18:42:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134793004/100001

[commit-bot: I haz the power, 2015-05-15 19:13:36]

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

[commit-bot: I haz the power, 2015-05-15 19:13:38]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Evan Stade, 2015-05-15 23:36:18]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-05-15 23:36:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sgurun@chromium.org, aurimas@chromium.org,
gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1134793004/#ps120001
(title: "aw")

[commit-bot: I haz the power, 2015-05-15 23:39:58]

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

[commit-bot: I haz the power, 2015-05-16 00:15:33]

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

[commit-bot: I haz the power, 2015-05-16 00:15:34]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Evan Stade, 2015-05-18 19:32:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-05-18 19:32:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sgurun@chromium.org, aurimas@chromium.org,
gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/1134793004/#ps140001
(title: "test fix")

[commit-bot: I haz the power, 2015-05-18 19:33:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134793004/140001

[commit-bot: I haz the power, 2015-05-18 20:51:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-18 22:42:58]

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