Change TaskManager test to be in line with new TabContents lifetime uncertainty when Prerendering.

Change TaskManager test to be in line with new TabContents lifetime uncertainty when Prerendering.

BUG=none
TEST=PrerenderBrowserTest.TaskManager


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

[cbentzel, 2011-05-06 16:07:25]

LGTM

[mmenke, 2011-05-06 16:08:08]

LGTM

http://codereview.chromium.org/6948002/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6948002/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:718:
EXPECT_GE(model()->ResourceCount(), 3);
Shouldn't we always have 4 here?  Browser, old (still in use), gpu, and the
prerender?

[dominich, 2011-05-06 16:29:46]

On 2011/05/06 16:08:08, Matt Menke wrote:
> LGTM
> 
>
http://codereview.chromium.org/6948002/diff/1/chrome/browser/prerender/preren...
> File chrome/browser/prerender/prerender_browsertest.cc (right):
> 
>
http://codereview.chromium.org/6948002/diff/1/chrome/browser/prerender/preren...
> chrome/browser/prerender/prerender_browsertest.cc:718:
> EXPECT_GE(model()->ResourceCount(), 3);
> Shouldn't we always have 4 here?  Browser, old (still in use), gpu, and the
> prerender?

Most of the time it's actually 3: Browser, blank, prerender. The flake I've seen
is that there's a 4th tab which is a Tab with a similar title to the prerender.
Thinking about it more, I'd expect the post-navigate TaskManager to have 3 or 4,
but the post-Prerender/pre-Navigate should always have 3, unless I'm missing
something.

Actually, if your build of Chromium has extensions installed, this test will
fail, so maybe it's worth restricting this test to checking that we have a
Prerender resource, and then that we don't (and that the Prerender resource has
become a normal Tab.

[mmenke, 2011-05-06 16:37:35]

On 2011/05/06 16:29:46, dominic wrote:
> Most of the time it's actually 3: Browser, blank, prerender. The flake I've
seen
> is that there's a 4th tab which is a Tab with a similar title to the
prerender.
> Thinking about it more, I'd expect the post-navigate TaskManager to have 3 or
4,
> but the post-Prerender/pre-Navigate should always have 3, unless I'm missing
> something.
> 
> Actually, if your build of Chromium has extensions installed, this test will
> fail, so maybe it's worth restricting this test to checking that we have a
> Prerender resource, and then that we don't (and that the Prerender resource
has
> become a normal Tab.

Oh...I was assuming the GPU process made it 3.  Looks like it's not listed, so
yea...Seems like we should always have 3, but also seems like we should have 2
or three for the next one, rather than 3 or 4.  Wonder what's going on there.

[dominich, 2011-05-06 16:51:07]

On 2011/05/06 16:37:35, Matt Menke wrote:
> On 2011/05/06 16:29:46, dominic wrote:
> > Most of the time it's actually 3: Browser, blank, prerender. The flake I've
> seen
> > is that there's a 4th tab which is a Tab with a similar title to the
> prerender.
> > Thinking about it more, I'd expect the post-navigate TaskManager to have 3
or
> 4,
> > but the post-Prerender/pre-Navigate should always have 3, unless I'm missing
> > something.
> > 
> > Actually, if your build of Chromium has extensions installed, this test will
> > fail, so maybe it's worth restricting this test to checking that we have a
> > Prerender resource, and then that we don't (and that the Prerender resource
> has
> > become a normal Tab.
> 
> Oh...I was assuming the GPU process made it 3.  Looks like it's not listed, so
> yea...Seems like we should always have 3, but also seems like we should have 2
> or three for the next one, rather than 3 or 4.  Wonder what's going on there.

This is what I'm seeing, and it's not quite what I expected:

[INFO:prerender_browsertest.cc(723)] After prerender:
[INFO:prerender_browsertest.cc(725)]   0: Browser
[INFO:prerender_browsertest.cc(725)]   1: Tab: Preloader
[INFO:prerender_browsertest.cc(725)]   2: Prerender: Prerender Page
[INFO:prerender_browsertest.cc(741)] After navigate:
[INFO:prerender_browsertest.cc(743)]   0: Browser
[INFO:prerender_browsertest.cc(743)]   1: Tab: Preloader
[INFO:prerender_browsertest.cc(743)]   2: Tab: Prerender Page

I expected after navigate to see 1 replaced by 2, but it seems we're creating a
new tab for the Navigate.

[mmenke, 2011-05-06 17:04:44]

On 2011/05/06 16:51:07, dominic wrote:
> This is what I'm seeing, and it's not quite what I expected:
> 
> [INFO:prerender_browsertest.cc(723)] After prerender:
> [INFO:prerender_browsertest.cc(725)]   0: Browser
> [INFO:prerender_browsertest.cc(725)]   1: Tab: Preloader
> [INFO:prerender_browsertest.cc(725)]   2: Prerender: Prerender Page
> [INFO:prerender_browsertest.cc(741)] After navigate:
> [INFO:prerender_browsertest.cc(743)]   0: Browser
> [INFO:prerender_browsertest.cc(743)]   1: Tab: Preloader
> [INFO:prerender_browsertest.cc(743)]   2: Tab: Prerender Page
> 
> I expected after navigate to see 1 replaced by 2, but it seems we're creating
a
> new tab for the Navigate.

That's actually expected, since TabContents are listed rather than tabs, and we
don't delete the original TabContents immediately, so it may not have been
destroyed by the time you walk through the task manager list.

I'm just wondering why it's 3 or 4, rather than 2 or 3 (And I think for the
first check, just checking for exactly 3 should be fine).

[dominich, 2011-05-06 17:21:38]

On 2011/05/06 17:04:44, Matt Menke wrote:
> On 2011/05/06 16:51:07, dominic wrote:
> > This is what I'm seeing, and it's not quite what I expected:
> > 
> > [INFO:prerender_browsertest.cc(723)] After prerender:
> > [INFO:prerender_browsertest.cc(725)]   0: Browser
> > [INFO:prerender_browsertest.cc(725)]   1: Tab: Preloader
> > [INFO:prerender_browsertest.cc(725)]   2: Prerender: Prerender Page
> > [INFO:prerender_browsertest.cc(741)] After navigate:
> > [INFO:prerender_browsertest.cc(743)]   0: Browser
> > [INFO:prerender_browsertest.cc(743)]   1: Tab: Preloader
> > [INFO:prerender_browsertest.cc(743)]   2: Tab: Prerender Page
> > 
> > I expected after navigate to see 1 replaced by 2, but it seems we're
creating
> a
> > new tab for the Navigate.
> 
> That's actually expected, since TabContents are listed rather than tabs, and
we
> don't delete the original TabContents immediately, so it may not have been
> destroyed by the time you walk through the task manager list.
> 
> I'm just wondering why it's 3 or 4, rather than 2 or 3 (And I think for the
> first check, just checking for exactly 3 should be fine).

http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/25829/step...
(from cbentzel's try run) shows the /first/ check returned 4 rather than 3.

In any case, I've updated the test to ignore the specific counts and instead
check what we're interested in: That we get a prerender tab and that it becomes
a real tab with the same title after navigation.

[mmenke, 2011-05-06 17:29:54]

LGTM.  Until/unless we understand just what's going on with the 4 tabs, this
seems the safest solution.

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:736: // There should be no
Tabs with the Prerender prefix.
nit: Don't capitalize "tab", as it's not a class, or use TabContents.

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:754: if
(!found_tab_with_prerender_page_title &&
Is this first check really needed?  Doesn't seem to make any difference.

Could ASSERT on it before setting it to true, if you want.  Or not.

[dominich, 2011-05-06 19:19:14]

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:736: // There should be no
Tabs with the Prerender prefix.
On 2011/05/06 17:29:54, Matt Menke wrote:
> nit: Don't capitalize "tab", as it's not a class, or use TabContents.

Done.

http://codereview.chromium.org/6948002/diff/9001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:754: if
(!found_tab_with_prerender_page_title &&
On 2011/05/06 17:29:54, Matt Menke wrote:
> Is this first check really needed?  Doesn't seem to make any difference.
> 
> Could ASSERT on it before setting it to true, if you want.  Or not.

Done.

[mmenke, 2011-05-06 20:05:09]

Still LGTM.

[cbentzel, 2011-05-06 21:21:03]

LGTM

http://codereview.chromium.org/6948002/diff/12002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6948002/diff/12002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:722: VLOG(1) << "After
prerender:";
Do you still need these statements?

http://codereview.chromium.org/6948002/diff/12002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:749:
ASSERT_TRUE(StartsWith(tab_title, tab_prefix, true));
Nit: EXPECT_TRUE is probably appropriate here. ASSERT is usually used when the
test will crash, EXPECT otherwise.

[commit-bot: I haz the power, 2011-05-06 22:52:48]

Change committed as 84517