Don't rely on user gestures for deciding when to dismiss infobars.

Don't rely on user gestures for deciding when to dismiss infobars.

Previously, we looked at the user gesture state when deciding whether to
dismiss infobars.  Now that WebKit's user gesture state doesn't lie, we can
tell that this behavior is incorrect.  Page-specific infobars, like translate,
should close whenever the main frame navigates to a new page, regardless of
whether that navigation was conducted from a user gesture.

BUG=86417
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89864

[darin (slow to review), 2011-06-20 16:15:23]

Not sure if you are doing this or not, but "client redirects" (meta refresh,
location assignment before the page is loaded, location.replace, etc.) probably
shouldn't cause the infobars to be dismissed.

I didn't look at the patch.  Just commenting on the CL description.  You
probably want to check to see if a new back/forward entry was created.  Not just
look for navigation.

[abarth-chromium, 2011-06-20 16:18:40]

On 2011/06/20 16:15:23, darin wrote:
> Not sure if you are doing this or not, but "client redirects" (meta refresh,
> location assignment before the page is loaded, location.replace, etc.)
probably
> shouldn't cause the infobars to be dismissed.

That probably depends on the type of infobar, right?  For example, the mixed
content and translate infobars are specific to the page and should be removed on
any navigation, including client redirects.  Maybe other infobars, like the
password manager, should be preserved across client redirects?

> I didn't look at the patch.  Just commenting on the CL description.  You
> probably want to check to see if a new back/forward entry was created.  Not
just
> look for navigation.

In either case, the user gesture state isn't relevant to the decision.

[abarth-chromium, 2011-06-20 16:55:11]

Looks like all the subclasses of ConfirmInfoBarDelegate (including the password
manager) want to persist through client redirects, so I added that branch at
that level of the class hierarchy.

[Peter Kasting, 2011-06-20 18:04:06]

On 2011/06/20 16:55:11, abarth wrote:
> Looks like all the subclasses of ConfirmInfoBarDelegate (including the
password
> manager) want to persist through client redirects, so I added that branch at
> that level of the class hierarchy.

I would think that all InfoBarDelegates would want that, not just the confirm
ones.  Seems like we should put that in InfoBarDelegate::ShouldExpire().

It seems like the infobar ShouldExpire() stuff is a little messy.  I think we
can collapse InfoBarDelegate::ShouldExpireInternal() into
InfoBarDelegate::ShouldExpire() and turn all the subclass ShouldExpire()
implementations into something like:

return should_expire_ && InfoBarDelegate::ShouldExpire();

I'm also not clear on the distinction between is_navigation_to_different_page()
and the unique_id comparison that ShouldExpireInternal() is currently doing. 
Are these redundant?

[abarth-chromium, 2011-06-20 18:11:23]

> I would think that all InfoBarDelegates would want that, not just the confirm
> ones.  Seems like we should put that in InfoBarDelegate::ShouldExpire().

Why?  Most infobars just pertain to the current page.  If you navigate to a new
page, even with a client redirect, then you don't want to see the infobar any
more.  It's only infobars that pertain to something in the past (like the
password manager infobar) that want to stick around between different pages.

> It seems like the infobar ShouldExpire() stuff is a little messy.

Indeed.

> I think we
> can collapse InfoBarDelegate::ShouldExpireInternal() into
> InfoBarDelegate::ShouldExpire() and turn all the subclass ShouldExpire()
> implementations into something like:
> 
> return should_expire_ && InfoBarDelegate::ShouldExpire();

I'm not sure I'm familiar enough with this code to make that change confidently.
 
> I'm also not clear on the distinction between
is_navigation_to_different_page()
> and the unique_id comparison that ShouldExpireInternal() is currently doing. 
> Are these redundant?

I have no idea.  I tried to make the minimal change to this system consistent
with fixing the bug.  If that logic is redundant, it was redundant previously as
well.

[abarth-chromium, 2011-06-20 18:19:03]

The current code is really very silly.  Any non-user initiated navigation (such
as a setTimeout) that goes to a new page will persist the infobar.  Why would
something like the translate infobar be at all relevant to the new page?

[Peter Kasting, 2011-06-20 18:22:46]

On 2011/06/20 18:19:03, abarth wrote:
> Any non-user initiated navigation (such
> as a setTimeout) that goes to a new page will persist the infobar.  Why would
> something like the translate infobar be at all relevant to the new page?

The translate infobar wouldn't; the save password infobar probably would.  So it
varies by infobar.

[abarth-chromium, 2011-06-20 18:25:40]

> The translate infobar wouldn't; the save password infobar probably would.  So
it
> varies by infobar.

Correct.  I think we're now just talking about what the default should be.  I
can make the more persistent behavior the default and override the behavior for
the infobars that want the less persistent behavior.  The approach in the
current patch seems relatively clean, though, since ConfirmInfoBarDelegate looks
like a nice choke-point for the more persistent behavior.

[abarth-chromium, 2011-06-20 18:41:02]

Brett,

pkasting says you should review this change because it's mostly related to
navigation controller code.  (For context, after this CL, I've promised Peter
that I'm going to do a follow-up CL that cleans up all the overrides of this
function.)

[darin (slow to review), 2011-06-20 19:25:53]

On Mon, Jun 20, 2011 at 11:25 AM, <abarth@chromium.org> wrote:

> The translate infobar wouldn't; the save password infobar probably would.
>>  So
>>
> it
>
>> varies by infobar.
>>
>
> Correct.  I think we're now just talking about what the default should be.
>  I
> can make the more persistent behavior the default and override the behavior
> for
> the infobars that want the less persistent behavior.  The approach in the
> current patch seems relatively clean, though, since ConfirmInfoBarDelegate
> looks
> like a nice choke-point for the more persistent behavior.
>
>
This makes a lot of sense to me.  I don't have a strong preference for what
the
default mode should be, but giving developers control over the degree of
persistence
sounds great.

-Darin



>
>
http://codereview.chromium.**org/7205026/<http://codereview.chromium.org/7205...
>

[brettw, 2011-06-21 16:32:26]

This does look a lot nicer.

In general, we do need to be careful about the password saving infobar in
particular. Lots of sites to different types of redirects after you log in, and
you want the password infobar to stick around. Sometimes, I've noticed, it
doesn't.

We should at least make sure that this patch doesn't cause a regression Gmail
and corp login (which seems to do more redirects), and maybe a few other big
sites like Amazon and Facebook.

[abarth-chromium, 2011-06-21 16:42:01]

Tested corp Gmail (both with an without OTP) and it worked fine.  Amazon also
worked fine.  I tried testing Facebook, but I forget my password.  :(

[brettw, 2011-06-21 17:14:29]

The navigation controller stuff LGTM. I guess just be on the lookout for
regressions.

http://codereview.chromium.org/7205026/diff/6012/content/browser/tab_contents...
File content/browser/tab_contents/navigation_details.h (right):

http://codereview.chromium.org/7205026/diff/6012/content/browser/tab_contents...
content/browser/tab_contents/navigation_details.h:60: // for showing or hiding
something, and this returns true only for main frame
This still mentions "user initiated" which in the context of the first sentence
doesn't make a lot of sense. Can you mention something about user-initiated in
the first sentence?

[abarth-chromium, 2011-06-21 17:35:54]

Thanks Brett.

[commit-bot: I haz the power, 2011-06-21 18:35:12]

Change committed as 89864