Make web store resilient against download completion happening before opening.

Make web store resilient against download completion happening before opening.

This isn't needed currently, but if we change the point at which a
download is considered complete to when we finish writing the file and
release it (as is about to happen), subsystems that have the download
auto-open (such as web store install) will need to wait for open to
occur rather than completion.

R=benjhayden@chromium.org
R=aa@chromium.org


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

[Randy Smith (Not in Mondays), 2012-09-10 00:16:03]

Ben, Aaron: Could you take a look at this CL?  Ben, this will look familiar to
you--peeling off pieces from my monster.  Aaron: This is just to make sure I'm
not making any mistaken assumptions in the context of Webstore install.

[benjhayden, 2012-09-10 14:48:52]

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.cc:69: return true;
Are you sure that OnDownloadDestroyed() will never run between
OnDownloadUpdated() and MessageLoop::Quit()? Couldn't hurt to double-check
filter_.Run(item_) here, could it?

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.h (right):

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:25: typedef
base::Callback<bool(DownloadItem*)> EventFilter;
Comment that this may be called multiple times? And may even be called after it
returns true?

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:42: };
DISALLOW_COPY_AND_ASSIGN?

[Randy Smith (Not in Mondays), 2012-09-10 17:16:42]

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.cc:69: return true;
On 2012/09/10 14:48:52, benjhayden_chromium wrote:
> Are you sure that OnDownloadDestroyed() will never run between
> OnDownloadUpdated() and MessageLoop::Quit()? Couldn't hurt to double-check
> filter_.Run(item_) here, could it?

I'm not quite sure the race you're protecting against, but OnDownloadDestroyed()
happening after OnDownloadUpdated() and before the return from the message loop
would result in a false negative, so I rewrite the class to protect against
that.  Let me know what you think.

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.h (right):

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:25: typedef
base::Callback<bool(DownloadItem*)> EventFilter;
On 2012/09/10 14:48:52, benjhayden_chromium wrote:
> Comment that this may be called multiple times? And may even be called after
it
> returns true?

Done.

http://codereview.chromium.org/10918136/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:42: };
On 2012/09/10 14:48:52, benjhayden_chromium wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[benjhayden, 2012-09-10 17:37:29]

http://codereview.chromium.org/10918136/diff/8002/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/8002/content/public/test/downloa...
content/public/test/download_test_observer.cc:65: return event_seen_;
Nice!

What if WaitForEvent() is called on the same DUO twice?
bool seen = event_seen_;
event_seen_ = false;
return seen;

[Randy Smith (Not in Mondays), 2012-09-10 18:32:34]

I'm postponing try jobs for a bit; I think I'm not going to get them to work
without a merge to nearer top of tree, and I have a bit more work in other
branches to do before I'm ready for that.

http://codereview.chromium.org/10918136/diff/8002/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/8002/content/public/test/downloa...
content/public/test/download_test_observer.cc:65: return event_seen_;
On 2012/09/10 17:37:29, benjhayden_chromium wrote:
> Nice!
> 
> What if WaitForEvent() is called on the same DUO twice?
> bool seen = event_seen_;
> event_seen_ = false;
> return seen;

I think of this type of class as tracking whether the event occurred in the
window between when the class is constructed and when WaitForEvent() is called. 
For that, event_seen_ shouldn't be reset.  Having said that, it *should* be
tested upon entry so that you don't wait forever.  I'll fix that.

[Aaron Boodman, 2012-09-11 21:11:36]

Removing self from reviewers. I don't know this code well enough to be of much
use.

[Randy Smith (Not in Mondays), 2012-09-12 17:31:47]

Ben: Ping?

[benjhayden, 2012-09-12 17:42:14]

lgtm

http://codereview.chromium.org/10918136/diff/13005/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10918136/diff/13005/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1607: downloads[0],
base::Bind(WasAutoOpened)).WaitForEvent();
Why don't you need/want the & address-of operator before the function name?

[Randy Smith (Not in Mondays), 2012-09-12 19:11:39]

http://codereview.chromium.org/10918136/diff/13005/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10918136/diff/13005/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1607: downloads[0],
base::Bind(WasAutoOpened)).WaitForEvent();
On 2012/09/12 17:42:14, benjhayden_chromium wrote:
> Why don't you need/want the & address-of operator before the function name?

Pure oversight.  I'm not sure why the compiler didn't object, but I'm not
inclined to worry about it.  Done.

[commit-bot: I haz the power, 2012-09-12 19:12:05]

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

[commit-bot: I haz the power, 2012-09-12 19:12:09]

Presubmit check for 10918136-4008 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/test

Presubmit checks took 1.6s to calculate.

[Randy Smith (Not in Mondays), 2012-09-13 20:16:30]

Scott: Rubbertstamp (or real review :-}) for content/public/test?

[sky, 2012-09-13 21:15:55]

LGTM

http://codereview.chromium.org/10918136/diff/4008/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/4008/content/public/test/downloa...
content/public/test/download_test_observer.cc:49: : item_(item),
filter_(filter), waiting_(false), event_seen_(false) {
each param on its own line.

[Randy Smith (Not in Mondays), 2012-09-13 21:33:15]

Thanks!

http://codereview.chromium.org/10918136/diff/4008/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10918136/diff/4008/content/public/test/downloa...
content/public/test/download_test_observer.cc:49: : item_(item),
filter_(filter), waiting_(false), event_seen_(false) {
On 2012/09/13 21:15:55, sky wrote:
> each param on its own line.

Done.

[commit-bot: I haz the power, 2012-09-13 21:34:32]

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

[commit-bot: I haz the power, 2012-09-13 23:48:48]

Change committed as 156683