Add flag to specify if explicitly requested download is from web.

Add flag to specify if explicitly requested download is from web.

This allows interposing the DownloadResourceThrottle on all web 
downloads, not just those occuring because of navigations.

BUG=127522
R=darin@chromium.org
TEST=Retry example in referenced issue; look for throttling message.


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

[Randy Smith (Not in Mondays), 2012-05-12 16:46:46]

Darin: My apologies, but I think you're the right reviewer for this, both
because you know the resource handling code best, and because I think I'm
modifying code you put in a little while back.  WDYT?

On specific question: I got rid of the "is_new_request" boolean to the
DownloadStarting() delegate as I generally want to keep the number of arguments
to functions as low as possible, and I don't think there are any layering
violations involved in accessing a URLRequest from chrome.  OTOH, there must be
some reason that flag was there, so that removal may have been a mistake.  Just
wanting to raise the flag to let you object or pull in another reviewer as
appropriate.

[darin (slow to review), 2012-05-12 18:23:07]

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/downloa...
File chrome/browser/download/download_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/downloa...
chrome/browser/download/download_extension_api.cc:506: false /* from_web */,
nit: use C++ style comment

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_installer.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:47: false /* from_web */,
nit: use C++ style comment

false,  // from_web

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:187: if
(from_web) {
elsewhere, we use "content-initiated" versus "browser-initiated" for things like
this.  maybe is_content_initiated would be better here?

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:193: //
If this isn't a new request, we've seen this before and added the safe
nit: this comment should be changed to say "and added the [standard resource
throttles] already..."

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:195: if
(!request->is_pending()) {
Definitely OK to make this change.  I had this on my list of changes to make
too.  (I actually foresee a lot more cleanup to the
ResourceDispatcherHostDelegate methods.)

https://chromiumcodereview.appspot.com/10381122/diff/1/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/content/browser/downlo...
content/browser/download/download_manager_impl.cc:98: false /* from_web */,
nit: use C++ style comment

[Randy Smith (Not in Mondays), 2012-05-12 19:56:37]

All comments incorporated; apologies for the style nits.  PTAL.

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/downloa...
File chrome/browser/download/download_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/downloa...
chrome/browser/download/download_extension_api.cc:506: false /* from_web */,
On 2012/05/12 18:23:08, darin wrote:
> nit: use C++ style comment

Done.

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_installer.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:47: false /* from_web */,
On 2012/05/12 18:23:08, darin wrote:
> nit: use C++ style comment
> 
> false,  // from_web

Done.

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:187: if
(from_web) {
On 2012/05/12 18:23:08, darin wrote:
> elsewhere, we use "content-initiated" versus "browser-initiated" for things
like
> this.  maybe is_content_initiated would be better here?

Done.

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:193: //
If this isn't a new request, we've seen this before and added the safe
On 2012/05/12 18:23:08, darin wrote:
> nit: this comment should be changed to say "and added the [standard resource
> throttles] already..."

Done.

https://chromiumcodereview.appspot.com/10381122/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:195: if
(!request->is_pending()) {
On 2012/05/12 18:23:08, darin wrote:
> Definitely OK to make this change.  I had this on my list of changes to make
> too.  (I actually foresee a lot more cleanup to the
> ResourceDispatcherHostDelegate methods.)

Done.

https://chromiumcodereview.appspot.com/10381122/diff/1/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/1/content/browser/downlo...
content/browser/download/download_manager_impl.cc:98: false /* from_web */,
On 2012/05/12 18:23:08, darin wrote:
> nit: use C++ style comment

Done.

[darin (slow to review), 2012-05-13 17:17:45]

LGTM

[benjhayden, 2012-05-14 13:51:51]

Two questions, no objections.

https://chromiumcodereview.appspot.com/10381122/diff/2005/chrome/browser/down...
File chrome/browser/download/download_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/2005/chrome/browser/down...
chrome/browser/download/download_extension_api.cc:506: false,  //
is_content_initiated
This |false| just means that downloads.download() will not be throttled, right?

https://chromiumcodereview.appspot.com/10381122/diff/2005/content/browser/dow...
File content/browser/download/download_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/2005/content/browser/dow...
content/browser/download/download_manager_impl.cc:98: false,  //
is_content_initiated
This should be configurable via DownloadUrlParameters, right?

[Randy Smith (Not in Mondays), 2012-05-14 15:55:27]

https://chromiumcodereview.appspot.com/10381122/diff/2005/chrome/browser/down...
File chrome/browser/download/download_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/2005/chrome/browser/down...
chrome/browser/download/download_extension_api.cc:506: false,  //
is_content_initiated
On 2012/05/14 13:51:51, benjhayden_chromium wrote:
> This |false| just means that downloads.download() will not be throttled,
right?

Yes, although I could imagine that we might add other meanings of "not
completely trusted" in the future.

https://chromiumcodereview.appspot.com/10381122/diff/2005/content/browser/dow...
File content/browser/download/download_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10381122/diff/2005/content/browser/dow...
content/browser/download/download_manager_impl.cc:98: false,  //
is_content_initiated
On 2012/05/14 13:51:51, benjhayden_chromium wrote:
> This should be configurable via DownloadUrlParameters, right?

Probably, although I don't have strong feelings given that all callers to
DM::BeginDownload are not content initiated currently.  But if we switch
everything over to going through DM::BeginDownload, it would have to be.

[commit-bot: I haz the power, 2012-05-14 15:56:22]

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

[commit-bot: I haz the power, 2012-05-14 15:56:27]

Can't apply patch for file content/browser/download/download_manager_impl.cc.
While running patch -p1 --forward --force;
patching file content/browser/download/download_manager_impl.cc
Hunk #1 FAILED at 95.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/download/download_manager_impl.cc.rej

[Randy Smith (Not in Mondays), 2012-05-14 21:21:32]

Ben: Could you review my integration with DownloadUrlParams?

[benjhayden, 2012-05-15 13:55:53]

lgtm

[commit-bot: I haz the power, 2012-05-15 14:00:54]

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

[commit-bot: I haz the power, 2012-05-15 15:27:09]

Change committed as 137137