Add PrerenderUnload browser test

Add PrerenderUnload browser test to ensure that unload handlers are run on the navigated-away from page.

This test fails if the bug fix for 85350 is not in, and succeeds with it in.

BUG=85350, 82856
TEST=browser_tests --gtest_filter=*PrerenderUnload

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

[cbentzel, 2011-06-08 22:07:27]

Spraying a couple people to get this reviewed.

If the test holds this up I may separate out the fix into a separate CL so it
can get in quickly.

[sreeram, 2011-06-08 22:11:09]

LGTM

[dominich, 2011-06-08 22:19:19]

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

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1351: class TitleWatcher :
public NotificationObserver {
Can you put this up with the other classes at the top of the file? I understand
that it's only used below, but keeping the supporting classes together makes the
file more readable.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1378:
MessageLoopForUI::current()->Quit();
Please make sure you run this on the release trybots. We have multiple places
where message loops are being quit and it's the root of the flakiness for a
number of tests. Adding another path for this worries me.

You might also want to make sure you have the
set_quit_loop_quit_on_destruction() change from Shishir integrated.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1402: string16 expected_title
= ASCIIToUTF16("Unloaded");
nit: No need for this to be a local variable.

[mmenke, 2011-06-08 22:29:05]

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

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1351: class TitleWatcher :
public NotificationObserver {
nit:  Should stick this in an anonymous namespace, to prevent collisions.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:85:
manager_->ScheduleDeleteOldTabContents(tab, this);
nit:  Any reason for not just doing:

tab_->tab_contents()->set_delegate(NULL);
manager_->ScheduleDeleteOldTabContents(tab_.release(), this);

http://codereview.chromium.org/7046053/diff/2001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_loader_with_unload.html (right):

http://codereview.chromium.org/7046053/diff/2001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_loader_with_unload.html:19: document.title
= "Unloaded";
nit:  I believe the convention is to use single quotes in Javascript, for
consistency and to avoid confusion when mixing with HTML.

[cbentzel, 2011-06-09 00:49:49]

Thanks for the quick reviews. I just kicked of a try job to the release bots in
addition to the previous run on the debug bots.

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

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1351: class TitleWatcher :
public NotificationObserver {
On 2011/06/08 22:19:19, dominich wrote:
> Can you put this up with the other classes at the top of the file? I
understand
> that it's only used below, but keeping the supporting classes together makes
the
> file more readable.

Done.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1351: class TitleWatcher :
public NotificationObserver {
On 2011/06/08 22:29:05, Matt Menke wrote:
> nit:  Should stick this in an anonymous namespace, to prevent collisions.

Done.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1402: string16 expected_title
= ASCIIToUTF16("Unloaded");
On 2011/06/08 22:19:19, dominich wrote:
> nit: No need for this to be a local variable.

Done.

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7046053/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:85:
manager_->ScheduleDeleteOldTabContents(tab, this);
On 2011/06/08 22:29:05, Matt Menke wrote:
> nit:  Any reason for not just doing:
> 
> tab_->tab_contents()->set_delegate(NULL);
> manager_->ScheduleDeleteOldTabContents(tab_.release(), this);

Nope. Changed to save one vertical line.

http://codereview.chromium.org/7046053/diff/2001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_loader_with_unload.html (right):

http://codereview.chromium.org/7046053/diff/2001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_loader_with_unload.html:19: document.title
= "Unloaded";
On 2011/06/08 22:29:05, Matt Menke wrote:
> nit:  I believe the convention is to use single quotes in Javascript, for
> consistency and to avoid confusion when mixing with HTML.

Fixed. Also removed the vestigial functions from my copy-and-paste.

[cbentzel, 2011-06-09 01:46:32]

On 2011/06/09 00:49:49, cbentzel wrote:
> Thanks for the quick reviews. I just kicked of a try job to the release bots
in
> addition to the previous run on the debug bots.

Did not complete on the release bots. Trying to repro locally and think of a
fix.

[cbentzel, 2011-06-09 03:07:13]

So, the "Did not finish" was not due to a stall - it was due to a null-pointer
dereference that was exposed in the release build. Patchset 4 fixes this.

[Paweł Hajdan Jr., 2011-06-09 09:23:15]

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

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:246: class TitleWatcher :
public NotificationObserver {
Could you move this to chrome/test/ui_test_utils? I'd suggest changing the name
to TabContentsTitleWatcher or TabTitleWatcher.

Please remember to move the implementation to the .cc file.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:278: bool WaitForTitleChange()
{
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:291: bool
quit_loop_on_observation_;
nit: DISALLOW_COPY_AND_ASSIGN.

[dominich, 2011-06-09 14:26:56]

LGTM

nits only.

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

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:520: void
set_loader_path(const std::string& path) {
Did you consider adding another PrerenderTestURL variant that takes this as a
parameter instead for consistency?

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:1405: TabContents*
tab_contents = browser()->GetSelectedTabContents();
nit: no need for this to be a local variable.

[mmenke, 2011-06-09 14:29:49]

LGTM

[cbentzel, 2011-06-09 15:29:21]

I updated the description to reflect that this is just adding the browser test,
and not the bug fix [which I landed separately for expediency purposes since it
was the top crasher].

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

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:246: class TitleWatcher :
public NotificationObserver {
On 2011/06/09 09:23:15, Paweł Hajdan Jr. wrote:
> Could you move this to chrome/test/ui_test_utils? I'd suggest changing the
name
> to TabContentsTitleWatcher or TabTitleWatcher.
> 
> Please remember to move the implementation to the .cc file.

This seems like a pretty specific class, and doesn't warrant inclusion in
ui_test_utils. If you really want me to to move it there, I will.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:278: bool WaitForTitleChange()
{
On 2011/06/09 09:23:15, Paweł Hajdan Jr. wrote:
> nit: Add WARN_UNUSED_RESULT from base/compiler_specific.

Done.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:291: bool
quit_loop_on_observation_;
On 2011/06/09 09:23:15, Paweł Hajdan Jr. wrote:
> nit: DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:520: void
set_loader_path(const std::string& path) {
On 2011/06/09 14:26:56, dominich wrote:
> Did you consider adding another PrerenderTestURL variant that takes this as a
> parameter instead for consistency?

I did consider it, but it seemed like the combinatoric space was getting too
high at that point.

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:1405: TabContents*
tab_contents = browser()->GetSelectedTabContents();
On 2011/06/09 14:26:56, dominich wrote:
> nit: no need for this to be a local variable.

Done.

[Paweł Hajdan Jr., 2011-06-09 19:24:12]

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

http://codereview.chromium.org/7046053/diff/4/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:246: class TitleWatcher :
public NotificationObserver {
On 2011/06/09 15:29:21, cbentzel wrote:
> On 2011/06/09 09:23:15, Paweł Hajdan Jr. wrote:
> > Could you move this to chrome/test/ui_test_utils? I'd suggest changing the
> name
> > to TabContentsTitleWatcher or TabTitleWatcher.
> > 
> > Please remember to move the implementation to the .cc file.
> 
> This seems like a pretty specific class, and doesn't warrant inclusion in
> ui_test_utils. If you really want me to to move it there, I will.

Yes please, for now it's not used by more tests but I'd prefer to have it in
ui_test_utils instead of duplicated for multiple tests. Many tests wait for
title to be updated, just in different, frequently non-optimal way.

[cbentzel, 2011-06-09 20:01:04]

On Thu, Jun 9, 2011 at 3:24 PM, <phajdan.jr@chromium.org> wrote:
>
>
> Yes please, for now it's not used by more tests but I'd prefer to have
> it in ui_test_utils instead of duplicated for multiple tests. Many tests
> wait for title to be updated, just in different, frequently non-optimal
> way.
>
>
Done.

[Paweł Hajdan Jr., 2011-06-09 20:08:27]

LGTM with two comments. Thanks!

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.cc...
chrome/test/ui_test_utils.cc:1041: 
nit: Remove redundant empty line.

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.h#...
chrome/test/ui_test_utils.h:569: bool WaitForTitleChange() WARN_UNUSED_RESULT;
And now, please make it consistent with WindowedNotificationObserver (could you
put those two classes close to each other?). Just "Wait" instead of
"WaitForTitleChange".

[cbentzel, 2011-06-10 00:41:31]

Thanks for the reviews. I plan to commit pending the most recent trybot results.

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.cc...
chrome/test/ui_test_utils.cc:1041: 
On 2011/06/09 20:08:27, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line.

Done.

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/7046053/diff/2007/chrome/test/ui_test_utils.h#...
chrome/test/ui_test_utils.h:569: bool WaitForTitleChange() WARN_UNUSED_RESULT;
On 2011/06/09 20:08:27, Paweł Hajdan Jr. wrote:
> And now, please make it consistent with WindowedNotificationObserver (could
you
> put those two classes close to each other?). Just "Wait" instead of
> "WaitForTitleChange".

Done.

[mmenke, 2011-06-10 00:53:09]

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

http://codereview.chromium.org/7046053/diff/3010/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:30: #include
"content/browser/tab_contents/title_updated_details.h"
nit:  No longer needed.

[cbentzel, 2011-06-10 01:00:20]

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

http://codereview.chromium.org/7046053/diff/3010/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:30: #include
"content/browser/tab_contents/title_updated_details.h"
On 2011/06/10 00:53:09, Matt Menke wrote:
> nit:  No longer needed.

Done.

[commit-bot: I haz the power, 2011-06-10 11:04:19]

Change committed as 88654