Use defer out-params instead of ResourceDispatcherHostImpl::PauseRequest(...true)

Add defer out params to ResourceHandler's OnResponseStarted
and OnReadCompleted methods.  Use that to pause the request
instead of directly calling ResourceDispatcherHostImpl's
PauseRequest function.

Eliminate direct calls to PauseRequest, with the exception of
resource_dispatcher_host_unittest.cc and some uses internal to
ResourceDispatcherHostImpl.  Those should ultimately be 
changed to use the defer out-param approach too.

This CL also changes the DownloadRequestHandle to talk back to
the DownloadResourceHandler in order to pause / resume / cancel
the URLRequest.  The handle keeps a reference to the handler,
which may extend the lifetime of the handler, but that should be
OK given that the handler already supports having its lifetime
extended beyond the URLRequest.

Now, instead of the DownloadFileManager calling PauseRequest, we
just wait for the SetDownloadID call to resume the URLRequest.
This simplifies some of the logic.  The Pause/ResumeRequest
issued on a DownloadRequestHandle gets its own pause counter,
which has nothing to do with the internal pausing that the
DownloadResourceHandler does when it is waiting for a DownloadId.
See DownloadResourceHandler::MaybeResumeRequest for the logic
that handles the conditions for resuming the URLRequest.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138111

[Randy Smith (Not in Mondays), 2012-05-17 15:53:48]

Did a quick, high level scan, primarily focussing on downloads.  Overall
approach looks good.  Happy to do a more thorough review when you think the CL
is ready.

It's wonderful to see this change happening.

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
File content/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
content/browser/download/download_request_handle.h:76:
scoped_refptr<DownloadResourceHandler> handler_;
Shouldn't this be a weak pointer?  I could easily imagine the handle outliving
the Handler.

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
content/browser/download/download_resource_handler.cc:382: // append data to it.
 Both of those operations happen on the FILE thread.
So just to be explicit, the ordering semantics you're relying on here are that
if we do:

[UI]
Post(FILE, taskA)
Post(IO, taskB(post(FILE, taskC)))

that taskA will be executed before taskC because it is put on the FILE thread
message queue first?

(I think this will work, though it makes me wince--OTOH, all this goes away as
soon as I land the ByteStream refactor, so I mostly just want to make sure I
understand it.)

[darin (slow to review), 2012-05-17 21:17:57]

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
File content/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
content/browser/download/download_request_handle.h:76:
scoped_refptr<DownloadResourceHandler> handler_;
On 2012/05/17 15:53:48, rdsmith wrote:
> Shouldn't this be a weak pointer?  I could easily imagine the handle outliving
> the Handler.

It seems OK to extend the lifetime of the Handler.  It is thread-safe ref
counted and it gets destroyed on the IO thread.  A weak pointer would also be OK
since the handler lives as long as the URLRequest, and if the URLRequest is
gone, then there is no point in pausing / resuming / cancelling the URLRequest. 
Hmm... maybe weak is better.

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/40/content/browser/download/down...
content/browser/download/download_resource_handler.cc:382: // append data to it.
 Both of those operations happen on the FILE thread.
On 2012/05/17 15:53:48, rdsmith wrote:
> So just to be explicit, the ordering semantics you're relying on here are that
> if we do:
> 
> [UI]
> Post(FILE, taskA)
> Post(IO, taskB(post(FILE, taskC)))
> 
> that taskA will be executed before taskC because it is put on the FILE thread
> message queue first?
> 
> (I think this will work, though it makes me wince--OTOH, all this goes away as
> soon as I land the ByteStream refactor, so I mostly just want to make sure I
> understand it.)

Yes, you are understanding correctly.  This enables me to get rid of the
DownloadRequestHandle::ResumeRequest() call made from download_file_manager.cc. 
I realized that all the DownloadResourceHandler needs is the DownloadId in order
to proceed with sending data to the FILE thread.

[darin (slow to review), 2012-05-17 21:34:01]

Ready for review, thanks!

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

Looks fairly good.  You might want to get another reviewer for
cross_site_resource_handler.cc; I did review it, but I don't know that code.

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.cc:384:
download_file_manager_->StartDownload(info.release(), handle);
Actually, when I think about this a little bit more, it scares me, in that the
file thread might generate posts to the IO thread (specifically in the case of
an error--I think there are currently cancels that target the IO thread without
going through the UI thread) which then will try to propagate failures to the UI
thread with a download id that hasn't been set.  It's an unlikely race, and it
may be an impossible one (in that I think the FILE->IO cancels only occur if the
download manager doesn't exist) but I'd feel better if we left the high level
logic alone, i.e. start the download in a paused state and have it be resumed
from the file thread after the download file is created.

Note that all of this is going to go down the toilet with a very satisfying
flushing sound as soon as I land http://codereview.chromium.org/10392111/.  But
my preference would be not to rock the boat at the inter-thread race level until
that point.

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.h:86: std::string
DebugString() const;
suggestion: My care factor isn't high, but I have a preference for DebugString()
being the last public method.

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.h:138: int pause_count_;
nit: Small comment ahead of these describing the state machine they implement?

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:293: else if
(info->allow_download() && ShouldDownload(NULL)) {
Suggestion (as I can't find any support for my position in the style guide): I
find the separation of the else from its previous curly brace jarring; any
chance that it could be put on that line and the comment moved underneath the if
without it being confusing?

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:336: // We don't want
to keep buffering as our buffer will fill up.
nit, suggestion: This comment feels a little strange to me, disassociated from
the pause.  I think it'd make more sense at the places where we translate the
return value from ShouldWaitForPlugins into a *defer = true;

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:396:
DCHECK(!defer_ignored);
What's the rational for this?

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:419: bool* defer) {
nit: Why two lines?

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
File content/browser/renderer_host/throttling_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/throttling_resource_handler.cc:91: index_ = 0;  //
Reset for next time.
Not relevant to this CL, just for my curiosity: throttles can't change their
mind after they've said ok?

[darin (slow to review), 2012-05-18 23:09:04]

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.cc:384:
download_file_manager_->StartDownload(info.release(), handle);
DownloadFileManager::StartDownload() gets called on the UI thread from
DownloadResourceHandler::StartOnUIThread(), and that calls
DownloadFileManager::CreateDownloadFile() on the FILE thread. 
CreateDownloadFile() just posts messages to the UI thread.

I don't see any problems.  What am I missing?

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.h:86: std::string
DebugString() const;
On 2012/05/18 20:17:02, rdsmith wrote:
> suggestion: My care factor isn't high, but I have a preference for
DebugString()
> being the last public method.

OK

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:293: else if
(info->allow_download() && ShouldDownload(NULL)) {
On 2012/05/18 20:17:02, rdsmith wrote:
> Suggestion (as I can't find any support for my position in the style guide): I
> find the separation of the else from its previous curly brace jarring; any
> chance that it could be put on that line and the comment moved underneath the
if
> without it being confusing?

Yes, that can probably be done.

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:336: // We don't want
to keep buffering as our buffer will fill up.
On 2012/05/18 20:17:02, rdsmith wrote:
> nit, suggestion: This comment feels a little strange to me, disassociated from
> the pause.  I think it'd make more sense at the places where we translate the
> return value from ShouldWaitForPlugins into a *defer = true;

Agreed.

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:396:
DCHECK(!defer_ignored);
On 2012/05/18 20:17:02, rdsmith wrote:
> What's the rational for this?  

The old code fails to handle pausing done by the old handler.  I think it should
probably be fixed.  This DCHECK will complain if the handler does try to pause,
and then we can notice that and fix this code to handle it properly.

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:419: bool* defer) {
On 2012/05/18 20:17:02, rdsmith wrote:
> nit: Why two lines?

Looks like the first parameter actually fits on the previous line.

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
File content/browser/renderer_host/throttling_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/throttling_resource_handler.cc:91: index_ = 0;  //
Reset for next time.
In this comment, "next time" refers to the next notification stage.  A throttle
will not see more than one OnResponseStarted event in other words.

[Randy Smith (Not in Mondays), 2012-05-19 00:42:14]

LGTM with changes agreed to.

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/download/do...
content/browser/download/download_resource_handler.cc:384:
download_file_manager_->StartDownload(info.release(), handle);
On 2012/05/18 23:09:05, darin wrote:
> DownloadFileManager::StartDownload() gets called on the UI thread from
> DownloadResourceHandler::StartOnUIThread(), and that calls
> DownloadFileManager::CreateDownloadFile() on the FILE thread. 
> CreateDownloadFile() just posts messages to the UI thread.
> 
> I don't see any problems.  What am I missing?

You're right, though I'm still nervous.  What I was remembering was the
occasionally DFM::CancelDownloadOnRename->DF::CancelDownloadRequest->IO, but
there isn't any such pathway in the creation path.  It's fragile code (IMO), but
that doesn't make it unusual in context :-J and it'll go away soon, so I'm good
with it.

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/10332130/diff/2016/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:396:
DCHECK(!defer_ignored);
On 2012/05/18 23:09:05, darin wrote:
> On 2012/05/18 20:17:02, rdsmith wrote:
> > What's the rational for this?  
> 
> The old code fails to handle pausing done by the old handler.  I think it
should
> probably be fixed.  This DCHECK will complain if the handler does try to
pause,
> and then we can notice that and fix this code to handle it properly.

SG.

[jam, 2012-05-21 15:35:03]

lgtm