Fix spurious visibility events when committing Instant.

Fix spurious visibility events when committing Instant.

This CL fixes things so that we consistently handle the Instant preview when
committing, on all platforms. On a commit, whether on the current tab or a new
tab, we'll never hide the preview.

R=sky@chromium.org
BUG=none
TEST=none; no functionality change.


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

[sreeram, 2012-10-23 22:57:56]

Please review.

[sreeram, 2012-10-23 23:44:11]

+dhollowa and +rohitrao for views and cocoa respectively.

This CL was born out of the desire to fix the issue mentioned in
https://codereview.chromium.org/11026050#msg8

[sky, 2012-10-24 03:51:16]

http://codereview.chromium.org/11233076/diff/1/chrome/browser/instant/instant...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:736:
MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
Add comment as to why delayed delete.

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1383: void
BrowserView::TabInsertedAt(TabContents* contents, int index, bool active) {
Why can't we wait until ActiveTabChanged?

[dhollowa, 2012-10-24 16:40:14]

*.mm LGTM.

[rohitrao (ping after 24h), 2012-10-24 16:42:08]

cocoa LGTM

[sreeram, 2012-10-24 16:46:44]

http://codereview.chromium.org/11233076/diff/1/chrome/browser/instant/instant...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:736:
MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
On 2012/10/24 03:51:16, sky wrote:
> Add comment as to why delayed delete.

Done.

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1383: void
BrowserView::TabInsertedAt(TabContents* contents, int index, bool active) {
On 2012/10/24 03:51:16, sky wrote:
> Why can't we wait until ActiveTabChanged?

I suppose we could. However, I plan to add support for the NEW_BACKGROUND_TAB
disposition as well very soon, at which time, we'll definitely need this method.

[sky, 2012-10-24 20:49:26]

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1383: void
BrowserView::TabInsertedAt(TabContents* contents, int index, bool active) {
On 2012/10/24 16:46:45, sreeram wrote:
> On 2012/10/24 03:51:16, sky wrote:
> > Why can't we wait until ActiveTabChanged?
> 
> I suppose we could. However, I plan to add support for the NEW_BACKGROUND_TAB
> disposition as well very soon, at which time, we'll definitely need this
method.

I don't like adding another method that overlaps with the existing one. Use the
existing one until we really need this.

[sreeram, 2012-10-25 00:44:33]

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/11233076/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1383: void
BrowserView::TabInsertedAt(TabContents* contents, int index, bool active) {
On 2012/10/24 20:49:26, sky wrote:
> On 2012/10/24 16:46:45, sreeram wrote:
> > On 2012/10/24 03:51:16, sky wrote:
> > > Why can't we wait until ActiveTabChanged?
> > 
> > I suppose we could. However, I plan to add support for the
NEW_BACKGROUND_TAB
> > disposition as well very soon, at which time, we'll definitely need this
> method.
> 
> I don't like adding another method that overlaps with the existing one. Use
the
> existing one until we really need this.

Done. In fact, I realized that pretty much everything can be consolidated into
ActiveTabChanged. So I've removed the insert/replace tab events from both views
& cocoa (gtk always just used active tab event). I've also updated the test to
be check the visibility transitions more thoroughly.

[sky, 2012-10-25 15:02:18]

LGTM

[commit-bot: I haz the power, 2012-10-25 18:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/11233076/16004

[commit-bot: I haz the power, 2012-10-26 16:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/11233076/21001

[commit-bot: I haz the power, 2012-10-26 17:12:00]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for dinner.
The relevant dude was pinged already.

[commit-bot: I haz the power, 2012-10-26 17:54:59]

Change committed as 164350