Persist "Save password" infobars across redirects

Persist "Save password" infobars across redirects

As described in the bug, on some pages we observe a chain of redirections between the sign-in and the final landing form, during which the "save password" infobar is prematurely destroyed.

This CL overrides the ShouldExpire method for "save password" infobars to survive page transitions which are redirects.

BUG=26186
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243889

[vabr (Chromium), 2014-01-08 17:28:20]

Hi Ilya,

(If you prefer to not start this before you are away, just let me know and I'll
ask Garrett.)

Please take a look. The added test fails without the fix (and works with it).
The real issue with todoist.com (described on the bug) seems solved by this as
well.

On todoist.com, if the user signs in, and then out, the infobar is still there
(on the un-signed todoist.com page) and can be used to store the password after
logging out. I understand that this is not exactly what we want, but it looks to
me like a lesser evil that the current state. Also, any usual navigation outside
of todoist.com should get rid of the infobar, because any non-redirection will
do that.

I was also considering whether I should modify ShouldExpire for some parents of
SavePasswordInfoBarDelegate rather than for SavePasswordInfoBarDelegate instead,
but to me this behaviour looks rather "save password"-specific, and also there
is no sign that the other infobars are not working as intended.

Thanks,
Vaclav

[Ilya Sherman, 2014-01-08 23:34:52]

Thanks!  LGTM % nits:

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:75: if
(auto_accept_infobar_)
nit: This needs curlies.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:85: }
nit: Pretty sure you need to also have a default case in order to appease clang.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:105: void
DisableAutoAcceptingInfobars() { auto_accept_infobar_ = false; }
nit: I'd name this "disable_foo", where "foo" is the variable name (see also my
comment below).

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:116: bool
auto_accept_infobar_;
nit: Please document this member var.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:116: bool
auto_accept_infobar_;
Optional nit: Perhaps a name like "should_automatically_accept_infobar_" would
be a bit clearer?

[vabr (Chromium), 2014-01-09 11:39:37]

Thanks, Ilya!

I addressed your comments, and changed the following three things:
1) break -> return, as documented in my comment below
2) fixing code to not trigger C4800 compiler warning (see comment below)
3) fixing a wrongly referenced PageTransition enum value

These other changes seem straightforward enough, so I'm sending this to CQ based
on your LGTM from yesterday. If you object, please feel free to revert, and I'll
address any further comments you might have.

Thanks,
Vaclav

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:75: if
(auto_accept_infobar_)
On 2014/01/08 23:34:53, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: This needs curlies.

Done.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:85: }
On 2014/01/08 23:34:53, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: Pretty sure you need to also have a default case in order to appease
clang.

Clang seemed to be fine with this, but I still added default: NOTREACHED for
documentation sake.

Also, I changed breaks to returns, because there is meant to be no common
processing after the switch statement.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:105: void
DisableAutoAcceptingInfobars() { auto_accept_infobar_ = false; }
On 2014/01/08 23:34:53, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: I'd name this "disable_foo", where "foo" is the variable name (see also
my
> comment below).

Done.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:116: bool
auto_accept_infobar_;
On 2014/01/08 23:34:53, Ilya Sherman (Away Jan11-Feb2) wrote:
> Optional nit: Perhaps a name like "should_automatically_accept_infobar_" would
> be a bit clearer?

Done.

https://codereview.chromium.org/127973004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager_browsertest.cc:116: bool
auto_accept_infobar_;
On 2014/01/08 23:34:53, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: Please document this member var.

Done.

https://codereview.chromium.org/127973004/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_delegate_impl.cc (right):

https://codereview.chromium.org/127973004/diff/180001/chrome/browser/password...
chrome/browser/password_manager/password_manager_delegate_impl.cc:165: bool
is_not_redirect = !(details.entry->GetTransitionType() &
This change is to get rid of the C4800 warning of the windows compiler -- it
dislikes when an int value is assigned to a bool.

[commit-bot: I haz the power, 2014-01-09 11:42:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/127973004/180001

[commit-bot: I haz the power, 2014-01-09 14:11:07]

Change committed as 243889