Add Cancel test for SavePackage.

Add Cancel test for SavePackage.

BUG=92289
TEST=New browser test for Cancel.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97065

[achuithb, 2011-08-11 02:11:13]

Test for Cancel case. Basically catches this bug:
http://code.google.com/p/chromium/issues/detail?id=91946

You can see the trybot run on patchset 7 - trybots fail on
SavePageBrowserTest.Cancel and SavePageBrowserTest.ExplicitCancel due to bug
91946. Here is a sample log:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
Search for 'ExplicitCancel'

Thanks!

[Paweł Hajdan Jr., 2011-08-11 17:26:39]

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:53:
ui_test_utils::NavigateToURL(browser(), *url);
GetPath doesn't really correspond to NavigateToURL for me... do you have better
ideas for name of this method?

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:178:
SavePackage::SAVE_AS_ONLY_HTML));
nit: Isn't this line misaligned now?

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:223:
SavePackage::SAVE_AS_COMPLETE_HTML));
nit: Isn't this line misaligned now?

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:262:
SavePackage::SAVE_AS_COMPLETE_HTML));
nit: Isn't this line misaligned now?

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:287:
SavePackage::SAVE_AS_ONLY_HTML));
nit: Isn't this line misaligned now?

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:313:
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, Cancel) {
nit: Add comment what this is testing.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:330: } // namespace
nit: Two spaces between } and //

[achuithb, 2011-08-11 23:21:59]

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:53:
ui_test_utils::NavigateToURL(browser(), *url);
The problem is that the function does 2 separate things - create URL + navigate
to that url, and initialize the full_file_name and dir variables. The name
GetPaths recognizes the second item but not the navigation part. 

This is really a setup/initialization function before we call SavePage, so maybe

Init or SetupSavePage?

The other option I can think of is to have two separate functions Navigate and
GetPaths.

Do any of these suggestions appeal to you, or do you have any other suggestion?
I'm leaning towards splitting this function into Navigate + GetPaths.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:178:
SavePackage::SAVE_AS_ONLY_HTML));
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Isn't this line misaligned now?

Done.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:223:
SavePackage::SAVE_AS_COMPLETE_HTML));
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Isn't this line misaligned now?

Done.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:262:
SavePackage::SAVE_AS_COMPLETE_HTML));
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Isn't this line misaligned now?

Done.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:287:
SavePackage::SAVE_AS_ONLY_HTML));
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Isn't this line misaligned now?

Done.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:313:
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, Cancel) {
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Add comment what this is testing.

Done.

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:330: } // namespace
On 2011/08/11 17:26:40, Paweł Hajdan Jr. wrote:
> nit: Two spaces between } and //

Done.

[Paweł Hajdan Jr., 2011-08-12 16:48:56]

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/7606024/diff/10001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:53:
ui_test_utils::NavigateToURL(browser(), *url);
On 2011/08/11 23:21:59, achuith.bhandarkar wrote:
> Do any of these suggestions appeal to you, or do you have any other
suggestion?
> I'm leaning towards splitting this function into Navigate + GetPaths.

SGTM

[achuithb, 2011-08-12 20:37:18]

PTAL

[Paweł Hajdan Jr., 2011-08-15 17:16:00]

LGTM

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:47: GURL Navigate(const
std::string& prefix) {
nit: Let's be more precise: NavigateToMockURL.

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:54: void GetPaths(const
std::string& prefix,
nit: Make the method name more precise: _what_ paths are they?

[achuithb, 2011-08-15 22:39:05]

I'm going to check this in. If you have further concerns, I'll address them in a
separate CL.

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:47: GURL Navigate(const
std::string& prefix) {
On 2011/08/15 17:16:00, Paweł Hajdan Jr. wrote:
> nit: Let's be more precise: NavigateToMockURL.

Done.

http://codereview.chromium.org/7606024/diff/20001/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:54: void GetPaths(const
std::string& prefix,
I've renamed this to GetDestinationPaths and added a comment to say that it
returns the paths of the destination file name and directory.

On 2011/08/15 17:16:00, Paweł Hajdan Jr. wrote:
> nit: Make the method name more precise: _what_ paths are they?

[Randy Smith (Not in Mondays), 2011-08-22 01:14:54]

LGTM.  Thanks for doing this.