Added some cancel/close tab tests to SavePackage.

Added some cancel/close tab tests to SavePackage.  

R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-08-21 19:38:30]

Ben: PTAL?  This is in response to some canary crashes I'm getting on the DFM
nuke; I wanted to put some tests in to cover what I think it root cause before
re-landing.

[benjhayden, 2012-08-21 20:00:24]

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:112:
manager_->RemoveObserver(this);
Why do this here?

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:298: GURL url =
NavigateToMockURL("a");
I'm just curious. Why "GURL url = ..." instead of "GURL url(...)", and "DM*
dm(...)" instead of "DM* dm = ..."? Seems like there's a part of C++ that I
don't know.

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:344:
EXPECT_FALSE(file_util::PathExists(full_file_name));
What happens if you WaitForSavePackageToFinish()? Is that what it should do?

[Randy Smith (Not in Mondays), 2012-08-21 22:00:37]

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:112:
manager_->RemoveObserver(this);
On 2012/08/21 20:00:24, benjhayden_chromium wrote:
> Why do this here?

We have no interest in hearing anything from the DownloadManager anymore; after
we get our first item, our work is done.  OTOH, there are fewer lines of code
without it, and this is test code, so we don't care about performance or
gratuitous calls, really.  Ok, removed.

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:298: GURL url =
NavigateToMockURL("a");
On 2012/08/21 20:00:24, benjhayden_chromium wrote:
> I'm just curious. Why "GURL url = ..." instead of "GURL url(...)", and "DM*
> dm(...)" instead of "DM* dm = ..."? Seems like there's a part of C++ that I
> don't know.

I don't think there's any difference at the C++ level (you'd think it would use
the assignment operator rather than constructing it directly, but I think it
constructs it directly).  It's this way because I copied from another test in
the same file, and I'm inclined to leave it that way unless you want me to
change the entire file (following the "consistency within files" guideline). 
But I'm happy to change the whole file if you'd like.

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:344:
EXPECT_FALSE(file_util::PathExists(full_file_name));
On 2012/08/21 20:00:24, benjhayden_chromium wrote:
> What happens if you WaitForSavePackageToFinish()? Is that what it should do?

Well, I didn't try it, but a tab close should result in a SavePackage::Stop(),
which shouldn't ever send the notification that we've successfully finished.  So
I assume it would hang forever.  If you think I should create a new notification
and listen for it, you may well be right :-J--is that where you were going?

[benjhayden, 2012-08-22 13:23:13]

LGTM

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:344:
EXPECT_FALSE(file_util::PathExists(full_file_name));
On 2012/08/21 22:00:37, rdsmith wrote:
> On 2012/08/21 20:00:24, benjhayden_chromium wrote:
> > What happens if you WaitForSavePackageToFinish()? Is that what it should do?
> 
> Well, I didn't try it, but a tab close should result in a SavePackage::Stop(),
> which shouldn't ever send the notification that we've successfully finished. 
So
> I assume it would hang forever.  If you think I should create a new
notification
> and listen for it, you may well be right :-J--is that where you were going?

If you think it would help your DFM CL. If not, then don't worry about it.

[Randy Smith (Not in Mondays), 2012-08-22 14:50:42]

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10831412/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:344:
EXPECT_FALSE(file_util::PathExists(full_file_name));
On 2012/08/22 13:23:13, benjhayden_chromium wrote:
> On 2012/08/21 22:00:37, rdsmith wrote:
> > On 2012/08/21 20:00:24, benjhayden_chromium wrote:
> > > What happens if you WaitForSavePackageToFinish()? Is that what it should
do?
> > 
> > Well, I didn't try it, but a tab close should result in a
SavePackage::Stop(),
> > which shouldn't ever send the notification that we've successfully finished.

> So
> > I assume it would hang forever.  If you think I should create a new
> notification
> > and listen for it, you may well be right :-J--is that where you were going?
> 
> If you think it would help your DFM CL. If not, then don't worry about it.

Nope; from the DFM perspective, the question is "Do these tests crash?" :-} :-|.

Thanks!

[commit-bot: I haz the power, 2012-08-22 14:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10831412/5

[commit-bot: I haz the power, 2012-08-22 16:35:47]

Try job failure for 10831412-5 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 18:29:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10831412/5

[commit-bot: I haz the power, 2012-08-22 20:16:50]

Try job failure for 10831412-5 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 21:13:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10831412/5

[commit-bot: I haz the power, 2012-08-22 23:55:46]

Change committed as 152878