Fixing visibility transitions for Instant.

Fixing visibility transitions for Instant


BUG=101593
TEST=instant_browsertest


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

[Shishir, 2011-10-25 23:04:13]

Hi Scott, sreeram,

I was hoping to get this CL merged for the M1 dev release. Please take a look.

Thanks
Shishir

[Shishir, 2011-10-25 23:05:38]

I meant M16 :)

[sreeram, 2011-10-25 23:26:48]

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:260: bool
IsTabContentsPageVisibiltyVisible(TabContents* tab_contents) {
Typo in "Visibilty". In any case, I'd recommend giving a simpler name:
    bool IsTabContentsVisible(...)
or even just
    bool IsVisible(...)

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:931:
observer.WaitForObservation();
This seems like a lot of trouble. Why not this?

ASSERT_TRUE(test_server()->Start());
EnableInstant();
SetupInstantProvider("search.html");

ui_test_utils::NavigateToURL(browser(), test_server()->GetURL("/"));
TabContents* initial_contents = browser()->GetSelectedTabContents();
EXPECT_TRUE(IsVisible(initial_contents));

FindLocationBar();
location_bar_->FocusLocation(false);
SetupLocationBar();
...

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:951:
EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));
Just FYI: At first glance, I wouldn't have expected this to work, since we need
to wait for the renderer to send back a setSuggestions call before we'll show
instant; but your visibility check actually sends a message and waits for the
renderer to respond, so I think we end up processing the message loop.

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4315:
instant_->tab_contents()->tab_contents()->HideContents();
Why not just: GetSelectedTabContents()->HideContents()?

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4321:
instant_->tab_contents()->tab_contents()->ShowContents();
Same as before, why not just use GetSelectedTabContents()?

[Shishir, 2011-10-25 23:46:26]

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:260: bool
IsTabContentsPageVisibiltyVisible(TabContents* tab_contents) {
On 2011/10/25 23:26:48, sreeram wrote:
> Typo in "Visibilty". In any case, I'd recommend giving a simpler name:
>     bool IsTabContentsVisible(...)
> or even just
>     bool IsVisible(...)

Done.

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:931:
observer.WaitForObservation();
On 2011/10/25 23:26:48, sreeram wrote:
> This seems like a lot of trouble. Why not this?
> 
> ASSERT_TRUE(test_server()->Start());
> EnableInstant();
> SetupInstantProvider("search.html");
> 
> ui_test_utils::NavigateToURL(browser(), test_server()->GetURL("/"));
> TabContents* initial_contents = browser()->GetSelectedTabContents();
> EXPECT_TRUE(IsVisible(initial_contents));
> 
> FindLocationBar();
> location_bar_->FocusLocation(false);
> SetupLocationBar();
> ...

Done.

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:951:
EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));
On 2011/10/25 23:26:48, sreeram wrote:
> Just FYI: At first glance, I wouldn't have expected this to work, since we
need
> to wait for the renderer to send back a setSuggestions call before we'll show
> instant; but your visibility check actually sends a message and waits for the
> renderer to respond, so I think we end up processing the message loop.

Do you suggest something else here to increase robustness.

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4315:
instant_->tab_contents()->tab_contents()->HideContents();
On 2011/10/25 23:26:48, sreeram wrote:
> Why not just: GetSelectedTabContents()->HideContents()?

Done.

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4321:
instant_->tab_contents()->tab_contents()->ShowContents();
On 2011/10/25 23:26:48, sreeram wrote:
> Same as before, why not just use GetSelectedTabContents()?

Done.

[sky, 2011-10-26 00:01:06]

http://codereview.chromium.org/8373010/diff/2002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8373010/diff/2002/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4321: GetSelectedTabContents()->ShowContents();
What is the event ordering on a tab switch? I want to make sure we don't send
things out of order.

[Shishir, 2011-10-26 00:24:09]

On 2011/10/26 00:01:06, sky wrote:
> http://codereview.chromium.org/8373010/diff/2002/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
http://codereview.chromium.org/8373010/diff/2002/chrome/browser/ui/browser.cc...
> chrome/browser/ui/browser.cc:4321: GetSelectedTabContents()->ShowContents();
> What is the event ordering on a tab switch? I want to make sure we don't send
> things out of order.

From the code it does look like the tab contents is switched first and then the
HideInstant call is made. The function TabStripModel::NotifyIfActiveTabChanged
is what makes the call to Instant (through the TabDeactivated notification).

void TabStripModel::NotifyIfActiveTabChanged(
    TabContentsWrapper* old_contents,
    bool user_gesture) {
  TabContentsWrapper* new_contents = GetContentsAt(active_index());
  if (old_contents != new_contents) {
    if (old_contents) {
      FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                        TabDeactivated(old_contents));
    }
    FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                      ActiveTabChanged(old_contents, new_contents,
                                       active_index(), user_gesture));
    // Activating a discarded tab reloads it, so it is no longer discarded.
    contents_data_[active_index()]->discarded = false;
  }
}

[sky, 2011-10-26 03:45:17]

LGTM

[sreeram, 2011-10-26 04:24:17]

On 2011/10/26 00:24:09, Shishir wrote:
> On 2011/10/26 00:01:06, sky wrote:
> > chrome/browser/ui/browser.cc:4321: GetSelectedTabContents()->ShowContents();
> > What is the event ordering on a tab switch? I want to make sure we don't
send
> > things out of order.
> 
> From the code it does look like the tab contents is switched first and then
the
> HideInstant call is made. The function TabStripModel::NotifyIfActiveTabChanged
> is what makes the call to Instant (through the TabDeactivated notification).

Good catch, Scott!

Shishir, based on your explanation, it's a good thing that you are calling
GetSelectedTabContents(), instead of using instant_->tab_contents(), which would
certainly be wrong (since it would point to the tab that was being deactivated).

So it turns out that there are still two more problems to be fixed:

The first one is easy. Sometimes, GetSelectedTabContents() returns NULL. This
happens when you are looking at an instant preview, and you close the window (or
if you close the tab, and the tab was the last remaining tab in the window).
This should be easy to handle.

The second problem is more insidious. If you are looking at an instant preview,
and you minimize the whole window, GetSelectedTabContents() returns the
underlying tab as expected, but I believe you should not be setting the visible
state to true (as per http://www.w3.org/TR/2011/WD-page-visibility-20110602/).
How do you propose to handle this?

Ideally, you would write browser tests for the above scenarios. In addition,
don't forget to test the interaction of prerendering with instant (both manually
and through browser tests).

[sreeram, 2011-10-26 04:29:52]

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/8373010/diff/5001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:951:
EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));
On 2011/10/25 23:46:26, Shishir wrote:
> On 2011/10/25 23:26:48, sreeram wrote:
> > Just FYI: At first glance, I wouldn't have expected this to work, since we
> need
> > to wait for the renderer to send back a setSuggestions call before we'll
show
> > instant; but your visibility check actually sends a message and waits for
the
> > renderer to respond, so I think we end up processing the message loop.
> 
> Do you suggest something else here to increase robustness.

I would normally have said to put in a call to
WaitForMessageToBeProcessedByRenderer(), so that if ever the visibility check
becomes something like a simple function call on the browser thread, instead of
needing to go around to the renderer and back, the test won't break. But this
(visibility check implementation changing) is unlikely to happen, so don't worry
about it.

[sreeram, 2011-10-26 18:14:28]

Problem #3: With the preview showing, press <Alt-Enter>. This opens (and
commits) the preview in a new foreground tab. In HideInstant() however,
GetSelectedTabContents() still points to the old tab, which incorrectly marks it
visible.

[Shishir, 2011-10-27 15:24:48]

On 2011/10/26 18:14:28, sreeram wrote:
> Problem #3: With the preview showing, press <Alt-Enter>. This opens (and
> commits) the preview in a new foreground tab. In HideInstant() however,
> GetSelectedTabContents() still points to the old tab, which incorrectly marks
it
> visible.

As per discussions with Sreeram, of the 3 cases pointed out, the first was
critical as it would cause a crash. That is fixed. The second is something we
need to fix for non instant case also. Since this CL fixes the visibility state
for the instant page, I am comitting it and working on fixing the rest of the
cases.

[commit-bot: I haz the power, 2011-10-27 15:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/8373010/10001

[sreeram, 2011-10-27 16:22:12]

lgtm

I'll file bugs for the other cases so you can track them independently.

On 2011/10/27 15:24:48, Shishir wrote:
> On 2011/10/26 18:14:28, sreeram wrote:
> > Problem #3: With the preview showing, press <Alt-Enter>. This opens (and
> > commits) the preview in a new foreground tab. In HideInstant() however,
> > GetSelectedTabContents() still points to the old tab, which incorrectly
marks
> it
> > visible.
> 
> As per discussions with Sreeram, of the 3 cases pointed out, the first was
> critical as it would cause a crash. That is fixed. The second is something we
> need to fix for non instant case also. Since this CL fixes the visibility
state
> for the instant page, I am comitting it and working on fixing the rest of the
> cases.

[commit-bot: I haz the power, 2011-10-27 16:37:51]

Change committed as 107585