DownloadManagerDelegate::ShouldCompleteDownload(callback)

DownloadManagerDelegate::ShouldCompleteDownload(callback)


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

[benjhayden, 2012-05-01 15:30:20]

Still need to dig into SafeBrowsing, but PTAL for everything else.

[asanka, 2012-05-01 15:54:09]

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:43: void
MarkAsComplete() {
Nit: Perhaps rename to something like CompleteDownload() since it's doing more
than marking it as complete.

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:144: const
base::Closure& maybe_complete_download) {
Nit: Just call it complete_cb or something generic.

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:151: (upload_data &&
upload_data->is_complete()))
Could upload_data be NULL at this point? It shouldn't because of a very critical
DI::UpdateObservers() call at DI::OnAllDataSaved().

Add a note to this effect?

http://codereview.chromium.org/10263019/diff/1/chrome/browser/download/chrome...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/1/chrome/browser/download/chrome...
chrome/browser/download/chrome_download_manager_delegate.cc:214: state->verdict
= DownloadProtectionService::SAFE;
Also store the Closure in |state| and invoke it from
CDMD::CheckClientDownloadDone.

http://codereview.chromium.org/10263019/diff/1/content/browser/download/downl...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10263019/diff/1/content/browser/download/downl...
content/browser/download/download_manager_impl.cc:647:
&DownloadManagerImpl::MaybeCompleteDownload, this, download)))
The download item could go away before the callback is invoked. You might want
to do a lookup by download ID instead. Ideally we'd use a weak reference.

[benjhayden, 2012-05-01 18:02:52]

PTAL

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:43: void
MarkAsComplete() {
On 2012/05/01 15:54:10, asanka wrote:
> Nit: Perhaps rename to something like CompleteDownload() since it's doing more
> than marking it as complete.

Done.

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:144: const
base::Closure& maybe_complete_download) {
On 2012/05/01 15:54:10, asanka wrote:
> Nit: Just call it complete_cb or something generic.

Done.

http://codereview.chromium.org/10263019/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:151: (upload_data &&
upload_data->is_complete()))
On 2012/05/01 15:54:10, asanka wrote:
> Could upload_data be NULL at this point? It shouldn't because of a very
critical
> DI::UpdateObservers() call at DI::OnAllDataSaved().
> 
> Add a note to this effect?

Done.

http://codereview.chromium.org/10263019/diff/1/chrome/browser/download/chrome...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/1/chrome/browser/download/chrome...
chrome/browser/download/chrome_download_manager_delegate.cc:214: state->verdict
= DownloadProtectionService::SAFE;
On 2012/05/01 15:54:10, asanka wrote:
> Also store the Closure in |state| and invoke it from
> CDMD::CheckClientDownloadDone.

Done.

http://codereview.chromium.org/10263019/diff/1/content/browser/download/downl...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10263019/diff/1/content/browser/download/downl...
content/browser/download/download_manager_impl.cc:647:
&DownloadManagerImpl::MaybeCompleteDownload, this, download)))
On 2012/05/01 15:54:10, asanka wrote:
> The download item could go away before the callback is invoked. You might want
> to do a lookup by download ID instead. Ideally we'd use a weak reference.

Done.

[Randy Smith (Not in Mondays), 2012-05-01 18:21:49]

Did you look at whether or not we can make DM::MaybeCompleteDownload
private/remove it from the DownloadManager interface with this change?

http://codereview.chromium.org/10263019/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/10263019/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.h:50: // becomes ready to
complete.  If you aren't careful to avoid setting
I'd actually just say "If this call is made multiple times with the download not
ready to complete, only one callback will be generated, to the last value passed
as maybe_complete_download."  Either forbid or permit; there is no discourage
:-}.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:145: const
base::Closure& maybe_complete_download) {
Same naming issue.  The phrase "maybe complete download" I think only makes
sense if you're looking at it from the perspective of the download execution
flow, not from outside.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:156: if
(!maybe_complete_download.is_null())
Don't you want this to be unilateral?  Doing it this way means that the last one
passed won't win, if it's null.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:243: if
(!complete_cb.is_null())
Same comment as for gdata; why not pass along a null callback?  It feels like
you're violating your interface (that last complete_cb wins) if you don't.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:247: return
!state->pending();
nit, suggestion: Naively, I'd expect this code not to set complete_cb if it
returns true.  I don't think it matters in terms of behavior, but it seems a
little weird to do.

http://codereview.chromium.org/10263019/diff/6003/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10263019/diff/6003/content/browser/download/do...
content/browser/download/download_manager_impl.cc:647:
&DownloadManagerImpl::MaybeCompleteDownloadById,
Hmmm.  I see why you're doing this.  Another alternative might be to pass in a
callback that expected to have a DownloadItem* passed into it, and rely on the
caller to provide that.  If they can't, they shouldn't be calling us, and all
current callers can (sorta obviously, since they already were.)  Neither
super-clean; your TODO points the way to good code.  But I wanted to at least
float the option that doesn't require adding a new routine.  Either way is fine.

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
content/public/browser/download_manager_delegate.h:63: //
|maybe_complete_download| is run at some point in the
I'm not sure maybe_complete_download is the right name for the parameter.  You
want to put yourself in the position of the caller; it's more a signal that
something really has happened?  on_download_ready_for_completion?

[benjhayden, 2012-05-01 19:00:30]

There is no DM::MaybeCompleteDownload, but there is
DownloadItem::MaybeCompleteDownload, and I could kill it so I did.

http://codereview.chromium.org/10263019/diff/11001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/10263019/diff/11001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_download_observer.h:50: // becomes ready to
complete.  If you aren't careful to avoid setting
On 2012/05/01 18:21:49, rdsmith wrote:
> I'd actually just say "If this call is made multiple times with the download
not
> ready to complete, only one callback will be generated, to the last value
passed
> as maybe_complete_download."  Either forbid or permit; there is no discourage
> :-}.

Done.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:145: const
base::Closure& maybe_complete_download) {
On 2012/05/01 18:21:49, rdsmith wrote:
> Same naming issue.  The phrase "maybe complete download" I think only makes
> sense if you're looking at it from the perspective of the download execution
> flow, not from outside.

Done.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:156: if
(!maybe_complete_download.is_null())
On 2012/05/01 18:21:49, rdsmith wrote:
> Don't you want this to be unilateral?  Doing it this way means that the last
one
> passed won't win, if it's null.

I did this intentionally in case another caller calls
CDMD::ShouldCompleteDownload() for the same download. If the new caller needs to
pass a different complete_cb, then we'll need to extend the mechanism to N
callbacks, but if the new caller doesn't need to pass a different complete_cb,
then it can pass the null callback and we won't need to change anything.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:243: if
(!complete_cb.is_null())
On 2012/05/01 18:21:49, rdsmith wrote:
> Same comment as for gdata; why not pass along a null callback?  It feels like
> you're violating your interface (that last complete_cb wins) if you don't.

"You can't unset it once you set it" was supposed to clarify the interface. See
my response to the other version of this comment.

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:247: return
!state->pending();
On 2012/05/01 18:21:49, rdsmith wrote:
> nit, suggestion: Naively, I'd expect this code not to set complete_cb if it
> returns true.  I don't think it matters in terms of behavior, but it seems a
> little weird to do.

Done.

http://codereview.chromium.org/10263019/diff/6003/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10263019/diff/6003/content/browser/download/do...
content/browser/download/download_manager_impl.cc:647:
&DownloadManagerImpl::MaybeCompleteDownloadById,
On 2012/05/01 18:21:49, rdsmith wrote:
> Hmmm.  I see why you're doing this.  Another alternative might be to pass in a
> callback that expected to have a DownloadItem* passed into it, and rely on the
> caller to provide that.  If they can't, they shouldn't be calling us, and all
> current callers can (sorta obviously, since they already were.)  Neither
> super-clean; your TODO points the way to good code.  But I wanted to at least
> float the option that doesn't require adding a new routine.  Either way is
fine.

The new routine with its lookup seems safer.

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
content/public/browser/download_manager_delegate.h:63: //
|maybe_complete_download| is run at some point in the
On 2012/05/01 18:21:49, rdsmith wrote:
> I'm not sure maybe_complete_download is the right name for the parameter.  You
> want to put yourself in the position of the caller; it's more a signal that
> something really has happened?  on_download_ready_for_completion?

Asanka suggested complete_cb, which is briefer.
LMK if you feel strongly.

[achuithb, 2012-05-01 19:45:21]

http://codereview.chromium.org/10263019/diff/2006/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/2006/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:58: base::Closure
complete_cb_;
I would prefer this be complete_callback_. We haven't shied from long names in
the rest of the gdata code.

[benjhayden, 2012-05-01 20:12:39]

PTAL

http://codereview.chromium.org/10263019/diff/2006/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/2006/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:58: base::Closure
complete_cb_;
On 2012/05/01 19:45:21, achuith.bhandarkar wrote:
> I would prefer this be complete_callback_. We haven't shied from long names in
> the rest of the gdata code.

Done.

[Randy Smith (Not in Mondays), 2012-05-02 18:14:07]

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:156: if
(!maybe_complete_download.is_null())
On 2012/05/01 19:00:30, benjhayden_chromium wrote:
> On 2012/05/01 18:21:49, rdsmith wrote:
> > Don't you want this to be unilateral?  Doing it this way means that the last
> one
> > passed won't win, if it's null.
> 
> I did this intentionally in case another caller calls
> CDMD::ShouldCompleteDownload() for the same download. If the new caller needs
to
> pass a different complete_cb, then we'll need to extend the mechanism to N
> callbacks, but if the new caller doesn't need to pass a different complete_cb,
> then it can pass the null callback and we won't need to change anything.

This feels like a hacky interface design--we're not designing for the double
callback case, we're designing for half of the double callback case.  I'd prefer
for us to have the simplest interface that meets our current requirements, and
wouldn't mind designing for the future if that's relatively simple and clean. 
But this seems like we're adding complexity to no good purpose.  If you don't
have strong feelings, I'd be more inclined to just allow null callbacks and have
them overwrite the previous callbacks.

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/6003/content/public/browser/down...
content/public/browser/download_manager_delegate.h:63: //
|maybe_complete_download| is run at some point in the
On 2012/05/01 19:00:30, benjhayden_chromium wrote:
> On 2012/05/01 18:21:49, rdsmith wrote:
> > I'm not sure maybe_complete_download is the right name for the parameter. 
You
> > want to put yourself in the position of the caller; it's more a signal that
> > something really has happened?  on_download_ready_for_completion?
> 
> Asanka suggested complete_cb, which is briefer.
> LMK if you feel strongly.

That's fine.

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
content/public/browser/download_manager_delegate.h:66: // will not be run.
This doesn't spec the behavior if this function is called multiple times, and it
should.  Consider the case where safe browsing blocks a download, then allows it
to complete, then gdrive blocks it; you'll get a callback after SB is done, but
ShouldCompleteDownload() will once again return false.  It's also worthwhile to
indicate whether or not it's ok to call before the callback's been run.

[benjhayden, 2012-05-03 19:21:14]

PTAL

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10263019/diff/6003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:156: if
(!maybe_complete_download.is_null())
On 2012/05/02 18:14:07, rdsmith wrote:
> On 2012/05/01 19:00:30, benjhayden_chromium wrote:
> > On 2012/05/01 18:21:49, rdsmith wrote:
> > > Don't you want this to be unilateral?  Doing it this way means that the
last
> > one
> > > passed won't win, if it's null.
> > 
> > I did this intentionally in case another caller calls
> > CDMD::ShouldCompleteDownload() for the same download. If the new caller
needs
> to
> > pass a different complete_cb, then we'll need to extend the mechanism to N
> > callbacks, but if the new caller doesn't need to pass a different
complete_cb,
> > then it can pass the null callback and we won't need to change anything.
> 
> This feels like a hacky interface design--we're not designing for the double
> callback case, we're designing for half of the double callback case.  I'd
prefer
> for us to have the simplest interface that meets our current requirements, and
> wouldn't mind designing for the future if that's relatively simple and clean. 
> But this seems like we're adding complexity to no good purpose.  If you don't
> have strong feelings, I'd be more inclined to just allow null callbacks and
have
> them overwrite the previous callbacks.  

Done.

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
content/public/browser/download_manager_delegate.h:66: // will not be run.
On 2012/05/02 18:14:07, rdsmith wrote:
> This doesn't spec the behavior if this function is called multiple times, and
it
> should.  Consider the case where safe browsing blocks a download, then allows
it
> to complete, then gdrive blocks it; you'll get a callback after SB is done,
but
> ShouldCompleteDownload() will once again return false.  It's also worthwhile
to
> indicate whether or not it's ok to call before the callback's been run.

PTAL
Feel free to suggest alternate text.

Also, I implemented the gateway/trampoline in CDMD in case both OS_CHROMEOS and
SAFE_BROWSING_ENABLED.

[Randy Smith (Not in Mondays), 2012-05-03 19:50:46]

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
content/public/browser/download_manager_delegate.h:66: // will not be run.
On 2012/05/03 19:21:14, benjhayden_chromium wrote:
> On 2012/05/02 18:14:07, rdsmith wrote:
> > This doesn't spec the behavior if this function is called multiple times,
and
> it
> > should.  Consider the case where safe browsing blocks a download, then
allows
> it
> > to complete, then gdrive blocks it; you'll get a callback after SB is done,
> but
> > ShouldCompleteDownload() will once again return false.  It's also worthwhile
> to
> > indicate whether or not it's ok to call before the callback's been run.
> 
> PTAL
> Feel free to suggest alternate text.
> 
> Also, I implemented the gateway/trampoline in CDMD in case both OS_CHROMEOS
and
> SAFE_BROWSING_ENABLED.

I was thinking about asking you to do that, but I think that it was actually ok
the way it was.  The issue is around how the consumer is expected to determine
if they should complete the download.  If we take the return value as
authoritative and the callback as a hint, the old code was ok--if both SB and
CrOS wanted to block the download, there would be an intermediate callback, and
then ShouldCompleteDownload would be called and it would still return false. 
Under the hood, the callback would be handed to CrOS instead of SB, but that's
ok.  

Given that's pretty much how the calling code (at least in DownloadManagerImpl)
wants to use ShouldCompleteDownload() and that the interface is designed to
assist that code, I think we're ok with the original code, and it's simpler. 
Let me know if you disagree--there may be something about the interface
subtleties I'm missing.

http://codereview.chromium.org/10263019/diff/26001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/26001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:69: // |complete_callback|
passed to it and begin returning true when called.
I'd phrase it somewhat differently.  I think what I'd say is that the last
complete_callback will be run when the return value of this function may have
changed to true, but that complete_callback being run doesn't actually indicate
that the download is ready for completion; the function needs to be called again
to check.  (And include that if the method returns true, complete_callback will
not be run.)  That gives a guideline for usage, and implies that the function
can be (is expected to be) called multiple times.

[benjhayden, 2012-05-04 17:02:53]

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/3006/content/public/browser/down...
content/public/browser/download_manager_delegate.h:66: // will not be run.
On 2012/05/03 19:50:46, rdsmith wrote:
> On 2012/05/03 19:21:14, benjhayden_chromium wrote:
> > On 2012/05/02 18:14:07, rdsmith wrote:
> > > This doesn't spec the behavior if this function is called multiple times,
> and
> > it
> > > should.  Consider the case where safe browsing blocks a download, then
> allows
> > it
> > > to complete, then gdrive blocks it; you'll get a callback after SB is
done,
> > but
> > > ShouldCompleteDownload() will once again return false.  It's also
worthwhile
> > to
> > > indicate whether or not it's ok to call before the callback's been run.
> > 
> > PTAL
> > Feel free to suggest alternate text.
> > 
> > Also, I implemented the gateway/trampoline in CDMD in case both OS_CHROMEOS
> and
> > SAFE_BROWSING_ENABLED.
> 
> I was thinking about asking you to do that, but I think that it was actually
ok
> the way it was.  The issue is around how the consumer is expected to determine
> if they should complete the download.  If we take the return value as
> authoritative and the callback as a hint, the old code was ok--if both SB and
> CrOS wanted to block the download, there would be an intermediate callback,
and
> then ShouldCompleteDownload would be called and it would still return false. 
> Under the hood, the callback would be handed to CrOS instead of SB, but that's
> ok.  
> 
> Given that's pretty much how the calling code (at least in
DownloadManagerImpl)
> wants to use ShouldCompleteDownload() and that the interface is designed to
> assist that code, I think we're ok with the original code, and it's simpler. 
> Let me know if you disagree--there may be something about the interface
> subtleties I'm missing.

That's how DM wants to use it now, but that's not how SavePackage wants to use
it. It would be simpler if SavePackage could pass &SP::Finish to CDMD instead of
writing a new MaybeFinish() method, just to support MHTML+gdata. MaybeFinish()
would look a little like the conditional in SavePackage::OnDownloadUpdated()
that you love so much. :-)

http://codereview.chromium.org/10263019/diff/26001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/26001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:69: // |complete_callback|
passed to it and begin returning true when called.
On 2012/05/03 19:50:47, rdsmith wrote:
> I'd phrase it somewhat differently.  I think what I'd say is that the last
> complete_callback will be run when the return value of this function may have
> changed to true, but that complete_callback being run doesn't actually
indicate
> that the download is ready for completion; the function needs to be called
again
> to check.  (And include that if the method returns true, complete_callback
will
> not be run.)  That gives a guideline for usage, and implies that the function
> can be (is expected to be) called multiple times.

Please just give the exact text that you want here so that I can copy-n-paste
it.

[Randy Smith (Not in Mondays), 2012-05-07 01:05:26]

On 2012/05/04 17:02:53, benjhayden_chromium wrote:
> That's how DM wants to use it now, but that's not how SavePackage wants to use
> it. It would be simpler if SavePackage could pass &SP::Finish to CDMD instead
of
> writing a new MaybeFinish() method, just to support MHTML+gdata. MaybeFinish()
> would look a little like the conditional in SavePackage::OnDownloadUpdated()
> that you love so much. :-)

Fair point, though a bit scary.  How bit a hassle would it be to include the
SavePackage changes in this CL?  If we're working on specing a new API, it's
worthwhile looking at all (currently planned) callers at the same time for
requirements. 

(Also: D'oh, can't believe I forgot about SavePackage.  But if you include it
it'll be much easier for me to balance the two in my head.)

> On 2012/05/03 19:50:47, rdsmith wrote:
> > I'd phrase it somewhat differently.  I think what I'd say is that the last
> > complete_callback will be run when the return value of this function may
have
> > changed to true, but that complete_callback being run doesn't actually
> indicate
> > that the download is ready for completion; the function needs to be called
> again
> > to check.  (And include that if the method returns true, complete_callback
> will
> > not be run.)  That gives a guideline for usage, and implies that the
function
> > can be (is expected to be) called multiple times.
> 
> Please just give the exact text that you want here so that I can copy-n-paste
> it.

Sure, though I'm going to delay until we've actually figured out the interface
spec we want :-}.  (Though I'll admit I'm torn between thinking that I should
make you document it clearly for the practice, and thinking that I'm picky and
specific enough about interface documentation that that could be a
million-monkey/PS exercise.  Please let me know if you think I'm being picayune
in a way that doesn't have useful value.)

[benjhayden, 2012-05-08 22:44:21]

PTAL.
Merged with the MHTML/SavePackage CL so you can see how SavePackage will use
this callback.

[Randy Smith (Not in Mondays), 2012-05-09 00:50:56]

I think I'll say: WDYT?

(This is very close--I think the only sticking point might be if I'm suggesting
a silly rewrite.)

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:85: // The verdict
that we got from calling CheckClientDownload.
nit, suggestion: Add "Only valid to call if |!pending()|".

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:250: void
ChromeDownloadManagerDelegate::ShouldCompleteDownloadInternal(
The splits of callback setup code for the subsystems between these two functions
feels weird to me.  We pass callbacks to GData in both functions, and safe
browsing only in ShouldCompleteDownload.  and there's a reasonable amount of
code and ifdef duplication.  Can we refactor this to be a bit cleaner?  I'm
thinking something like:

IsDownloadReadyForCompletion: Returns true if ready, otherwise setups up passed
in callback to be called back (by whichever subsystem is responsible for the
block).

ShouldCompleteDownloadInternal: Probes IsDownloadReadyForCompletion() and calls
the user callback if it returns true.  

Possibly IsDownloadReadyForCompletion could be spelled ShouldCompleteDownload. 
This all might require storing the user complete callback in the external data
on the download item, or possibly there's callback magic that could get around
that.

http://codereview.chromium.org/10263019/diff/42001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/42001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:84: // function needs to be
called again to check.  If the
Minor point: This is false to how this interface is used in save package, i.e.
we're counting on undocumented behavior, which isn't good.  However, I think
we're rewriting this anyway :-}.  Suggested doc text (please feel free to tweak
if you believe clarity would be enhanced):

Allows the delegate to delay completion of the download.  This function will
either return true (in which case the download is ready to complete) or arrange
for complete_callback to be called at some point in the future when the download
is ready to complete.  

The function may be called multiple times; if it is, only the last callback
specified (while the delegate is delaying completion) will be run.  Calls made
after the callback is run are guaranteed to return true.

[benjhayden, 2012-05-14 15:21:53]

merge

[benjhayden, 2012-05-14 15:35:04]

PTAL

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:85: // The verdict
that we got from calling CheckClientDownload.
On 2012/05/09 00:50:56, rdsmith wrote:
> nit, suggestion: Add "Only valid to call if |!pending()|".

Done.

http://codereview.chromium.org/10263019/diff/42001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:250: void
ChromeDownloadManagerDelegate::ShouldCompleteDownloadInternal(
On 2012/05/09 00:50:56, rdsmith wrote:
> The splits of callback setup code for the subsystems between these two
functions
> feels weird to me.  We pass callbacks to GData in both functions, and safe
> browsing only in ShouldCompleteDownload.  and there's a reasonable amount of
> code and ifdef duplication.  Can we refactor this to be a bit cleaner?  I'm
> thinking something like:
> 
> IsDownloadReadyForCompletion: Returns true if ready, otherwise setups up
passed
> in callback to be called back (by whichever subsystem is responsible for the
> block).
> 
> ShouldCompleteDownloadInternal: Probes IsDownloadReadyForCompletion() and
calls
> the user callback if it returns true.  
> 
> Possibly IsDownloadReadyForCompletion could be spelled ShouldCompleteDownload.

> This all might require storing the user complete callback in the external data
> on the download item, or possibly there's callback magic that could get around
> that.

Sorry, I don't see how that's different from what's already here.

WDYT of an ExternalData intermediate base class, and maybe a shadow
GDataDownloadObserver::IsReadyToComplete() that does not take a callback?

http://codereview.chromium.org/10263019/diff/42001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10263019/diff/42001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:84: // function needs to be
called again to check.  If the
On 2012/05/09 00:50:56, rdsmith wrote:
> Minor point: This is false to how this interface is used in save package, i.e.
> we're counting on undocumented behavior, which isn't good.  However, I think
> we're rewriting this anyway :-}.  Suggested doc text (please feel free to
tweak
> if you believe clarity would be enhanced):
> 
> Allows the delegate to delay completion of the download.  This function will
> either return true (in which case the download is ready to complete) or
arrange
> for complete_callback to be called at some point in the future when the
download
> is ready to complete.  
> 
> The function may be called multiple times; if it is, only the last callback
> specified (while the delegate is delaying completion) will be run.  Calls made
> after the callback is run are guaranteed to return true.  

LGTM Thanks!

[Randy Smith (Not in Mondays), 2012-05-14 18:30:31]

Summary of offline conversation: I'm suggesting a rewrite that'll combine all
the safe browsing and gdata logic in one function.  That function'll be very
similar to ShouldCompleteDownload(), except that it won't guarantee that when
the callback is called the download is ready for completion, and we'll use
callback magic to translate that semantics into ShouldCompleteDownload().  Quick
summary of functions:


ShouldCompleteDownload:
  return
IsDownloadReadyForCompletion(ShouldCompleteDownloadInternal(user_callback))    

ShouldCompleteDownloadInternal(user_callback):
  if (IsDownloadReadyForCompletion)
    calls user callback

IsDownloadReadyForCompletion(callback)
  # Probes one, setting up callback (?), returns false if not ready
  # Probes second, setting up callback (?), returns false if not ready
  # returns true

http://codereview.chromium.org/10263019/diff/53001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/53001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:283: return true;
Isn't this a bug, if a not currently important one because Gdata and safe
browsing don't happen together?  I.e. if you enter this function when safe
browsing is happy and GData is not, won't you return true when you shouldn't?

[benjhayden, 2012-05-15 21:25:27]

PTAL

http://codereview.chromium.org/10263019/diff/53001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/53001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:283: return true;
On 2012/05/14 18:30:31, rdsmith wrote:
> Isn't this a bug, if a not currently important one because Gdata and safe
> browsing don't happen together?  I.e. if you enter this function when safe
> browsing is happy and GData is not, won't you return true when you shouldn't?

Done.

[Randy Smith (Not in Mondays), 2012-05-16 19:04:53]

I like it.  With nits below (suggestion up to you) LGTM.

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:287: // because it
can return true synchronously.
nit: Shouldn't this comment be in front of ShouldCompleteDownloadInternal?

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
File chrome/browser/download/download_completion_blocker.h (right):

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
chrome/browser/download/download_completion_blocker.h:14: // completion of a
DownloadItem.
I'd be explicit about the semantics of this class, i.e. that completion will
call the most recent specified callback, and will only call it once.  You might
also want to specify whether or not it's valid to call CompleteDownload()
multiple times (no, I don't have a use case, but to me that just means forbid it
:-}) and what happens if you do.

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
chrome/browser/download/download_completion_blocker.h:29: // Subclasses must
chain if they override this method.
nit, suggestion: My guess as to what you meant by "chain" is "call this method
from their override", but I'm not sure; maybe be more explicit?

[asanka, 2012-05-16 19:26:32]

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:279: item,
user_complete_callback)))
Is the DownloadItem* guaranteed to outlive the callback?

[benjhayden, 2012-05-16 19:42:29]

PTAL

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:279: item,
user_complete_callback)))
On 2012/05/16 19:26:32, asanka wrote:
> Is the DownloadItem* guaranteed to outlive the callback?

Good catch!
How're those weak pointers coming? :-)

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:287: // because it
can return true synchronously.
On 2012/05/16 19:04:53, rdsmith wrote:
> nit: Shouldn't this comment be in front of ShouldCompleteDownloadInternal?

Done.

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
File chrome/browser/download/download_completion_blocker.h (right):

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
chrome/browser/download/download_completion_blocker.h:14: // completion of a
DownloadItem.
On 2012/05/16 19:04:53, rdsmith wrote:
> I'd be explicit about the semantics of this class, i.e. that completion will
> call the most recent specified callback, and will only call it once.  You
might
> also want to specify whether or not it's valid to call CompleteDownload()
> multiple times (no, I don't have a use case, but to me that just means forbid
it
> :-}) and what happens if you do.

Done.

http://codereview.chromium.org/10263019/diff/42003/chrome/browser/download/do...
chrome/browser/download/download_completion_blocker.h:29: // Subclasses must
chain if they override this method.
On 2012/05/16 19:04:53, rdsmith wrote:
> nit, suggestion: My guess as to what you meant by "chain" is "call this method
> from their override", but I'm not sure; maybe be more explicit?

Done.

[commit-bot: I haz the power, 2012-05-18 19:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10263019/53005

[commit-bot: I haz the power, 2012-05-18 20:42:38]

Change committed as 137949