Checking that items exist in the downloads panel on ChromeOS.

Checking that items exist in the downloads panel on ChromeOS.

BUG=chromium-os:16914
TEST=Download browser tests should continue to pass on all platforms.

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

[achuithb, 2011-06-24 00:10:06]

I added some support to access the downloads list from the download panel here:
http://codereview.chromium.org/7241007/

Now I'm able to check that the download exists in the download browser tests.

Next I'll be able to check that the download exists in the save_package browser
tests as well. But for now, let me know what you think of these changes. Thanks!

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (left):

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:611: 
Fix lint error.

[achuithb, 2011-06-24 00:11:30]

Also modified the comments so they are consistent everywhere.

[Paweł Hajdan Jr., 2011-06-24 08:36:33]

Drive-by with testing comments. No need to wait for me if you just apply all of
them.

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:783: DCHECK(popup);
No DCHECKs in test code. They crash the entire thing and don't work in Release
mode. I'm pretty sure you want ASSERT_TRUE.

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:789: EXPECT_EQ(downloads.size(),
size_t(1));
nit: 1U instead of size_t(1).

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:800: #endif
nit:  // defined(OS_CHROMEOS)

[Randy Smith (Not in Mondays), 2011-06-24 17:39:54]

Achuith, two high level questions:

* Can we do the same check in Chrome main (for the download shelf)?  I'd like to
minimize the divergence between ChromeOS and Chrome.

* If we can't, can we put these checks inside CheckDownloadUIVisible (possibly
adding arguments to that function)?  It seems misleading to add an apparently
generic call to a whole lot of tests that only actually checks something on one
OS.

[achuithb, 2011-06-24 21:08:53]

PTAL.

On 2011/06/24 17:39:54, rdsmith wrote:
> Achuith, two high level questions:
> 
> * Can we do the same check in Chrome main (for the download shelf)?  I'd like
to
> minimize the divergence between ChromeOS and Chrome.

I looked into this, and there's N variants of DownloadShelf for views, gtk, mac,
etc. The base class doesn't have any data members, and I'm going to have to
touch a lot of files to do this check. 

I don't know the history of the download shelf, so I'm not sure if there have
been bugs in the past with the UI showing the wrong item. If this hasn't
happened, it's probably not a big win to add this additional check right now.
I've added a TODO for this. Hopefully that's ok.

> * If we can't, can we put these checks inside CheckDownloadUIVisible (possibly
> adding arguments to that function)?  It seems misleading to add an apparently
> generic call to a whole lot of tests that only actually checks something on
one
> OS.

I started to go down this path but didn't like the fact that we'd have a
function with 4 arguments. But it actually turned out cleaner. Done.

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:783: DCHECK(popup);
On 2011/06/24 08:36:33, Paweł Hajdan Jr. wrote:
> No DCHECKs in test code. They crash the entire thing and don't work in Release
> mode. I'm pretty sure you want ASSERT_TRUE.

Done.

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:789: EXPECT_EQ(downloads.size(),
size_t(1));
On 2011/06/24 08:36:33, Paweł Hajdan Jr. wrote:
> nit: 1U instead of size_t(1).

Done.

http://codereview.chromium.org/7245013/diff/7001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:800: #endif
On 2011/06/24 08:36:33, Paweł Hajdan Jr. wrote:
> nit:  // defined(OS_CHROMEOS)

There's now also an else clause for non-chromeos. Since this thread:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
didn't produce a good answer, I'm going to leave out the comments altogether
since the block is short enough to fit in one screen. Hope that's ok.

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (left):

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:611: 
Fix lint error.

[Randy Smith (Not in Mondays), 2011-06-26 21:11:45]

LGTM; you can take the below suggestion or not as you choose.

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:778: // is correct (currently
only on chromeos).
nit: Suggested comment change "filename is correct" --> "filename is present in
download UI".

[Paweł Hajdan Jr., 2011-06-27 10:19:15]

Code I commented in the drive-by LGTM.

[achuithb, 2011-06-27 18:34:21]

Thanks for the reviews Randy & Pawel!

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7245013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:778: // is correct (currently
only on chromeos).
On 2011/06/26 21:11:45, rdsmith wrote:
> nit: Suggested comment change "filename is correct" --> "filename is present
in
> download UI".

Done.

[commit-bot: I haz the power, 2011-06-27 20:37:52]

Change committed as 90634