Treat files downloaded from the address bar as "always safe" (including extensions per discussion...

Treat files downloaded from the address bar as "always safe" (including extensions per discussion with asargent and the extensions folks).

This required plumbing the PageTransition::Type from render_view.cc back up through various layers to the download system, as well as adding an extra state qualifier bit on the Type to mark navigations triggered "FROM_ADDRESS_BAR" (since the Type itself sans-qualifier cannot be used to reliably check this).

This also fixes an inconsistency in IsDangerousFile() where "auto-open" lowered our safety checks for Dangerous files but not for AllowOnUserGesture files.

BUG=87192, 92345
TEST=Paste the PDF link from bug 87192 comment 0 into your address bar and hit enter.  The file should download without triggering any warning UI in the download shelf.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98897

[Peter Kasting, 2011-08-17 23:08:35]

cevans: chrome_download_manager_delegate.cc
rdsmith: All other download system changes
asargent: chrome/browser/extensions; IsDangerousFile() changes FYI
brettw: browser_navigator.cc, page_transition_types.h
sky: chrome/browser/ui/*/location_bar*, page_transition_types.h
darin: content/browser/renderer_host, content/common/resource_*, render_view.cc,
webkit/glue

[Peter Kasting, 2011-08-17 23:10:42]

Also brettw/darin, inform me how to proceed w.r.t. check_deps failing on
webkit/glue #including "content/common/page_transition_types.h"

[sky, 2011-08-18 00:13:00]

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:370: bool user_initiated =
params->transition & PageTransition::FROM_ADDRESS_BAR ||
How unlike you not to use () around all these.

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:374: base_transition ==
PageTransition::START_PAGE ||
How come you're adding START_PAGE here?

[asargent_no_longer_on_chrome, 2011-08-18 00:13:21]

LGTM on chrome/browser/extensions/user_script_listener_unittest.cc

mpcomplete - can you look at the change to 
chrome/browser/extensions/extension_webnavigation_api.cc ?

[Matt Perry, 2011-08-18 00:22:40]

http://codereview.chromium.org/7624031/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/7624031/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:112:
qualifiers->Append(Value::CreateStringValue("from_address_bar"));
This changes the API. It seems like a reasonable change, but can you add it to
the API definition here:
http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/api...

[Peter Kasting, 2011-08-18 00:53:38]

http://codereview.chromium.org/7624031/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/7624031/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webnavigation_api.cc:112:
qualifiers->Append(Value::CreateStringValue("from_address_bar"));
On 2011/08/18 00:22:40, Matt Perry wrote:
> This changes the API. It seems like a reasonable change, but can you add it to
> the API definition here:
>
http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/api...

Sure, will be in next revision.

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:374: base_transition ==
PageTransition::START_PAGE ||
On 2011/08/18 00:13:00, sky wrote:
> How come you're adding START_PAGE here?

It seemed like starting the browser ought to qualify as something the user
intentionally did, and the start page shouldn't be DDoS restricted.  But maybe
this is inappropriate.  You're right that this bends the definition a bit.  I'm
not sure which way to go.

[brettw, 2011-08-18 02:48:11]

Yeah, you definitely shouldn't be including stuff from content at this layer...

Reading the comment on the extra data class you're modifying, it talks about
"the RenderView uses this to store the data...". This seems like a big red flag
/ clue that maybe this class should be moved to content/renderer, or we can have
a way for the renderer to implement a subclass of it that adds the stuff it
needs. Does that look possible? (I didn't actually check)

[darin (slow to review), 2011-08-18 04:21:01]

http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:33: #include
"content/common/page_transition_types.h"
i see...

hmm, it certainly seems a bit wrong for this layer to need to know anything
about whether the request was initiated by typing in the omnibox.  we probably
do want to think about some way to push this through transparently. 
hmmmmmmmm...

[Peter Kasting, 2011-08-18 19:47:36]

http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:33: #include
"content/common/page_transition_types.h"
On 2011/08/18 04:21:01, darin wrote:
> i see...
> 
> hmm, it certainly seems a bit wrong for this layer to need to know anything
> about whether the request was initiated by typing in the omnibox.  we probably
> do want to think about some way to push this through transparently. 
> hmmmmmmmm...

I'm happy to do the gruntwork of whatever the right change is, but as I'm
touching classes here that I know nothing about, I'm not sure how to figure out
what should actually be done :).  Perhaps either you can think of something, or
direct me to someone with whom I should discuss this stuff?

[darin (slow to review), 2011-08-18 20:18:55]

On Thu, Aug 18, 2011 at 12:47 PM, <pkasting@chromium.org> wrote:

>
> http://codereview.chromium.**org/7624031/diff/1/webkit/**
>
glue/resource_loader_bridge.h<http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bridge.h>
> File webkit/glue/resource_loader_**bridge.h (right):
>
> http://codereview.chromium.**org/7624031/diff/1/webkit/**
>
glue/resource_loader_bridge.h#**newcode33<http://codereview.chromium.org/7624031/diff/1/webkit/glue/resource_loader_bridge.h#newcode33>
> webkit/glue/resource_loader_**bridge.h:33: #include
> "content/common/page_**transition_types.h"
> On 2011/08/18 04:21:01, darin wrote:
>
>> i see...
>>
>
>  hmm, it certainly seems a bit wrong for this layer to need to know
>>
> anything
>
>> about whether the request was initiated by typing in the omnibox.  we
>>
> probably
>
>> do want to think about some way to push this through transparently.
>> hmmmmmmmm...
>>
>
> I'm happy to do the gruntwork of whatever the right change is, but as
> I'm touching classes here that I know nothing about, I'm not sure how to
> figure out what should actually be done :).  Perhaps either you can
> think of something, or direct me to someone with whom I should discuss
> this stuff?
>
>
One solution would be to plumb a 'user_typed' boolean instead.  However,
even that seems to be a bit of a layering violation.

Another idea might be to pass the extraData associated with
the WebURLRequest
through to the ResourceLoaderBridge implementation.  At least, then you
would
end back in code that lives in content/, where you would be able to use the
PageTransition data type.  I'd probably try to do this.

-Darin



>
>
http://codereview.chromium.**org/7624031/<http://codereview.chromium.org/7624...
>

[Randy Smith (Not in Mondays), 2011-08-19 15:03:51]

Andy, could you take a look at this CL from the downloads perspective?  It may
end up being a complex one--feel free to consult with me or ask me for a second
opinion.  But I suspect it's in an area that you have some familiarity with. 
Thanks!

[ahendrickson, 2011-08-19 15:51:47]

LGTM with respect to downloads.

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

http://codereview.chromium.org/7624031/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:150:
&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone));
Nit:  I think the indentation is wrong here.

[Peter Kasting, 2011-08-19 17:27:37]

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

http://codereview.chromium.org/7624031/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:150:
&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone));
On 2011/08/19 15:51:47, ahendrickson wrote:
> Nit:  I think the indentation is wrong here.

I'll indent the last line 4 more.  I asked Brett what the right style is and it
sounded like sometimes he does one and sometimes the other, but indenting more
if you can is legal, so I'll do it (I think it looks better here, just wasn't
sure it was allowed).

[sky, 2011-08-19 17:34:34]

chrome/browser/ui/*/location_bar*, page_transition_types.h LGTM

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7624031/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:374: base_transition ==
PageTransition::START_PAGE ||
On 2011/08/18 00:53:39, Peter Kasting wrote:
> On 2011/08/18 00:13:00, sky wrote:
> > How come you're adding START_PAGE here?
> 
> It seemed like starting the browser ought to qualify as something the user
> intentionally did, and the start page shouldn't be DDoS restricted.  But maybe
> this is inappropriate.  You're right that this bends the definition a bit. 
I'm
> not sure which way to go.

Peter stopped by and we talked about START_PAGE. Since START_PAGE is typically
used when the user actually launches the app it seems fine to make it user
initiated.

[Peter Kasting, 2011-08-26 18:51:53]

New snap up.  Still need review:

cevans: chrome_download_manager_delegate.cc
mpcomplete: chrome/common/extensions/{api,docs} changes
brettw: browser_navigator.cc, page_transition_types.h
darin: content/browser/renderer_host, content/common/{request,resource}*,
render_view.cc

[Matt Perry, 2011-08-26 18:56:42]

lgtm

[brettw, 2011-08-26 19:26:41]

browser_navigator & page_transition_types LGTM

[darin (slow to review), 2011-08-29 19:56:40]

LGTM

[Chris Evans, 2011-08-30 23:49:05]

On 2011/08/29 19:56:40, darin wrote:
> LGTM

LGTM. Sorry for the holdup.