Handle saved password deletion when preference has been recreated

Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370
Committed: https://crrev.com/2ad9de17fc7177a7ca3a87118447d4871447dd21
Cr-Commit-Position: refs/heads/master@{#368785}

[Theresa, 2016-01-08 23:49:19]

Description was changed from

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, if the password list is
not available when the request to delete the password or password
exceptoin occurs, deleting is delayed until the password list is
available.

BUG=575370
==========

to

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated when returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available, and PasswordManagerPresenter
would fail to delete the password. Now, if the password list is
not available when the request to delete the password or password
exception occurs, deleting is delayed until the password list is
available.

BUG=575370
==========

[Theresa, 2016-01-08 23:56:28]

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

[Theresa, 2016-01-08 23:56:29]

ptal

[newt (away), 2016-01-11 19:47:20]

This is a reasonable fix, but I think it makes this already error-prone class
even harder to reason about. What do you think of my suggestion below?

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77:
// Variables to assist with deleting a password if the PreferenceFragment is
being re-created
This adds yet more state complexity to this class, which already has state
that's rather difficult to reason about.

How about encapsulating the deletion logic, possibly inside of
PasswordEntryEditor. E.g. PasswordEntryEditor.removeItem() could be written as:

  private void removeItem() {
      PasswordUIView passwordUIView = new PasswordUIView();
      PasswordListObserver passwordDeleter = new PasswordListObserver() {
          @Override
          public void passwordListAvailable(int count) {
              // delete the password
              // destroy passwordUIView
          }
          ...
      }

      passwordUIView.addObserver(this);
      passwordUIView.updatePasswordLists();
  }

[Theresa, 2016-01-11 23:23:41]

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77:
// Variables to assist with deleting a password if the PreferenceFragment is
being re-created
On 2016/01/11 19:47:20, newt wrote:
> This adds yet more state complexity to this class, which already has state
> that's rather difficult to reason about.
> 
> How about encapsulating the deletion logic, possibly inside of
> PasswordEntryEditor. E.g. PasswordEntryEditor.removeItem() could be written
as:
> 
>   private void removeItem() {
>       PasswordUIView passwordUIView = new PasswordUIView();
>       PasswordListObserver passwordDeleter = new PasswordListObserver() {
>           @Override
>           public void passwordListAvailable(int count) {
>               // delete the password
>               // destroy passwordUIView
>           }
>           ...
>       }
> 
>       passwordUIView.addObserver(this);
>       passwordUIView.updatePasswordLists();
>   }  

That would work too, but I think there are performance implications. In the case
where SavePasswordsPreferences has been destroyed and re-created from a saved
state, we have to retrieve the password list anyway so it doesn't really matter
if we update the password list in PasswordEntryEditor or
SavePasswordsPreferences. In the case where the activity isn't re-created (e.g.
on a newer device w/ more memory), requesting the updated password list here
instead of in SavePasswordsPreferences results in extra (unnecessary) work.

Would it make sense to make PasswordUIView parcelable and sending it with the
intent that creates the PasswordEntryEditor to avoid recreating?

[newt (away), 2016-01-11 23:34:41]

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77:
// Variables to assist with deleting a password if the PreferenceFragment is
being re-created
On 2016/01/11 23:23:41, Theresa Wellington wrote:
> On 2016/01/11 19:47:20, newt wrote:
> > This adds yet more state complexity to this class, which already has state
> > that's rather difficult to reason about.
> > 
> > How about encapsulating the deletion logic, possibly inside of
> > PasswordEntryEditor. E.g. PasswordEntryEditor.removeItem() could be written
> as:
> > 
> >   private void removeItem() {
> >       PasswordUIView passwordUIView = new PasswordUIView();
> >       PasswordListObserver passwordDeleter = new PasswordListObserver() {
> >           @Override
> >           public void passwordListAvailable(int count) {
> >               // delete the password
> >               // destroy passwordUIView
> >           }
> >           ...
> >       }
> > 
> >       passwordUIView.addObserver(this);
> >       passwordUIView.updatePasswordLists();
> >   }  
> 
> That would work too, but I think there are performance implications.

How long does it take to query the list from C++ on a typical slow device? This
isn't a performance sensitive area, so unless it's quite slow, I wouldn't worry
about it. The overhead of transitioning between activities may well dwarf the
time spent deleting the password.

> In the case
> where SavePasswordsPreferences has been destroyed and re-created from a saved
> state, we have to retrieve the password list anyway so it doesn't really
matter
> if we update the password list in PasswordEntryEditor or
> SavePasswordsPreferences. In the case where the activity isn't re-created
(e.g.
> on a newer device w/ more memory), requesting the updated password list here
> instead of in SavePasswordsPreferences results in extra (unnecessary) work.
> 
> Would it make sense to make PasswordUIView parcelable and sending it with the
> intent that creates the PasswordEntryEditor to avoid recreating? 

Perhaps. Android may serialize and deserialize Parcelable objects in the process
of sending the intent though (I'm not sure), in which case this wouldn't give
any benefit.

[Theresa, 2016-01-12 00:09:40]

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:77:
// Variables to assist with deleting a password if the PreferenceFragment is
being re-created
On 2016/01/11 23:34:41, newt wrote:
> On 2016/01/11 23:23:41, Theresa Wellington wrote:
> > On 2016/01/11 19:47:20, newt wrote:
> > > This adds yet more state complexity to this class, which already has state
> > > that's rather difficult to reason about.
> > > 
> > > How about encapsulating the deletion logic, possibly inside of
> > > PasswordEntryEditor. E.g. PasswordEntryEditor.removeItem() could be
written
> > as:
> > > 
> > >   private void removeItem() {
> > >       PasswordUIView passwordUIView = new PasswordUIView();
> > >       PasswordListObserver passwordDeleter = new PasswordListObserver() {
> > >           @Override
> > >           public void passwordListAvailable(int count) {
> > >               // delete the password
> > >               // destroy passwordUIView
> > >           }
> > >           ...
> > >       }
> > > 
> > >       passwordUIView.addObserver(this);
> > >       passwordUIView.updatePasswordLists();
> > >   }  
> > 
> > That would work too, but I think there are performance implications.
> 
> How long does it take to query the list from C++ on a typical slow device?
This
> isn't a performance sensitive area, so unless it's quite slow, I wouldn't
worry
> about it. The overhead of transitioning between activities may well dwarf the
> time spent deleting the password.
> 
> > In the case
> > where SavePasswordsPreferences has been destroyed and re-created from a
saved
> > state, we have to retrieve the password list anyway so it doesn't really
> matter
> > if we update the password list in PasswordEntryEditor or
> > SavePasswordsPreferences. In the case where the activity isn't re-created
> (e.g.
> > on a newer device w/ more memory), requesting the updated password list here
> > instead of in SavePasswordsPreferences results in extra (unnecessary) work.
> > 
> > Would it make sense to make PasswordUIView parcelable and sending it with
the
> > intent that creates the PasswordEntryEditor to avoid recreating? 
> 
> Perhaps. Android may serialize and deserialize Parcelable objects in the
process
> of sending the intent though (I'm not sure), in which case this wouldn't give
> any benefit.

Re how long - I haven't timed it (primarily because logging in to more than ~6
sites and saving passwords takes a long time and timing with that few seems less
interesting than timing with hundreds of saved passwords). Ultimately we call
Connection::GetCachedStatement to get the password list, so it shouldn't take
too long to query the list from C++. We get all of the pw data via
GetCachedStatement but only return the count to Java, rather than all of the pw
data in the UpdatePasswordsList request.


Re parcelable - I think it will recreate the PasswordUI view using
Parcelable.Creator.createFromParcel; in this case we would just set
mNativePasswordUIViewAndroid equal to whatever the parcel has rather than
calling nativeInit(). A new C++ password_manager_creator won't need to get
created, so the password lists in that class (in theory) won't have size 0.

[Theresa, 2016-01-12 02:08:49]

Description was changed from

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated when returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available, and PasswordManagerPresenter
would fail to delete the password. Now, if the password list is
not available when the request to delete the password or password
exception occurs, deleting is delayed until the password list is
available.

BUG=575370
==========

to

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370
==========

[Theresa, 2016-01-12 02:11:26]

ptal - made discussed changed

[newt (away), 2016-01-12 02:15:42]

lgtm after one comment

Thanks!

https://codereview.chromium.org/1571513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:65:
public static final int RESULT_DELETE_PASSWORD = 1;
looks like RESULT_DELETE_PASSWORD can be deleted too (and change
startActivityForResult() to plain startActivity())

[Theresa, 2016-01-12 02:21:31]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-01-12 02:21:31]

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

[Theresa, 2016-01-12 02:21:46]

https://codereview.chromium.org/1571513003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java
(right):

https://codereview.chromium.org/1571513003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:65:
public static final int RESULT_DELETE_PASSWORD = 1;
On 2016/01/12 02:15:42, newt wrote:
> looks like RESULT_DELETE_PASSWORD can be deleted too (and change
> startActivityForResult() to plain startActivity())

Done.

[commit-bot: I haz the power, 2016-01-12 02:21:46]

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

[commit-bot: I haz the power, 2016-01-12 03:04:42]

Description was changed from

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370
==========

to

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370
==========

[commit-bot: I haz the power, 2016-01-12 03:04:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-12 03:05:55]

Description was changed from

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370
==========

to

==========
Handle saved password deletion when preference has been recreated

Previously, if the PreferenceFragment was recreated after returning
to the preference after attempting to delete a saved password,
the password list wouldn't be available and PasswordManagerPresenter
would fail to delete the password. Now, password deletion is handled
by PasswordEntryEditor, which always updates the password list
before attempting to delete.

BUG=575370

Committed: https://crrev.com/2ad9de17fc7177a7ca3a87118447d4871447dd21
Cr-Commit-Position: refs/heads/master@{#368785}
==========

[commit-bot: I haz the power, 2016-01-12 03:05:56]

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