Makes InfoBarService not close infobars on a reload from browser instant

Makes InfoBarService not close infobars on a reload from browser instant

BrowserInstantController calls Reload() if the google url
changes. This triggers closing the session crashed info bar
prematurely. Seems like we want this for all InfoBars, so I've
centralized the ignore.

BUG=401024
TEST=none
R=pkasting@chromium.org

Committed: https://crrev.com/4bdad24e8dc090068cb2cfcca50880186679eb7a
Cr-Commit-Position: refs/heads/master@{#295546}

[sky, 2014-09-17 21:50:48]

I would have liked a more clean way to identify the reload triggered from the
browserinstantcontroller. But I couldn't come up with anything.

[Peter Kasting, 2014-09-17 23:11:31]

Does the Reload() call result in NavigationEntryCommitted() getting called back
synchronously?  If not, there is a race condition here.

Honestly, the best fix here is probably that the "session crashed" infobar
should change to being not-an-infobar, e.g. some sort of bubble on the wrench
menu.  The UI leads probably have an opinion here.

[sky, 2014-09-17 23:19:39]

On 2014/09/17 23:11:31, Peter Kasting wrote:
> Does the Reload() call result in NavigationEntryCommitted() getting called
back
> synchronously?  If not, there is a race condition here.

You are definitely right there is a race condition here. I don't have a good way
to fix that. At worse the race results in keeping the info bar around longer.
I'm ok with that, given the alternative is a premature close.

> Honestly, the best fix here is probably that the "session crashed" infobar
> should change to being not-an-infobar, e.g. some sort of bubble on the wrench
> menu.  The UI leads probably have an opinion here.

There is a bubble implementation. It has the same bug and would have the same
race if I apply this fix to it.
I wanted to fix the infobar case before I worried about the bubble.

[Peter Kasting, 2014-09-17 23:39:30]

On 2014/09/17 23:19:39, sky wrote:
> There is a bubble implementation. It has the same bug and would have the same
> race if I apply this fix to it.

Why should the bubble be affected by reloads at all?  The point of a bubble is
to disconnect the UI here from the page content, since the message we're
conveying is unrelated to the underlying page.  If that's the case, page reloads
and navigations etc. shouldn't affect the bubble at all to begin with.

[sky, 2014-09-18 15:49:37]

I believe I've addressed the race condition. Take another look?

[Peter Kasting, 2014-09-18 17:48:05]

LGTM

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.cc:110: if (ignore_next_reload_ &&
IsReload(load_details))
Hmm, this seems subtle.  If we set ignore_next_reload_, then we'll check for
reloads on all subsequent commits until we get another pending navigation.  Is
doing this (instead of only looking at the next commit) important?  If so, leave
a comment here as to why.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.h (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:41: // reload than it is treated as if
nothing happened and no infobars are
Nit: than -> then

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:42: // attempted to be closed.
Nit: Remove "attempted to be"

Consider adding a motivation clause as to why this functionality is useful.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:165:
contents->GetController().Reload(false);
Nit: Maybe add a comment about whether the order of the Reload() and
set_ignore_next_reload() calls matters, since the order you have is opposite
what someone would probably assume is correct.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:167: InfoBarService*
info_bar_service =
Nit: Consider adding a comment here about why we want to ignore the next reload.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:168:
InfoBarService::FromWebContents(contents);
Can this actually be NULL?  If so add a comment about when.

[sky, 2014-09-18 19:22:27]

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.cc:110: if (ignore_next_reload_ &&
IsReload(load_details))
On 2014/09/18 17:48:04, Peter Kasting wrote:
> Hmm, this seems subtle.  If we set ignore_next_reload_, then we'll check for
> reloads on all subsequent commits until we get another pending navigation.  Is
> doing this (instead of only looking at the next commit) important?  If so,
leave
> a comment here as to why.

Done.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.h (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:41: // reload than it is treated as if
nothing happened and no infobars are
On 2014/09/18 17:48:04, Peter Kasting wrote:
> Nit: than -> then

Done.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:42: // attempted to be closed.
On 2014/09/18 17:48:04, Peter Kasting wrote:
> Nit: Remove "attempted to be"
> 
> Consider adding a motivation clause as to why this functionality is useful.

Done.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:165:
contents->GetController().Reload(false);
On 2014/09/18 17:48:04, Peter Kasting wrote:
> Nit: Maybe add a comment about whether the order of the Reload() and
> set_ignore_next_reload() calls matters, since the order you have is opposite
> what someone would probably assume is correct.

Done.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:167: InfoBarService*
info_bar_service =
On 2014/09/18 17:48:05, Peter Kasting wrote:
> Nit: Consider adding a comment here about why we want to ignore the next
reload.

Done.

https://codereview.chromium.org/553333006/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:168:
InfoBarService::FromWebContents(contents);
On 2014/09/18 17:48:05, Peter Kasting wrote:
> Can this actually be NULL?  If so add a comment about when.

You're right, it can't. Nuked.

[sky, 2014-09-18 19:22:31]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-09-18 19:23:20]

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

[commit-bot: I haz the power, 2014-09-18 20:22:27]

Committed patchset #3 (id:40001) as 641ea7bf8760de7dbee94161bf5c99baf83b6428

[commit-bot: I haz the power, 2014-09-18 20:23:18]

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