Enable Update Password UI on iOS behind a flag

Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257
Committed: https://crrev.com/c020e00daee72eb145e2888c930f1c6381e59fb9
Cr-Commit-Position: refs/heads/master@{#403910}

[Jackie Quinn, 2016-07-05 13:11:27]

Description was changed from

==========
Enable update password UI

BUG=622244
==========

to

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622244
==========

[Jackie Quinn, 2016-07-05 13:11:27]

jyquinn@chromium.org changed reviewers:
+ vabr@chromium.org

[Jackie Quinn, 2016-07-05 13:11:47]

jyquinn@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Jackie Quinn, 2016-07-05 13:12:20]

vabr@ for passwords/, sdefresne for flags.

[sdefresne, 2016-07-05 13:29:55]

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:78: if
(ios_internal::experimental_flags::IsUpdatePasswordUIEnabled() &&
I think this should be "experimental_flags::IsUpdatePasswordUIEnabled()" (i.e.
without "ios_internal::").

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:37: #import
"ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.h"
I don't see this file in this CL nor in the repository nor in any of the
dependent CL. Missing file?

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:46: namespace {
Please move this after the "using" statements, and separated with a new line.

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:48: typedef NS_ENUM(BOOL,
PasswordInfoBarType) { SAVE, UPDATE };
Since this is internal only and in an anonymous namespace, I would prefer to use
a C++ enum here.

In addition, the enum value should be namespaced (as they are in the global
namespace) but naming them PASSWORD_INFO_BAR_{SAVE,UPDATE}. An alternative is to
use "enum class".

namespace {
// Types of password infobars to display.
enum class PasswordInfoBarType {
  SAVE,
  UPDATE,
};
}

Then you use it as "PasswordInfoBarType::SAVE" or "PasswordInfoBarType::UPDATE".

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:865: default:
If you use C++ enum or enum class, then you should be able to remove this
default case and the compiler will error if you forget to deal with same values.

[Jackie Quinn, 2016-07-05 13:52:35]

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:78: if
(ios_internal::experimental_flags::IsUpdatePasswordUIEnabled() &&
On 2016/07/05 13:29:55, sdefresne wrote:
> I think this should be "experimental_flags::IsUpdatePasswordUIEnabled()" (i.e.
> without "ios_internal::").

Yes, thanks. Done.

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/password_controller.mm (right):

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:37: #import
"ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.h"
On 2016/07/05 13:29:55, sdefresne wrote:
> I don't see this file in this CL nor in the repository nor in any of the
> dependent CL. Missing file?

It just landed earlier today: https://codereview.chromium.org/2108643002

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:46: namespace {
On 2016/07/05 13:29:55, sdefresne wrote:
> Please move this after the "using" statements, and separated with a new line.

Done.

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:48: typedef NS_ENUM(BOOL,
PasswordInfoBarType) { SAVE, UPDATE };
On 2016/07/05 13:29:55, sdefresne wrote:
> Since this is internal only and in an anonymous namespace, I would prefer to
use
> a C++ enum here.
> 
> In addition, the enum value should be namespaced (as they are in the global
> namespace) but naming them PASSWORD_INFO_BAR_{SAVE,UPDATE}. An alternative is
to
> use "enum class".
> 
> namespace {
> // Types of password infobars to display.
> enum class PasswordInfoBarType {
>   SAVE,
>   UPDATE,
> };
> }
> 
> Then you use it as "PasswordInfoBarType::SAVE" or
"PasswordInfoBarType::UPDATE".

Done.

https://codereview.chromium.org/2106353003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/password_controller.mm:865: default:
On 2016/07/05 13:29:55, sdefresne wrote:
> If you use C++ enum or enum class, then you should be able to remove this
> default case and the compiler will error if you forget to deal with same
values.

Done.

[vabr (Chromium), 2016-07-05 14:30:57]

LGTM with a comment and a nit. Thanks!
Vaclav

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:22: 
nit: Why this blank line? Normally the non-system #includes are in one block
except for #ifdef parts.

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:83:
[delegate_ showSavePasswordInfoBar:std::move(form_to_save)];
This seems like it would start showing the "save password" infobar for password
updates if the flag is off. It should be rather:
if (update_password) {
  if (/*flag set*/)
    /*show update infobar*/
} else {
  /*show save infobar*/
}

[Jackie Quinn, 2016-07-05 15:12:36]

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:22: 
On 2016/07/05 14:30:57, vabr (Chromium) wrote:
> nit: Why this blank line? Normally the non-system #includes are in one block
> except for #ifdef parts.

user error. Fixed.

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:83:
[delegate_ showSavePasswordInfoBar:std::move(form_to_save)];
On 2016/07/05 14:30:57, vabr (Chromium) wrote:
> This seems like it would start showing the "save password" infobar for
password
> updates if the flag is off. It should be rather:
> if (update_password) {
>   if (/*flag set*/)
>     /*show update infobar*/
> } else {
>   /*show save infobar*/
> }

I had it this way since it was always showing the save infobar anyways. Happy to
change it though :-)

[vabr (Chromium), 2016-07-05 15:32:17]

Thanks, LGTM with an inversion of my previous comment (sorry!).

Cheers,
Vaclav

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:83:
[delegate_ showSavePasswordInfoBar:std::move(form_to_save)];
On 2016/07/05 15:12:36, Jackie Quinn wrote:
> On 2016/07/05 14:30:57, vabr (Chromium) wrote:
> > This seems like it would start showing the "save password" infobar for
> password
> > updates if the flag is off. It should be rather:
> > if (update_password) {
> >   if (/*flag set*/)
> >     /*show update infobar*/
> > } else {
> >   /*show save infobar*/
> > }
> 
> I had it this way since it was always showing the save infobar anyways. Happy
to
> change it though :-)

Oh, you are right, Jackie, your patch set 4 keeps the original behaviour. In
that case I would suggest to keep it (= what you had in patch set 4). Sorry for
the back and forth.

Just out of curiosity, does iOS (without the change password support enabled)
prompt to save if you update a password? Looking at Android, in its section of
ChromePasswordManagerClient::PromptUserToSaveOrUpdatePassword, it does exactly
what you do here in patch set 4. Still, it updates silently. I have no idea why,
need to have a look later.

[sdefresne, 2016-07-05 16:51:18]

https://codereview.chromium.org/2106353003/diff/80001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/80001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:79: if
(update_password) {
I think this should be:

if (update_password && experimental_flags::IsUpdatePasswordUIEnabled()) {
  [delegate_ showUpdatePasswordInfoBar:std::move(form_to_save)];
} else {
  [delegate_ showPasswordInfoBar:std::move(form_to_save)];
}

[Jackie Quinn, 2016-07-06 07:12:34]

On 2016/07/05 15:32:17, vabr (Chromium) wrote:
> Thanks, LGTM with an inversion of my previous comment (sorry!).
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
> File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm
(right):
> 
>
https://codereview.chromium.org/2106353003/diff/60001/ios/chrome/browser/pass...
> ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:83:
> [delegate_ showSavePasswordInfoBar:std::move(form_to_save)];
> On 2016/07/05 15:12:36, Jackie Quinn wrote:
> > On 2016/07/05 14:30:57, vabr (Chromium) wrote:
> > > This seems like it would start showing the "save password" infobar for
> > password
> > > updates if the flag is off. It should be rather:
> > > if (update_password) {
> > >   if (/*flag set*/)
> > >     /*show update infobar*/
> > > } else {
> > >   /*show save infobar*/
> > > }
> > 
> > I had it this way since it was always showing the save infobar anyways.
Happy
> to
> > change it though :-)
> 
> Oh, you are right, Jackie, your patch set 4 keeps the original behaviour. In
> that case I would suggest to keep it (= what you had in patch set 4). Sorry
for
> the back and forth.
> 
> Just out of curiosity, does iOS (without the change password support enabled)
> prompt to save if you update a password? Looking at Android, in its section of
> ChromePasswordManagerClient::PromptUserToSaveOrUpdatePassword, it does exactly
> what you do here in patch set 4. Still, it updates silently. I have no idea
why,
> need to have a look later.

I have seen the save password dialog on iOS on Vadym's change password form, so
I believe it does. I'm not entirely sure though, because I may have been trying
the test page before saving any passwords for the site.

[Jackie Quinn, 2016-07-06 07:14:27]

https://codereview.chromium.org/2106353003/diff/80001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm (right):

https://codereview.chromium.org/2106353003/diff/80001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm:79: if
(update_password) {
On 2016/07/05 16:51:18, sdefresne wrote:
> I think this should be:
> 
> if (update_password && experimental_flags::IsUpdatePasswordUIEnabled()) {
>   [delegate_ showUpdatePasswordInfoBar:std::move(form_to_save)];
> } else {
>   [delegate_ showPasswordInfoBar:std::move(form_to_save)];
> }

Done.

[vabr (Chromium), 2016-07-06 07:33:41]

Hi Jackie!

It seems we should actually override
PasswordManagerClient::IsUpdatePasswordUIEnabled in
IOSChromePasswordManagerClient::IsUpdatePasswordUIEnabled. It should return
experimental_flags::IsUpdatePasswordUIEnabled() and we should replace all other
calls to experimental_flags::IsUpdatePasswordUIEnabled() with
IsUpdatePasswordUIEnabled.

Here is why:

I just had a look at PasswordManager::OnLoginSuccessful: it only calls
PromptUserToSaveOrUpdatePassword if the saved password is a new (=non-updated)
one, unless IsUpdatePasswordUIEnabled returns true (check done in
PasswordManager::ShouldPromptUserToSavePassword). So |update_password| is never
true without IsUpdatePasswordUIEnabled returning true. So far, iOS used the
default implementation of IsUpdatePasswordUIEnabled, which returns false, which
is why on iOS the save prompt is never shown for updated passwords so far.

Cheers,
Vaclav

[Jackie Quinn, 2016-07-06 07:48:24]

On 2016/07/06 07:33:41, vabr (Chromium) wrote:
> Hi Jackie!
> 
> It seems we should actually override
> PasswordManagerClient::IsUpdatePasswordUIEnabled in
> IOSChromePasswordManagerClient::IsUpdatePasswordUIEnabled. It should return
> experimental_flags::IsUpdatePasswordUIEnabled() and we should replace all
other
> calls to experimental_flags::IsUpdatePasswordUIEnabled() with
> IsUpdatePasswordUIEnabled.
> 
> Here is why:
> 
> I just had a look at PasswordManager::OnLoginSuccessful: it only calls
> PromptUserToSaveOrUpdatePassword if the saved password is a new (=non-updated)
> one, unless IsUpdatePasswordUIEnabled returns true (check done in
> PasswordManager::ShouldPromptUserToSavePassword). So |update_password| is
never
> true without IsUpdatePasswordUIEnabled returning true. So far, iOS used the
> default implementation of IsUpdatePasswordUIEnabled, which returns false,
which
> is why on iOS the save prompt is never shown for updated passwords so far.
> 
> Cheers,
> Vaclav

Okay, that makes sense. 

I was able to get the change password ui to show up without overriding it
though, so I'm not sure whether it's all hooked up correctly.

[vabr (Chromium), 2016-07-06 07:55:29]

LGTM!

On 2016/07/06 07:48:24, Jackie Quinn wrote:
> On 2016/07/06 07:33:41, vabr (Chromium) wrote:
> > Hi Jackie!
> > 
> > It seems we should actually override
> > PasswordManagerClient::IsUpdatePasswordUIEnabled in
> > IOSChromePasswordManagerClient::IsUpdatePasswordUIEnabled. It should return
> > experimental_flags::IsUpdatePasswordUIEnabled() and we should replace all
> other
> > calls to experimental_flags::IsUpdatePasswordUIEnabled() with
> > IsUpdatePasswordUIEnabled.
> > 
> > Here is why:
> > 
> > I just had a look at PasswordManager::OnLoginSuccessful: it only calls
> > PromptUserToSaveOrUpdatePassword if the saved password is a new
(=non-updated)
> > one, unless IsUpdatePasswordUIEnabled returns true (check done in
> > PasswordManager::ShouldPromptUserToSavePassword). So |update_password| is
> never
> > true without IsUpdatePasswordUIEnabled returning true. So far, iOS used the
> > default implementation of IsUpdatePasswordUIEnabled, which returns false,
> which
> > is why on iOS the save prompt is never shown for updated passwords so far.
> > 
> > Cheers,
> > Vaclav
> 
> Okay, that makes sense. 
> 
> I was able to get the change password ui to show up without overriding it
> though, so I'm not sure whether it's all hooked up correctly.

Yeah, I was also wondering how that worked, but there is a couple more
conditions in PasswordManager::ShouldPromptUserToSavePassword which could
actually lead to showing the prompt. In particular the
is_possible_change_password_form_without_username() call (in other words, I was
not completely precise in my previous e-mail). The latter might have caused the
prompt to appear on some change password form you tested, but would not let it
be shown on a simple sign-in form like on http://rsolomakhin.github.io/autofill.
Does that match your observations?

[Jackie Quinn, 2016-07-06 08:00:17]

On 2016/07/06 07:55:29, vabr (Chromium) wrote:
> LGTM!
> 
> On 2016/07/06 07:48:24, Jackie Quinn wrote:
> > On 2016/07/06 07:33:41, vabr (Chromium) wrote:
> > > Hi Jackie!
> > > 
> > > It seems we should actually override
> > > PasswordManagerClient::IsUpdatePasswordUIEnabled in
> > > IOSChromePasswordManagerClient::IsUpdatePasswordUIEnabled. It should
return
> > > experimental_flags::IsUpdatePasswordUIEnabled() and we should replace all
> > other
> > > calls to experimental_flags::IsUpdatePasswordUIEnabled() with
> > > IsUpdatePasswordUIEnabled.
> > > 
> > > Here is why:
> > > 
> > > I just had a look at PasswordManager::OnLoginSuccessful: it only calls
> > > PromptUserToSaveOrUpdatePassword if the saved password is a new
> (=non-updated)
> > > one, unless IsUpdatePasswordUIEnabled returns true (check done in
> > > PasswordManager::ShouldPromptUserToSavePassword). So |update_password| is
> > never
> > > true without IsUpdatePasswordUIEnabled returning true. So far, iOS used
the
> > > default implementation of IsUpdatePasswordUIEnabled, which returns false,
> > which
> > > is why on iOS the save prompt is never shown for updated passwords so far.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > Okay, that makes sense. 
> > 
> > I was able to get the change password ui to show up without overriding it
> > though, so I'm not sure whether it's all hooked up correctly.
> 
> Yeah, I was also wondering how that worked, but there is a couple more
> conditions in PasswordManager::ShouldPromptUserToSavePassword which could
> actually lead to showing the prompt. In particular the
> is_possible_change_password_form_without_username() call (in other words, I
was
> not completely precise in my previous e-mail). The latter might have caused
the
> prompt to appear on some change password form you tested, but would not let it
> be shown on a simple sign-in form like on
http://rsolomakhin.github.io/autofill.
> Does that match your observations?

Yeah, that tracks with what I was seeing :-)

[Jackie Quinn, 2016-07-06 12:29:02]

Description was changed from

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622244
==========

to

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257
==========

[sdefresne, 2016-07-06 14:43:12]

lgtm

[Jackie Quinn, 2016-07-06 14:44:04]

The CQ bit was checked by jyquinn@chromium.org

[commit-bot: I haz the power, 2016-07-06 14:44:12]

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-07-06 15:21:39]

Description was changed from

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257
==========

to

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257
==========

[commit-bot: I haz the power, 2016-07-06 15:21:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-06 15:21:42]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 15:23:39]

Description was changed from

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257
==========

to

==========
Enable Update Password UI on iOS behind a flag

Modifies PasswordController and IOSChromePasswordManagerClient
to display UI for updating passwords, if flag UpdatePasswordUIEnabled
is set to YES.

BUG=622257

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

[commit-bot: I haz the power, 2016-07-06 15:23:40]

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