Change policy to not trigger dangerous download UI for web intents dispatched files.

Change policy to not trigger dangerous download UI for web intents dispatched files.

R=asanka@chromium.org
BUG=140845
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155443

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

I don't understand why you're adding a WEB_INTENTS entry to danger type (I also
don't want you to do it, but I'll get to that after I understand why :-}).  It
looks like you just set it, and never test it.  

My image of what you were going to do is that you'd change the file name at file
name determination if you were going to handle something with web intents, and
you'd rely on that "should we handle with web intents?" probe to grab that file
at open time.  Can you give me a sense of what I'm missing?

[Greg Billock, 2012-08-09 17:56:42]

On Thu, Aug 9, 2012 at 10:42 AM,  <rdsmith@chromium.org> wrote:
> I don't understand why you're adding a WEB_INTENTS entry to danger type (I
> also
> don't want you to do it, but I'll get to that after I understand why :-}).
> It
> looks like you just set it, and never test it.

Yes. :-) This is a bit awkward. I originally just set it to
NOT_DANGEROUS, but that obviously loses information, so I made a new
enum value. I'd be happy to kill it off.

This works for a couple black-magick-y reasons. First is that the
GetIntermediatePath function will then provision an alternate filename
for dangerous files -- that ends up working without alteration if we
use the new enum type. The second is that the details of how the
download item computes its dangerousness function will place new enum
types in the "not dangerous" bucket. So there's no change required
there either. I agree that it ends up looking very odd in this change,
but the result is what we want, I think. (I have only run it on Linux,
but that used to pop the warning, and no longer does, so I believe it
will work on all platforms.)

Changing to plain old NOT_DANGEROUS will require some filename manipulation.

>
> My image of what you were going to do is that you'd change the file name at
> file
> name determination if you were going to handle something with web intents,
> and
> you'd rely on that "should we handle with web intents?" probe to grab that
> file
> at open time.  Can you give me a sense of what I'm missing?

Yes, I want to do this, but I can't figure out where. You see me
manipulating the filename around line 400 (in the delegate) to account
for what I wanted to do, but I can't figure out where to do it. Is the
final filename set up before the download system kicks in? If so, can
I retroactively change it?

I also still need to add the callback to the dispatcher to delete the
file upon return, but I wanted to send this off before I left to see
what you thought.


>
>
> http://codereview.chromium.org/10836164/

[Randy Smith (Not in Mondays), 2012-08-09 21:09:11]

On 2012/08/09 17:56:42, Greg Billock wrote:
> On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org> wrote:
> > I don't understand why you're adding a WEB_INTENTS entry to danger type (I
> > also
> > don't want you to do it, but I'll get to that after I understand why :-}).
> > It
> > looks like you just set it, and never test it.
> 
> Yes. :-) This is a bit awkward. I originally just set it to
> NOT_DANGEROUS, but that obviously loses information, so I made a new
> enum value. I'd be happy to kill it off.
> 
> This works for a couple black-magick-y reasons. First is that the
> GetIntermediatePath function will then provision an alternate filename
> for dangerous files -- that ends up working without alteration if we
> use the new enum type. The second is that the details of how the
> download item computes its dangerousness function will place new enum
> types in the "not dangerous" bucket. So there's no change required
> there either. I agree that it ends up looking very odd in this change,
> but the result is what we want, I think. (I have only run it on Linux,
> but that used to pop the warning, and no longer does, so I believe it
> will work on all platforms.)
> 
> Changing to plain old NOT_DANGEROUS will require some filename manipulation.
> 
> >
> > My image of what you were going to do is that you'd change the file name at
> > file
> > name determination if you were going to handle something with web intents,
> > and
> > you'd rely on that "should we handle with web intents?" probe to grab that
> > file
> > at open time.  Can you give me a sense of what I'm missing?
> 
> Yes, I want to do this, but I can't figure out where. You see me
> manipulating the filename around line 400 (in the delegate) to account
> for what I wanted to do, but I can't figure out where to do it. Is the
> final filename set up before the download system kicks in? If so, can
> I retroactively change it?

So Asanka's the right person to evaluate the details here (and I've added him to
the CL so he can comment) but I can sketch out the big picture and point to
where in that picture you want to go.

When we start a download, we start downloading to a temporary file with a random
name.  Then we call the function
ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function is
responsible for a) Determining the final filename that we want to use for the
download, b) uniquifying that name c) optionally prompting the user for the name
if that makes sense,  d) Figuring out the intermediate name for the download,
and e) figuring out the basic danger status for the download.  What I believe
you in the case of web intents is:  a) To unilaterally pick a name that's
specific to web intents, b) Uniquify that name, c) Not prompt the user, d) Keep
the intermediate name logic, and e) Mark the download as not dangerous.  

DetermineDownloadTarget starts a large cascade of calls (DetermineDownloadTarget
-> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
OnPathReservationAvailable -> OnTargetPathDetermined).  I think the right place
for you to put your stuff is in CheckVisitedReferrerBeforeDone; that allows you
to null out prompting, reset danger type, and specify the path you want.

However, there are some subtle questions we might want to answer first:
* Do you really want to completely ignore the danger signals?  Dangerous file
types are fine to ignore (you'll be setting your own file type) but I'm not
clear that dangerous URLs as reported by Safe Browsing are good to ignore, nor
that you should ignore the binary evaluations.  So maybe this means picking a
new name before we do the dangerous file type evaluation, but leaving everything
else to work so we do catch dangerous binaries or URLs?

* Avoiding prompting makes sense for the moment (because you're going to
unilaterally change the file name--why ask the user for input if you're going to
ignore it?) but we're going to want to prompt in save page as ... situations. 
But that's probably going to be by disabling web intents, so it's all good.

Asanka, anything else I've missed?

[asanka, 2012-08-10 06:04:14]

On 2012/08/09 21:09:11, rdsmith wrote:
> On 2012/08/09 17:56:42, Greg Billock wrote:
> > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org> wrote:
> > > I don't understand why you're adding a WEB_INTENTS entry to danger type (I
> > > also
> > > don't want you to do it, but I'll get to that after I understand why :-}).
> > > It
> > > looks like you just set it, and never test it.
> > 
> > Yes. :-) This is a bit awkward. I originally just set it to
> > NOT_DANGEROUS, but that obviously loses information, so I made a new
> > enum value. I'd be happy to kill it off.
> > 
> > This works for a couple black-magick-y reasons. First is that the
> > GetIntermediatePath function will then provision an alternate filename
> > for dangerous files -- that ends up working without alteration if we
> > use the new enum type. The second is that the details of how the
> > download item computes its dangerousness function will place new enum
> > types in the "not dangerous" bucket. So there's no change required
> > there either. I agree that it ends up looking very odd in this change,
> > but the result is what we want, I think. (I have only run it on Linux,
> > but that used to pop the warning, and no longer does, so I believe it
> > will work on all platforms.)
> > 
> > Changing to plain old NOT_DANGEROUS will require some filename manipulation.
> > 
> > >
> > > My image of what you were going to do is that you'd change the file name
at
> > > file
> > > name determination if you were going to handle something with web intents,
> > > and
> > > you'd rely on that "should we handle with web intents?" probe to grab that
> > > file
> > > at open time.  Can you give me a sense of what I'm missing?
> > 
> > Yes, I want to do this, but I can't figure out where. You see me
> > manipulating the filename around line 400 (in the delegate) to account
> > for what I wanted to do, but I can't figure out where to do it. Is the
> > final filename set up before the download system kicks in? If so, can
> > I retroactively change it?
> 
> So Asanka's the right person to evaluate the details here (and I've added him
to
> the CL so he can comment) but I can sketch out the big picture and point to
> where in that picture you want to go.
> 
> When we start a download, we start downloading to a temporary file with a
random
> name.  Then we call the function
> ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function is
> responsible for a) Determining the final filename that we want to use for the
> download, b) uniquifying that name c) optionally prompting the user for the
name
> if that makes sense,  d) Figuring out the intermediate name for the download,
> and e) figuring out the basic danger status for the download.  What I believe
> you in the case of web intents is:  a) To unilaterally pick a name that's
> specific to web intents, b) Uniquify that name, c) Not prompt the user, d)
Keep
> the intermediate name logic, and e) Mark the download as not dangerous.  
> 
> DetermineDownloadTarget starts a large cascade of calls
(DetermineDownloadTarget
> -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
> OnPathReservationAvailable -> OnTargetPathDetermined).  I think the right
place
> for you to put your stuff is in CheckVisitedReferrerBeforeDone; that allows
you
> to null out prompting, reset danger type, and specify the path you want.

I'd suggest not overriding the danger type. Picking a filename that is specific
to web intents at CheckVisitedReferrerBeforeDone before the IsDangerousFile()
check will cause the file to be considered safe (for the right reasons). If the
download is considered dangerous because of the URL, then I think we should
allow the warning.

> However, there are some subtle questions we might want to answer first:
> * Do you really want to completely ignore the danger signals?  Dangerous file
> types are fine to ignore (you'll be setting your own file type) but I'm not
> clear that dangerous URLs as reported by Safe Browsing are good to ignore, nor
> that you should ignore the binary evaluations.  So maybe this means picking a
> new name before we do the dangerous file type evaluation, but leaving
everything
> else to work so we do catch dangerous binaries or URLs?
> 
> * Avoiding prompting makes sense for the moment (because you're going to
> unilaterally change the file name--why ask the user for input if you're going
to
> ignore it?) but we're going to want to prompt in save page as ... situations. 
> But that's probably going to be by disabling web intents, so it's all good.

We prompt if the user has "always prompt for download location" preference set
unless the download is an extension or of a filetype that we are going to open.
I think it should also check if the download is going to be handled by web
intents here (this is also in CheckVisitedReferrerBeforeDone() at the
'if(download_prefs_->PromptForDownload())' check).

I'm not sure whether you should suppress prompting beyond that if "Save as"
downloads are going to not be handled by web intents.

> Asanka, anything else I've missed?

What about downloads with forced file paths? These could be programmatic
downloads or a drag and drop.

[Randy Smith (Not in Mondays), 2012-08-10 16:15:20]

On 2012/08/10 06:04:14, asanka wrote:
> On 2012/08/09 21:09:11, rdsmith wrote:
> > On 2012/08/09 17:56:42, Greg Billock wrote:
> > > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org> wrote:
> > > > I don't understand why you're adding a WEB_INTENTS entry to danger type
(I
> > > > also
> > > > don't want you to do it, but I'll get to that after I understand why
:-}).
> > > > It
> > > > looks like you just set it, and never test it.
> > > 
> > > Yes. :-) This is a bit awkward. I originally just set it to
> > > NOT_DANGEROUS, but that obviously loses information, so I made a new
> > > enum value. I'd be happy to kill it off.
> > > 
> > > This works for a couple black-magick-y reasons. First is that the
> > > GetIntermediatePath function will then provision an alternate filename
> > > for dangerous files -- that ends up working without alteration if we
> > > use the new enum type. The second is that the details of how the
> > > download item computes its dangerousness function will place new enum
> > > types in the "not dangerous" bucket. So there's no change required
> > > there either. I agree that it ends up looking very odd in this change,
> > > but the result is what we want, I think. (I have only run it on Linux,
> > > but that used to pop the warning, and no longer does, so I believe it
> > > will work on all platforms.)
> > > 
> > > Changing to plain old NOT_DANGEROUS will require some filename
manipulation.
> > > 
> > > >
> > > > My image of what you were going to do is that you'd change the file name
> at
> > > > file
> > > > name determination if you were going to handle something with web
intents,
> > > > and
> > > > you'd rely on that "should we handle with web intents?" probe to grab
that
> > > > file
> > > > at open time.  Can you give me a sense of what I'm missing?
> > > 
> > > Yes, I want to do this, but I can't figure out where. You see me
> > > manipulating the filename around line 400 (in the delegate) to account
> > > for what I wanted to do, but I can't figure out where to do it. Is the
> > > final filename set up before the download system kicks in? If so, can
> > > I retroactively change it?
> > 
> > So Asanka's the right person to evaluate the details here (and I've added
him
> to
> > the CL so he can comment) but I can sketch out the big picture and point to
> > where in that picture you want to go.
> > 
> > When we start a download, we start downloading to a temporary file with a
> random
> > name.  Then we call the function
> > ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function is
> > responsible for a) Determining the final filename that we want to use for
the
> > download, b) uniquifying that name c) optionally prompting the user for the
> name
> > if that makes sense,  d) Figuring out the intermediate name for the
download,
> > and e) figuring out the basic danger status for the download.  What I
believe
> > you in the case of web intents is:  a) To unilaterally pick a name that's
> > specific to web intents, b) Uniquify that name, c) Not prompt the user, d)
> Keep
> > the intermediate name logic, and e) Mark the download as not dangerous.  
> > 
> > DetermineDownloadTarget starts a large cascade of calls
> (DetermineDownloadTarget
> > -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
> > OnPathReservationAvailable -> OnTargetPathDetermined).  I think the right
> place
> > for you to put your stuff is in CheckVisitedReferrerBeforeDone; that allows
> you
> > to null out prompting, reset danger type, and specify the path you want.
> 
> I'd suggest not overriding the danger type. Picking a filename that is
specific
> to web intents at CheckVisitedReferrerBeforeDone before the IsDangerousFile()
> check will cause the file to be considered safe (for the right reasons). If
the
> download is considered dangerous because of the URL, then I think we should
> allow the warning.
> 
> > However, there are some subtle questions we might want to answer first:
> > * Do you really want to completely ignore the danger signals?  Dangerous
file
> > types are fine to ignore (you'll be setting your own file type) but I'm not
> > clear that dangerous URLs as reported by Safe Browsing are good to ignore,
nor
> > that you should ignore the binary evaluations.  So maybe this means picking
a
> > new name before we do the dangerous file type evaluation, but leaving
> everything
> > else to work so we do catch dangerous binaries or URLs?
> > 
> > * Avoiding prompting makes sense for the moment (because you're going to
> > unilaterally change the file name--why ask the user for input if you're
going
> to
> > ignore it?) but we're going to want to prompt in save page as ...
situations. 
> > But that's probably going to be by disabling web intents, so it's all good.
> 
> We prompt if the user has "always prompt for download location" preference set
> unless the download is an extension or of a filetype that we are going to
open.
> I think it should also check if the download is going to be handled by web
> intents here (this is also in CheckVisitedReferrerBeforeDone() at the
> 'if(download_prefs_->PromptForDownload())' check).
> 
> I'm not sure whether you should suppress prompting beyond that if "Save as"
> downloads are going to not be handled by web intents.
> 
> > Asanka, anything else I've missed?
> 
> What about downloads with forced file paths? These could be programmatic
> downloads or a drag and drop.

Agreed with all your points.  I'd be inclined to not allow web intents for
forced file paths; those are pretty specific (d&d or chrome internal) and I
don't think we should intercept them.

[Greg Billock, 2012-08-10 18:08:44]

This all sounds good. Preparing second patch now. In particular, my goals are:

1. If the download is dangerous because of something legit (safe
browsing) then maintain the warning
2. Make the new filename suffixed with ".webintents" when we'll use it that way
3. Avoid the dangerous warning in other circumstances.
4. Delete the file when we're done with it.

On Fri, Aug 10, 2012 at 9:15 AM,  <rdsmith@chromium.org> wrote:
> On 2012/08/10 06:04:14, asanka wrote:
>>
>> On 2012/08/09 21:09:11, rdsmith wrote:
>> > On 2012/08/09 17:56:42, Greg Billock wrote:
>> > > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org> wrote:
>> > > > I don't understand why you're adding a WEB_INTENTS entry to danger
>> > > > type
>
> (I
>>
>> > > > also
>> > > > don't want you to do it, but I'll get to that after I understand why
>
> :-}).
>>
>> > > > It
>> > > > looks like you just set it, and never test it.
>> > >
>> > > Yes. :-) This is a bit awkward. I originally just set it to
>> > > NOT_DANGEROUS, but that obviously loses information, so I made a new
>> > > enum value. I'd be happy to kill it off.
>> > >
>> > > This works for a couple black-magick-y reasons. First is that the
>> > > GetIntermediatePath function will then provision an alternate filename
>> > > for dangerous files -- that ends up working without alteration if we
>> > > use the new enum type. The second is that the details of how the
>> > > download item computes its dangerousness function will place new enum
>> > > types in the "not dangerous" bucket. So there's no change required
>> > > there either. I agree that it ends up looking very odd in this change,
>> > > but the result is what we want, I think. (I have only run it on Linux,
>> > > but that used to pop the warning, and no longer does, so I believe it
>> > > will work on all platforms.)
>> > >
>> > > Changing to plain old NOT_DANGEROUS will require some filename
>
> manipulation.
>>
>> > >
>> > > >
>> > > > My image of what you were going to do is that you'd change the file
>> > > > name
>> at
>> > > > file
>> > > > name determination if you were going to handle something with web
>
> intents,
>>
>> > > > and
>> > > > you'd rely on that "should we handle with web intents?" probe to
>> > > > grab
>
> that
>>
>> > > > file
>> > > > at open time.  Can you give me a sense of what I'm missing?
>> > >
>> > > Yes, I want to do this, but I can't figure out where. You see me
>> > > manipulating the filename around line 400 (in the delegate) to account
>> > > for what I wanted to do, but I can't figure out where to do it. Is the
>> > > final filename set up before the download system kicks in? If so, can
>> > > I retroactively change it?
>> >
>> > So Asanka's the right person to evaluate the details here (and I've
>> > added
>
> him
>>
>> to
>> > the CL so he can comment) but I can sketch out the big picture and point
>> > to
>> > where in that picture you want to go.
>> >
>> > When we start a download, we start downloading to a temporary file with
>> > a
>> random
>> > name.  Then we call the function
>> > ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function
>> > is
>> > responsible for a) Determining the final filename that we want to use
>> > for
>
> the
>>
>> > download, b) uniquifying that name c) optionally prompting the user for
>> > the
>> name
>> > if that makes sense,  d) Figuring out the intermediate name for the
>
> download,
>>
>> > and e) figuring out the basic danger status for the download.  What I
>
> believe
>>
>> > you in the case of web intents is:  a) To unilaterally pick a name
>> > that's
>> > specific to web intents, b) Uniquify that name, c) Not prompt the user,
>> > d)
>> Keep
>> > the intermediate name logic, and e) Mark the download as not dangerous.
>> >
>> > DetermineDownloadTarget starts a large cascade of calls
>> (DetermineDownloadTarget
>> > -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
>> > OnPathReservationAvailable -> OnTargetPathDetermined).  I think the
>> > right
>> place
>> > for you to put your stuff is in CheckVisitedReferrerBeforeDone; that
>> > allows
>> you
>> > to null out prompting, reset danger type, and specify the path you want.
>
>
>> I'd suggest not overriding the danger type. Picking a filename that is
>
> specific
>>
>> to web intents at CheckVisitedReferrerBeforeDone before the
>> IsDangerousFile()
>> check will cause the file to be considered safe (for the right reasons).
>> If
>
> the
>>
>> download is considered dangerous because of the URL, then I think we
>> should
>> allow the warning.
>
>
>> > However, there are some subtle questions we might want to answer first:
>> > * Do you really want to completely ignore the danger signals?  Dangerous
>
> file
>>
>> > types are fine to ignore (you'll be setting your own file type) but I'm
>> > not
>> > clear that dangerous URLs as reported by Safe Browsing are good to
>> > ignore,
>
> nor
>>
>> > that you should ignore the binary evaluations.  So maybe this means
>> > picking
>
> a
>>
>> > new name before we do the dangerous file type evaluation, but leaving
>> everything
>> > else to work so we do catch dangerous binaries or URLs?
>> >
>> > * Avoiding prompting makes sense for the moment (because you're going to
>> > unilaterally change the file name--why ask the user for input if you're
>
> going
>>
>> to
>> > ignore it?) but we're going to want to prompt in save page as ...
>
> situations.
>>
>> > But that's probably going to be by disabling web intents, so it's all
>> > good.
>
>
>> We prompt if the user has "always prompt for download location" preference
>> set
>> unless the download is an extension or of a filetype that we are going to
>
> open.
>>
>> I think it should also check if the download is going to be handled by web
>> intents here (this is also in CheckVisitedReferrerBeforeDone() at the
>> 'if(download_prefs_->PromptForDownload())' check).
>
>
>> I'm not sure whether you should suppress prompting beyond that if "Save
>> as"
>> downloads are going to not be handled by web intents.
>
>
>> > Asanka, anything else I've missed?
>
>
>> What about downloads with forced file paths? These could be programmatic
>> downloads or a drag and drop.
>
>
> Agreed with all your points.  I'd be inclined to not allow web intents for
> forced file paths; those are pretty specific (d&d or chrome internal) and I
> don't think we should intercept them.
>
> https://chromiumcodereview.appspot.com/10836164/

[Greg Billock, 2012-08-16 01:10:07]

On 2012/08/10 18:08:44, Greg Billock wrote:
> This all sounds good. Preparing second patch now. In particular, my goals are:
> 
> 1. If the download is dangerous because of something legit (safe
> browsing) then maintain the warning
> 2. Make the new filename suffixed with ".webintents" when we'll use it that
way
> 3. Avoid the dangerous warning in other circumstances.
> 4. Delete the file when we're done with it.
> 
> On Fri, Aug 10, 2012 at 9:15 AM,  <mailto:rdsmith@chromium.org> wrote:
> > On 2012/08/10 06:04:14, asanka wrote:
> >>
> >> On 2012/08/09 21:09:11, rdsmith wrote:
> >> > On 2012/08/09 17:56:42, Greg Billock wrote:
> >> > > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org> wrote:
> >> > > > I don't understand why you're adding a WEB_INTENTS entry to danger
> >> > > > type
> >
> > (I
> >>
> >> > > > also
> >> > > > don't want you to do it, but I'll get to that after I understand why
> >
> > :-}).
> >>
> >> > > > It
> >> > > > looks like you just set it, and never test it.
> >> > >
> >> > > Yes. :-) This is a bit awkward. I originally just set it to
> >> > > NOT_DANGEROUS, but that obviously loses information, so I made a new
> >> > > enum value. I'd be happy to kill it off.
> >> > >
> >> > > This works for a couple black-magick-y reasons. First is that the
> >> > > GetIntermediatePath function will then provision an alternate filename
> >> > > for dangerous files -- that ends up working without alteration if we
> >> > > use the new enum type. The second is that the details of how the
> >> > > download item computes its dangerousness function will place new enum
> >> > > types in the "not dangerous" bucket. So there's no change required
> >> > > there either. I agree that it ends up looking very odd in this change,
> >> > > but the result is what we want, I think. (I have only run it on Linux,
> >> > > but that used to pop the warning, and no longer does, so I believe it
> >> > > will work on all platforms.)
> >> > >
> >> > > Changing to plain old NOT_DANGEROUS will require some filename
> >
> > manipulation.
> >>
> >> > >
> >> > > >
> >> > > > My image of what you were going to do is that you'd change the file
> >> > > > name
> >> at
> >> > > > file
> >> > > > name determination if you were going to handle something with web
> >
> > intents,
> >>
> >> > > > and
> >> > > > you'd rely on that "should we handle with web intents?" probe to
> >> > > > grab
> >
> > that
> >>
> >> > > > file
> >> > > > at open time.  Can you give me a sense of what I'm missing?
> >> > >
> >> > > Yes, I want to do this, but I can't figure out where. You see me
> >> > > manipulating the filename around line 400 (in the delegate) to account
> >> > > for what I wanted to do, but I can't figure out where to do it. Is the
> >> > > final filename set up before the download system kicks in? If so, can
> >> > > I retroactively change it?
> >> >
> >> > So Asanka's the right person to evaluate the details here (and I've
> >> > added
> >
> > him
> >>
> >> to
> >> > the CL so he can comment) but I can sketch out the big picture and point
> >> > to
> >> > where in that picture you want to go.
> >> >
> >> > When we start a download, we start downloading to a temporary file with
> >> > a
> >> random
> >> > name.  Then we call the function
> >> > ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function
> >> > is
> >> > responsible for a) Determining the final filename that we want to use
> >> > for
> >
> > the
> >>
> >> > download, b) uniquifying that name c) optionally prompting the user for
> >> > the
> >> name
> >> > if that makes sense,  d) Figuring out the intermediate name for the
> >
> > download,
> >>
> >> > and e) figuring out the basic danger status for the download.  What I
> >
> > believe
> >>
> >> > you in the case of web intents is:  a) To unilaterally pick a name
> >> > that's
> >> > specific to web intents, b) Uniquify that name, c) Not prompt the user,
> >> > d)
> >> Keep
> >> > the intermediate name logic, and e) Mark the download as not dangerous.
> >> >
> >> > DetermineDownloadTarget starts a large cascade of calls
> >> (DetermineDownloadTarget
> >> > -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
> >> > OnPathReservationAvailable -> OnTargetPathDetermined).  I think the
> >> > right
> >> place
> >> > for you to put your stuff is in CheckVisitedReferrerBeforeDone; that
> >> > allows
> >> you
> >> > to null out prompting, reset danger type, and specify the path you want.
> >
> >
> >> I'd suggest not overriding the danger type. Picking a filename that is
> >
> > specific
> >>
> >> to web intents at CheckVisitedReferrerBeforeDone before the
> >> IsDangerousFile()
> >> check will cause the file to be considered safe (for the right reasons).
> >> If
> >
> > the
> >>
> >> download is considered dangerous because of the URL, then I think we
> >> should
> >> allow the warning.
> >
> >
> >> > However, there are some subtle questions we might want to answer first:
> >> > * Do you really want to completely ignore the danger signals?  Dangerous
> >
> > file
> >>
> >> > types are fine to ignore (you'll be setting your own file type) but I'm
> >> > not
> >> > clear that dangerous URLs as reported by Safe Browsing are good to
> >> > ignore,
> >
> > nor
> >>
> >> > that you should ignore the binary evaluations.  So maybe this means
> >> > picking
> >
> > a
> >>
> >> > new name before we do the dangerous file type evaluation, but leaving
> >> everything
> >> > else to work so we do catch dangerous binaries or URLs?
> >> >
> >> > * Avoiding prompting makes sense for the moment (because you're going to
> >> > unilaterally change the file name--why ask the user for input if you're
> >
> > going
> >>
> >> to
> >> > ignore it?) but we're going to want to prompt in save page as ...
> >
> > situations.
> >>
> >> > But that's probably going to be by disabling web intents, so it's all
> >> > good.
> >
> >
> >> We prompt if the user has "always prompt for download location" preference
> >> set
> >> unless the download is an extension or of a filetype that we are going to
> >
> > open.
> >>
> >> I think it should also check if the download is going to be handled by web
> >> intents here (this is also in CheckVisitedReferrerBeforeDone() at the
> >> 'if(download_prefs_->PromptForDownload())' check).
> >
> >
> >> I'm not sure whether you should suppress prompting beyond that if "Save
> >> as"
> >> downloads are going to not be handled by web intents.
> >
> >
> >> > Asanka, anything else I've missed?
> >
> >
> >> What about downloads with forced file paths? These could be programmatic
> >> downloads or a drag and drop.
> >
> >
> > Agreed with all your points.  I'd be inclined to not allow web intents for
> > forced file paths; those are pretty specific (d&d or chrome internal) and I
> > don't think we should intercept them.
> >
> > https://chromiumcodereview.appspot.com/10836164/

ping. Any further thoughts on this approach?

[Randy Smith (Not in Mondays), 2012-08-16 13:34:26]

On 2012/08/16 01:10:07, Greg Billock wrote:
> On 2012/08/10 18:08:44, Greg Billock wrote:
> > This all sounds good. Preparing second patch now. In particular, my goals
are:
> > 
> > 1. If the download is dangerous because of something legit (safe
> > browsing) then maintain the warning
> > 2. Make the new filename suffixed with ".webintents" when we'll use it that
> way
> > 3. Avoid the dangerous warning in other circumstances.
> > 4. Delete the file when we're done with it.
> > 
> > On Fri, Aug 10, 2012 at 9:15 AM,  <mailto:rdsmith@chromium.org> wrote:
> > > On 2012/08/10 06:04:14, asanka wrote:
> > >>
> > >> On 2012/08/09 21:09:11, rdsmith wrote:
> > >> > On 2012/08/09 17:56:42, Greg Billock wrote:
> > >> > > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org>
wrote:
> > >> > > > I don't understand why you're adding a WEB_INTENTS entry to danger
> > >> > > > type
> > >
> > > (I
> > >>
> > >> > > > also
> > >> > > > don't want you to do it, but I'll get to that after I understand
why
> > >
> > > :-}).
> > >>
> > >> > > > It
> > >> > > > looks like you just set it, and never test it.
> > >> > >
> > >> > > Yes. :-) This is a bit awkward. I originally just set it to
> > >> > > NOT_DANGEROUS, but that obviously loses information, so I made a new
> > >> > > enum value. I'd be happy to kill it off.
> > >> > >
> > >> > > This works for a couple black-magick-y reasons. First is that the
> > >> > > GetIntermediatePath function will then provision an alternate
filename
> > >> > > for dangerous files -- that ends up working without alteration if we
> > >> > > use the new enum type. The second is that the details of how the
> > >> > > download item computes its dangerousness function will place new enum
> > >> > > types in the "not dangerous" bucket. So there's no change required
> > >> > > there either. I agree that it ends up looking very odd in this
change,
> > >> > > but the result is what we want, I think. (I have only run it on
Linux,
> > >> > > but that used to pop the warning, and no longer does, so I believe it
> > >> > > will work on all platforms.)
> > >> > >
> > >> > > Changing to plain old NOT_DANGEROUS will require some filename
> > >
> > > manipulation.
> > >>
> > >> > >
> > >> > > >
> > >> > > > My image of what you were going to do is that you'd change the file
> > >> > > > name
> > >> at
> > >> > > > file
> > >> > > > name determination if you were going to handle something with web
> > >
> > > intents,
> > >>
> > >> > > > and
> > >> > > > you'd rely on that "should we handle with web intents?" probe to
> > >> > > > grab
> > >
> > > that
> > >>
> > >> > > > file
> > >> > > > at open time.  Can you give me a sense of what I'm missing?
> > >> > >
> > >> > > Yes, I want to do this, but I can't figure out where. You see me
> > >> > > manipulating the filename around line 400 (in the delegate) to
account
> > >> > > for what I wanted to do, but I can't figure out where to do it. Is
the
> > >> > > final filename set up before the download system kicks in? If so, can
> > >> > > I retroactively change it?
> > >> >
> > >> > So Asanka's the right person to evaluate the details here (and I've
> > >> > added
> > >
> > > him
> > >>
> > >> to
> > >> > the CL so he can comment) but I can sketch out the big picture and
point
> > >> > to
> > >> > where in that picture you want to go.
> > >> >
> > >> > When we start a download, we start downloading to a temporary file with
> > >> > a
> > >> random
> > >> > name.  Then we call the function
> > >> > ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That function
> > >> > is
> > >> > responsible for a) Determining the final filename that we want to use
> > >> > for
> > >
> > > the
> > >>
> > >> > download, b) uniquifying that name c) optionally prompting the user for
> > >> > the
> > >> name
> > >> > if that makes sense,  d) Figuring out the intermediate name for the
> > >
> > > download,
> > >>
> > >> > and e) figuring out the basic danger status for the download.  What I
> > >
> > > believe
> > >>
> > >> > you in the case of web intents is:  a) To unilaterally pick a name
> > >> > that's
> > >> > specific to web intents, b) Uniquify that name, c) Not prompt the user,
> > >> > d)
> > >> Keep
> > >> > the intermediate name logic, and e) Mark the download as not dangerous.
> > >> >
> > >> > DetermineDownloadTarget starts a large cascade of calls
> > >> (DetermineDownloadTarget
> > >> > -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
> > >> > OnPathReservationAvailable -> OnTargetPathDetermined).  I think the
> > >> > right
> > >> place
> > >> > for you to put your stuff is in CheckVisitedReferrerBeforeDone; that
> > >> > allows
> > >> you
> > >> > to null out prompting, reset danger type, and specify the path you
want.
> > >
> > >
> > >> I'd suggest not overriding the danger type. Picking a filename that is
> > >
> > > specific
> > >>
> > >> to web intents at CheckVisitedReferrerBeforeDone before the
> > >> IsDangerousFile()
> > >> check will cause the file to be considered safe (for the right reasons).
> > >> If
> > >
> > > the
> > >>
> > >> download is considered dangerous because of the URL, then I think we
> > >> should
> > >> allow the warning.
> > >
> > >
> > >> > However, there are some subtle questions we might want to answer first:
> > >> > * Do you really want to completely ignore the danger signals? 
Dangerous
> > >
> > > file
> > >>
> > >> > types are fine to ignore (you'll be setting your own file type) but I'm
> > >> > not
> > >> > clear that dangerous URLs as reported by Safe Browsing are good to
> > >> > ignore,
> > >
> > > nor
> > >>
> > >> > that you should ignore the binary evaluations.  So maybe this means
> > >> > picking
> > >
> > > a
> > >>
> > >> > new name before we do the dangerous file type evaluation, but leaving
> > >> everything
> > >> > else to work so we do catch dangerous binaries or URLs?
> > >> >
> > >> > * Avoiding prompting makes sense for the moment (because you're going
to
> > >> > unilaterally change the file name--why ask the user for input if you're
> > >
> > > going
> > >>
> > >> to
> > >> > ignore it?) but we're going to want to prompt in save page as ...
> > >
> > > situations.
> > >>
> > >> > But that's probably going to be by disabling web intents, so it's all
> > >> > good.
> > >
> > >
> > >> We prompt if the user has "always prompt for download location"
preference
> > >> set
> > >> unless the download is an extension or of a filetype that we are going to
> > >
> > > open.
> > >>
> > >> I think it should also check if the download is going to be handled by
web
> > >> intents here (this is also in CheckVisitedReferrerBeforeDone() at the
> > >> 'if(download_prefs_->PromptForDownload())' check).
> > >
> > >
> > >> I'm not sure whether you should suppress prompting beyond that if "Save
> > >> as"
> > >> downloads are going to not be handled by web intents.
> > >
> > >
> > >> > Asanka, anything else I've missed?
> > >
> > >
> > >> What about downloads with forced file paths? These could be programmatic
> > >> downloads or a drag and drop.
> > >
> > >
> > > Agreed with all your points.  I'd be inclined to not allow web intents for
> > > forced file paths; those are pretty specific (d&d or chrome internal) and
I
> > > don't think we should intercept them.
> > >
> > > https://chromiumcodereview.appspot.com/10836164/
> 
> ping. Any further thoughts on this approach?

The approach sounds good.  I'll let Asanka do the actual code review (which I
think you're asking for?).

[Greg Billock, 2012-08-16 14:49:08]

Sounds good. Took you off R=

On Thu, Aug 16, 2012 at 6:34 AM,  <rdsmith@chromium.org> wrote:
> On 2012/08/16 01:10:07, Greg Billock wrote:
>>
>> On 2012/08/10 18:08:44, Greg Billock wrote:
>> > This all sounds good. Preparing second patch now. In particular, my
>> > goals
>
> are:
>>
>> >
>> > 1. If the download is dangerous because of something legit (safe
>> > browsing) then maintain the warning
>> > 2. Make the new filename suffixed with ".webintents" when we'll use it
>> > that
>> way
>> > 3. Avoid the dangerous warning in other circumstances.
>> > 4. Delete the file when we're done with it.
>> >
>> > On Fri, Aug 10, 2012 at 9:15 AM,  <mailto:rdsmith@chromium.org> wrote:
>> > > On 2012/08/10 06:04:14, asanka wrote:
>> > >>
>> > >> On 2012/08/09 21:09:11, rdsmith wrote:
>> > >> > On 2012/08/09 17:56:42, Greg Billock wrote:
>> > >> > > On Thu, Aug 9, 2012 at 10:42 AM,  <mailto:rdsmith@chromium.org>
>
> wrote:
>>
>> > >> > > > I don't understand why you're adding a WEB_INTENTS entry to
>> > >> > > > danger
>> > >> > > > type
>> > >
>> > > (I
>> > >>
>> > >> > > > also
>> > >> > > > don't want you to do it, but I'll get to that after I
>> > >> > > > understand
>
> why
>>
>> > >
>> > > :-}).
>> > >>
>> > >> > > > It
>> > >> > > > looks like you just set it, and never test it.
>> > >> > >
>> > >> > > Yes. :-) This is a bit awkward. I originally just set it to
>> > >> > > NOT_DANGEROUS, but that obviously loses information, so I made a
>> > >> > > new
>> > >> > > enum value. I'd be happy to kill it off.
>> > >> > >
>> > >> > > This works for a couple black-magick-y reasons. First is that the
>> > >> > > GetIntermediatePath function will then provision an alternate
>
> filename
>>
>> > >> > > for dangerous files -- that ends up working without alteration if
>> > >> > > we
>> > >> > > use the new enum type. The second is that the details of how the
>> > >> > > download item computes its dangerousness function will place new
>> > >> > > enum
>> > >> > > types in the "not dangerous" bucket. So there's no change
>> > >> > > required
>> > >> > > there either. I agree that it ends up looking very odd in this
>
> change,
>>
>> > >> > > but the result is what we want, I think. (I have only run it on
>
> Linux,
>>
>> > >> > > but that used to pop the warning, and no longer does, so I
>> > >> > > believe it
>> > >> > > will work on all platforms.)
>> > >> > >
>> > >> > > Changing to plain old NOT_DANGEROUS will require some filename
>> > >
>> > > manipulation.
>> > >>
>> > >> > >
>> > >> > > >
>> > >> > > > My image of what you were going to do is that you'd change the
>> > >> > > > file
>> > >> > > > name
>> > >> at
>> > >> > > > file
>> > >> > > > name determination if you were going to handle something with
>> > >> > > > web
>> > >
>> > > intents,
>> > >>
>> > >> > > > and
>> > >> > > > you'd rely on that "should we handle with web intents?" probe
>> > >> > > > to
>> > >> > > > grab
>> > >
>> > > that
>> > >>
>> > >> > > > file
>> > >> > > > at open time.  Can you give me a sense of what I'm missing?
>> > >> > >
>> > >> > > Yes, I want to do this, but I can't figure out where. You see me
>> > >> > > manipulating the filename around line 400 (in the delegate) to
>
> account
>>
>> > >> > > for what I wanted to do, but I can't figure out where to do it.
>> > >> > > Is
>
> the
>>
>> > >> > > final filename set up before the download system kicks in? If so,
>> > >> > > can
>> > >> > > I retroactively change it?
>> > >> >
>> > >> > So Asanka's the right person to evaluate the details here (and I've
>> > >> > added
>> > >
>> > > him
>> > >>
>> > >> to
>> > >> > the CL so he can comment) but I can sketch out the big picture and
>
> point
>>
>> > >> > to
>> > >> > where in that picture you want to go.
>> > >> >
>> > >> > When we start a download, we start downloading to a temporary file
>> > >> > with
>> > >> > a
>> > >> random
>> > >> > name.  Then we call the function
>> > >> > ChromeDownloadManagerDelegate::DetermineDownloadTarget.  That
>> > >> > function
>> > >> > is
>> > >> > responsible for a) Determining the final filename that we want to
>> > >> > use
>> > >> > for
>> > >
>> > > the
>> > >>
>> > >> > download, b) uniquifying that name c) optionally prompting the user
>> > >> > for
>> > >> > the
>> > >> name
>> > >> > if that makes sense,  d) Figuring out the intermediate name for the
>> > >
>> > > download,
>> > >>
>> > >> > and e) figuring out the basic danger status for the download.  What
>> > >> > I
>> > >
>> > > believe
>> > >>
>> > >> > you in the case of web intents is:  a) To unilaterally pick a name
>> > >> > that's
>> > >> > specific to web intents, b) Uniquify that name, c) Not prompt the
>> > >> > user,
>> > >> > d)
>> > >> Keep
>> > >> > the intermediate name logic, and e) Mark the download as not
>> > >> > dangerous.
>> > >> >
>> > >> > DetermineDownloadTarget starts a large cascade of calls
>> > >> (DetermineDownloadTarget
>> > >> > -> CheckDownloadUrlDone -> CheckVisitedReferrerBeforeDone ->
>> > >> > OnPathReservationAvailable -> OnTargetPathDetermined).  I think the
>> > >> > right
>> > >> place
>> > >> > for you to put your stuff is in CheckVisitedReferrerBeforeDone;
>> > >> > that
>> > >> > allows
>> > >> you
>> > >> > to null out prompting, reset danger type, and specify the path you
>
> want.
>>
>> > >
>> > >
>> > >> I'd suggest not overriding the danger type. Picking a filename that
>> > >> is
>> > >
>> > > specific
>> > >>
>> > >> to web intents at CheckVisitedReferrerBeforeDone before the
>> > >> IsDangerousFile()
>> > >> check will cause the file to be considered safe (for the right
>> > >> reasons).
>> > >> If
>> > >
>> > > the
>> > >>
>> > >> download is considered dangerous because of the URL, then I think we
>> > >> should
>> > >> allow the warning.
>> > >
>> > >
>> > >> > However, there are some subtle questions we might want to answer
>> > >> > first:
>> > >> > * Do you really want to completely ignore the danger signals?
>
> Dangerous
>>
>> > >
>> > > file
>> > >>
>> > >> > types are fine to ignore (you'll be setting your own file type) but
>> > >> > I'm
>> > >> > not
>> > >> > clear that dangerous URLs as reported by Safe Browsing are good to
>> > >> > ignore,
>> > >
>> > > nor
>> > >>
>> > >> > that you should ignore the binary evaluations.  So maybe this means
>> > >> > picking
>> > >
>> > > a
>> > >>
>> > >> > new name before we do the dangerous file type evaluation, but
>> > >> > leaving
>> > >> everything
>> > >> > else to work so we do catch dangerous binaries or URLs?
>> > >> >
>> > >> > * Avoiding prompting makes sense for the moment (because you're
>> > >> > going
>
> to
>>
>> > >> > unilaterally change the file name--why ask the user for input if
>> > >> > you're
>> > >
>> > > going
>> > >>
>> > >> to
>> > >> > ignore it?) but we're going to want to prompt in save page as ...
>> > >
>> > > situations.
>> > >>
>> > >> > But that's probably going to be by disabling web intents, so it's
>> > >> > all
>> > >> > good.
>> > >
>> > >
>> > >> We prompt if the user has "always prompt for download location"
>
> preference
>>
>> > >> set
>> > >> unless the download is an extension or of a filetype that we are
>> > >> going to
>> > >
>> > > open.
>> > >>
>> > >> I think it should also check if the download is going to be handled
>> > >> by
>
> web
>>
>> > >> intents here (this is also in CheckVisitedReferrerBeforeDone() at the
>> > >> 'if(download_prefs_->PromptForDownload())' check).
>> > >
>> > >
>> > >> I'm not sure whether you should suppress prompting beyond that if
>> > >> "Save
>> > >> as"
>> > >> downloads are going to not be handled by web intents.
>> > >
>> > >
>> > >> > Asanka, anything else I've missed?
>> > >
>> > >
>> > >> What about downloads with forced file paths? These could be
>> > >> programmatic
>> > >> downloads or a drag and drop.
>> > >
>> > >
>> > > Agreed with all your points.  I'd be inclined to not allow web intents
>> > > for
>> > > forced file paths; those are pretty specific (d&d or chrome internal)
>> > > and
>
> I
>>
>> > > don't think we should intercept them.
>> > >
>> > > https://chromiumcodereview.appspot.com/10836164/
>
>
>> ping. Any further thoughts on this approach?
>
>
> The approach sounds good.  I'll let Asanka do the actual code review (which
> I
> think you're asking for?).
>
> http://codereview.chromium.org/10836164/

[asanka, 2012-08-16 18:27:25]

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:403: void
DeleteFile(const FilePath& file_path) {
Nit: Move to anonymous namespace.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:409: void
OnWebIntentDispatchCompleted(
Nit: This as well.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:431:
ReplaceFirstSubstringAfterOffset(&display_name, display_name.length() - 11,
See comment below about display names.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:725: suggested_path
= suggested_path.AddExtension(".webintents");
I suggest calling download->SetDisplayName(suggested_path.BaseName())
beforehand. This will set the display name of the download to the suggested
name.

Setting the display name affects the name shown in the download shelf as well
what is returned by DownloadItem::GetFileNameToReportUser(). So you can get rid
of the logic in OpenWithWebIntent() to remove the ".webintents" extension.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:725: suggested_path
= suggested_path.AddExtension(".webintents");
Also, consider using a named constant for the ".webintents" extension.

[Greg Billock, 2012-08-16 21:29:51]

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:403: void
DeleteFile(const FilePath& file_path) {
On 2012/08/16 18:27:25, asanka wrote:
> Nit: Move to anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:409: void
OnWebIntentDispatchCompleted(
On 2012/08/16 18:27:25, asanka wrote:
> Nit: This as well.

Done.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:431:
ReplaceFirstSubstringAfterOffset(&display_name, display_name.length() - 11,
On 2012/08/16 18:27:25, asanka wrote:
> See comment below about display names.

Done.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:725: suggested_path
= suggested_path.AddExtension(".webintents");
On 2012/08/16 18:27:25, asanka wrote:
> Also, consider using a named constant for the ".webintents" extension.

Done.

https://chromiumcodereview.appspot.com/10836164/diff/7001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:725: suggested_path
= suggested_path.AddExtension(".webintents");
Got it. Yes. That's very nice.

On 2012/08/16 18:27:25, asanka wrote:
> I suggest calling download->SetDisplayName(suggested_path.BaseName())
> beforehand. This will set the display name of the download to the suggested
> name.
> 
> Setting the display name affects the name shown in the download shelf as well
> what is returned by DownloadItem::GetFileNameToReportUser(). So you can get
rid
> of the logic in OpenWithWebIntent() to remove the ".webintents" extension.

[asanka, 2012-08-17 15:26:42]

LGTM.

Can you add some tests to chrome_download_manager_delegate_unittest.cc? You
could copy something like StartDownload_Basic as a starting point for the
filename generation tests.

I'm not sure what test coverage is there for other aspects of using web intents
that are affected.

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

http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:126: 
Nit: extra space.

[Greg Billock, 2012-08-17 23:22:21]

I'll add tests.

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

http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:126: 
On 2012/08/17 15:26:42, asanka wrote:
> Nit: extra space.

Done.

[Greg Billock, 2012-08-22 21:16:50]

Tests are in. Boy, that was painful to figure out what was going on. And now
deps are keeping me from using TestWebContents? What's a good workaround here?
inherit from ChromeRenderViewHostTestHarness?

On 2012/08/17 23:22:21, Greg Billock wrote:
> I'll add tests.
> 
>
http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
> chrome/browser/download/chrome_download_manager_delegate.cc:126: 
> On 2012/08/17 15:26:42, asanka wrote:
> > Nit: extra space.
> 
> Done.

[Greg Billock, 2012-08-24 18:39:17]

On 2012/08/22 21:16:50, Greg Billock wrote:
> Tests are in. Boy, that was painful to figure out what was going on. And now
> deps are keeping me from using TestWebContents? What's a good workaround here?
> inherit from ChromeRenderViewHostTestHarness?
> 
> On 2012/08/17 23:22:21, Greg Billock wrote:
> > I'll add tests.
> > 
> >
>
http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
> > File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> > 
> >
>
http://codereview.chromium.org/10836164/diff/10001/chrome/browser/download/ch...
> > chrome/browser/download/chrome_download_manager_delegate.cc:126: 
> > On 2012/08/17 15:26:42, asanka wrote:
> > > Nit: extra space.
> > 
> > Done.

Shall I split off the test into another CL? There's a CHECKDEPS issue for
TestWebContents we need to figure out.

[asanka, 2012-08-24 18:45:34]

On 2012/08/22 21:16:50, Greg Billock wrote:
> Tests are in. Boy, that was painful to figure out what was going on. And now
> deps are keeping me from using TestWebContents? What's a good workaround here?
> inherit from ChromeRenderViewHostTestHarness?

Sorry. I somehow didn't see this earlier. Does
content/public/web_contents_tester.h help?

[asanka, 2012-08-24 18:46:12]

On 2012/08/24 18:45:34, asanka wrote:
> On 2012/08/22 21:16:50, Greg Billock wrote:
> > Tests are in. Boy, that was painful to figure out what was going on. And now
> > deps are keeping me from using TestWebContents? What's a good workaround
here?
> > inherit from ChromeRenderViewHostTestHarness?
> 
> Sorry. I somehow didn't see this earlier. Does
> content/public/web_contents_tester.h help?

I mean, content/public/test/web_contents_tester.h.

[Greg Billock, 2012-08-24 18:47:59]

I needed to add some machinery to TestWebContents to not drag in a
bunch of view stuff. I'll plumb those through the Tester and use that.
Thanks.

On Fri, Aug 24, 2012 at 11:46 AM,  <asanka@chromium.org> wrote:
> On 2012/08/24 18:45:34, asanka wrote:
>>
>> On 2012/08/22 21:16:50, Greg Billock wrote:
>> > Tests are in. Boy, that was painful to figure out what was going on. And
>> > now
>> > deps are keeping me from using TestWebContents? What's a good workaround
>
> here?
>>
>> > inherit from ChromeRenderViewHostTestHarness?
>
>
>> Sorry. I somehow didn't see this earlier. Does
>> content/public/web_contents_tester.h help?
>
>
> I mean, content/public/test/web_contents_tester.h.
>
>
>
> http://codereview.chromium.org/10836164/

[Greg Billock, 2012-08-24 22:00:29]

On 2012/08/24 18:47:59, Greg Billock wrote:
> I needed to add some machinery to TestWebContents to not drag in a
> bunch of view stuff. I'll plumb those through the Tester and use that.
> Thanks.
> 
> On Fri, Aug 24, 2012 at 11:46 AM,  <mailto:asanka@chromium.org> wrote:
> > On 2012/08/24 18:45:34, asanka wrote:
> >>
> >> On 2012/08/22 21:16:50, Greg Billock wrote:
> >> > Tests are in. Boy, that was painful to figure out what was going on. And
> >> > now
> >> > deps are keeping me from using TestWebContents? What's a good workaround
> >
> > here?
> >>
> >> > inherit from ChromeRenderViewHostTestHarness?
> >
> >
> >> Sorry. I somehow didn't see this earlier. Does
> >> content/public/web_contents_tester.h help?
> >
> >
> > I mean, content/public/test/web_contents_tester.h.
> >
> >
> >
> > http://codereview.chromium.org/10836164/

John, can you have a look? There are checkdeps rules against using
TestWebContents outside of content (is that right?) I moved the initialization
code tweaks I need to WebContentsTester. (The default TestWebContents init
brings in a bunch of views which crash on destruction, and failing to do all
init crashes the RenderManager on destruction. I tried using the harness, but
that was also crashing on destruction: I think that test class is more
special-purpose than it ought to be. This new construction path makes it a bit
less so.

[jam, 2012-08-27 17:35:13]

On 2012/08/24 22:00:29, Greg Billock wrote:
> On 2012/08/24 18:47:59, Greg Billock wrote:
> > I needed to add some machinery to TestWebContents to not drag in a
> > bunch of view stuff. I'll plumb those through the Tester and use that.
> > Thanks.
> > 
> > On Fri, Aug 24, 2012 at 11:46 AM,  <mailto:asanka@chromium.org> wrote:
> > > On 2012/08/24 18:45:34, asanka wrote:
> > >>
> > >> On 2012/08/22 21:16:50, Greg Billock wrote:
> > >> > Tests are in. Boy, that was painful to figure out what was going on.
And
> > >> > now
> > >> > deps are keeping me from using TestWebContents? What's a good
workaround
> > >
> > > here?
> > >>
> > >> > inherit from ChromeRenderViewHostTestHarness?
> > >
> > >
> > >> Sorry. I somehow didn't see this earlier. Does
> > >> content/public/web_contents_tester.h help?
> > >
> > >
> > > I mean, content/public/test/web_contents_tester.h.
> > >
> > >
> > >
> > > http://codereview.chromium.org/10836164/
> 
> John, can you have a look? There are checkdeps rules against using
> TestWebContents outside of content (is that right?)

yes, code outside of content can only depend on public interface (including
tests)

> I moved the initialization
> code tweaks I need to WebContentsTester. (The default TestWebContents init
> brings in a bunch of views which crash on destruction,

which classes are you referring to?

> and failing to do all
> init crashes the RenderManager on destruction. I tried using the harness,

which harness?

> but
> that was also crashing on destruction: I think that test class is more
> special-purpose than it ought to be. This new construction path makes it a bit
> less so.

[Greg Billock, 2012-08-27 17:59:05]

On Mon, Aug 27, 2012 at 10:35 AM,  <jam@chromium.org> wrote:
> On 2012/08/24 22:00:29, Greg Billock wrote:
>>
>> On 2012/08/24 18:47:59, Greg Billock wrote:
>> > I needed to add some machinery to TestWebContents to not drag in a
>> > bunch of view stuff. I'll plumb those through the Tester and use that.
>> > Thanks.
>> >
>> > On Fri, Aug 24, 2012 at 11:46 AM,  <mailto:asanka@chromium.org> wrote:
>> > > On 2012/08/24 18:45:34, asanka wrote:
>> > >>
>> > >> On 2012/08/22 21:16:50, Greg Billock wrote:
>> > >> > Tests are in. Boy, that was painful to figure out what was going
>> > >> > on.
>
> And
>>
>> > >> > now
>> > >> > deps are keeping me from using TestWebContents? What's a good
>
> workaround
>>
>> > >
>> > > here?
>> > >>
>> > >> > inherit from ChromeRenderViewHostTestHarness?
>> > >
>> > >
>> > >> Sorry. I somehow didn't see this earlier. Does
>> > >> content/public/web_contents_tester.h help?
>> > >
>> > >
>> > > I mean, content/public/test/web_contents_tester.h.
>> > >
>> > >
>> > >
>> > > http://codereview.chromium.org/10836164/
>
>
>> John, can you have a look? There are checkdeps rules against using
>> TestWebContents outside of content (is that right?)
>
>
> yes, code outside of content can only depend on public interface (including
> tests)
>
>
>> I moved the initialization
>> code tweaks I need to WebContentsTester. (The default TestWebContents init
>> brings in a bunch of views which crash on destruction,
>
>
> which classes are you referring to?

TestWebContents : WebContentsImpl --> RenderViewHostManager. This
member crashes on destruction without Init() being called. I wrestled
with the prerequisites for getting Init() to work in the test,
including extending RenderViewHostTestHarness, but finally ended up
believing it would likely just be more useful to make a WebContents
mock with less machinery underneath. (All I need is to attach a
delegate.)

>
>
>> and failing to do all
>> init crashes the RenderManager on destruction. I tried using the harness,
>
>
> which harness?
>
>
>> but
>> that was also crashing on destruction: I think that test class is more
>> special-purpose than it ought to be. This new construction path makes it a
>> bit
>> less so.
>
>
>
>
> http://codereview.chromium.org/10836164/

[jam, 2012-08-28 15:59:50]

On 2012/08/27 17:59:05, Greg Billock wrote:
> On Mon, Aug 27, 2012 at 10:35 AM,  <mailto:jam@chromium.org> wrote:
> > On 2012/08/24 22:00:29, Greg Billock wrote:
> >>
> >> On 2012/08/24 18:47:59, Greg Billock wrote:
> >> > I needed to add some machinery to TestWebContents to not drag in a
> >> > bunch of view stuff. I'll plumb those through the Tester and use that.
> >> > Thanks.
> >> >
> >> > On Fri, Aug 24, 2012 at 11:46 AM,  <mailto:asanka@chromium.org> wrote:
> >> > > On 2012/08/24 18:45:34, asanka wrote:
> >> > >>
> >> > >> On 2012/08/22 21:16:50, Greg Billock wrote:
> >> > >> > Tests are in. Boy, that was painful to figure out what was going
> >> > >> > on.
> >
> > And
> >>
> >> > >> > now
> >> > >> > deps are keeping me from using TestWebContents? What's a good
> >
> > workaround
> >>
> >> > >
> >> > > here?
> >> > >>
> >> > >> > inherit from ChromeRenderViewHostTestHarness?
> >> > >
> >> > >
> >> > >> Sorry. I somehow didn't see this earlier. Does
> >> > >> content/public/web_contents_tester.h help?
> >> > >
> >> > >
> >> > > I mean, content/public/test/web_contents_tester.h.
> >> > >
> >> > >
> >> > >
> >> > > http://codereview.chromium.org/10836164/
> >
> >
> >> John, can you have a look? There are checkdeps rules against using
> >> TestWebContents outside of content (is that right?)
> >
> >
> > yes, code outside of content can only depend on public interface (including
> > tests)
> >
> >
> >> I moved the initialization
> >> code tweaks I need to WebContentsTester. (The default TestWebContents init
> >> brings in a bunch of views which crash on destruction,
> >
> >
> > which classes are you referring to?
> 
> TestWebContents : WebContentsImpl --> RenderViewHostManager. This
> member crashes on destruction without Init() being called. I wrestled
> with the prerequisites for getting Init() to work in the test,
> including extending RenderViewHostTestHarness, but finally ended up
> believing it would likely just be more useful to make a WebContents
> mock with less machinery underneath. (All I need is to attach a
> delegate.)


Where was the crash in RVHM's destructor? in the render_view_host_-> call here
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/web_conten...

if so, just add a null check there and say it can be null in tests. seems
simpler than adding another way to use the test object

> 
> >
> >
> >> and failing to do all
> >> init crashes the RenderManager on destruction. I tried using the harness,
> >
> >
> > which harness?
> >
> >
> >> but
> >> that was also crashing on destruction: I think that test class is more
> >> special-purpose than it ought to be. This new construction path makes it a
> >> bit
> >> less so.
> >
> >
> >
> >
> > http://codereview.chromium.org/10836164/

[Greg Billock, 2012-08-28 16:06:36]

On 2012/08/28 15:59:50, John Abd-El-Malek wrote:
> Where was the crash in RVHM's destructor? in the render_view_host_-> call here
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/web_conten...

Yep, that's the spot. I can add the check there.

[Greg Billock, 2012-08-28 18:24:06]

On 2012/08/28 16:06:36, Greg Billock wrote:
> On 2012/08/28 15:59:50, John Abd-El-Malek wrote:
> > Where was the crash in RVHM's destructor? in the render_view_host_-> call
here
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/web_conten...
> 
> Yep, that's the spot. I can add the check there.

OK, that shrunk the footprint a bit. :-) Like it?

[jam, 2012-08-28 21:50:19]

lgtm

http://codereview.chromium.org/10836164/diff/20001/content/public/test/web_co...
File content/public/test/web_contents_tester.cc (right):

http://codereview.chromium.org/10836164/diff/20001/content/public/test/web_co...
content/public/test/web_contents_tester.cc:68: TestWebContents* contents =
TestWebContents::CreateWithoutInit(browser_context);
nit: 80 chars...

[Greg Billock, 2012-08-29 00:55:30]

http://codereview.chromium.org/10836164/diff/20001/content/public/test/web_co...
File content/public/test/web_contents_tester.cc (right):

http://codereview.chromium.org/10836164/diff/20001/content/public/test/web_co...
content/public/test/web_contents_tester.cc:68: TestWebContents* contents =
TestWebContents::CreateWithoutInit(browser_context);
On 2012/08/28 21:50:19, John Abd-El-Malek wrote:
> nit: 80 chars...

(was reverted in this change)

[commit-bot: I haz the power, 2012-08-29 00:56:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/25002

[commit-bot: I haz the power, 2012-08-29 02:27:06]

Try job failure for 10836164-25002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-29 18:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/36001

[commit-bot: I haz the power, 2012-08-29 20:49:28]

Try job failure for 10836164-36001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-31 16:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/36001

[commit-bot: I haz the power, 2012-08-31 22:35:41]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_chromeos, revision is 154521, job name
was 10836164-36001 (retry) (retry) (previous was lost) (retry).

[commit-bot: I haz the power, 2012-09-01 05:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/36001

[commit-bot: I haz the power, 2012-09-01 09:24:32]

Try job failure for 10836164-36001 (retry) on linux_chromeos for step
"unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-04 18:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/49002

[commit-bot: I haz the power, 2012-09-04 21:31:26]

Try job failure for 10836164-49002 (retry) on linux_chromeos for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-04 23:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/49002

[commit-bot: I haz the power, 2012-09-04 23:52:57]

Try job failure for 10836164-49002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-05 20:43:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/42002

[commit-bot: I haz the power, 2012-09-05 22:34:39]

Try job failure for 10836164-42002 (retry) on linux_chromeos for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Greg Billock, 2012-09-06 01:02:32]

Boy, this test is fighting back really hard. I've given up trying to
get TestWebContents to work standalone and am now trying to get
ChromeRenderViewHostTestHarness to cooperate. Brittle test fixture is
brittle.

On Wed, Sep 5, 2012 at 3:34 PM,  <commit-bot@chromium.org> wrote:
> Try job failure for 10836164-42002 (retry) on linux_chromeos for step
> "unit_tests".
> It's a second try, previously, step "unit_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
>
>
> https://chromiumcodereview.appspot.com/10836164/

[Randy Smith (Not in Mondays), 2012-09-06 15:37:43]

On 2012/09/06 01:02:32, Greg Billock wrote:
> Boy, this test is fighting back really hard. I've given up trying to
> get TestWebContents to work standalone and am now trying to get
> ChromeRenderViewHostTestHarness to cooperate. Brittle test fixture is
> brittle.

If it's the downloads test fixture that's fragile, I'd like to at least look
into fixing it.  Do you have a sense as to where the race/fragility is coming
from?

> 
> On Wed, Sep 5, 2012 at 3:34 PM,  <mailto:commit-bot@chromium.org> wrote:
> > Try job failure for 10836164-42002 (retry) on linux_chromeos for step
> > "unit_tests".
> > It's a second try, previously, step "unit_tests" failed.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> >
> >
> > https://chromiumcodereview.appspot.com/10836164/

[Greg Billock, 2012-09-06 18:30:53]

No, it's the TestWebContents fixtures that are causing the problems.
I've won, but fought for every inch. :-) Want to take a look? It ends
up not being a huge diff.

On Thu, Sep 6, 2012 at 8:37 AM,  <rdsmith@chromium.org> wrote:
> On 2012/09/06 01:02:32, Greg Billock wrote:
>>
>> Boy, this test is fighting back really hard. I've given up trying to
>> get TestWebContents to work standalone and am now trying to get
>> ChromeRenderViewHostTestHarness to cooperate. Brittle test fixture is
>> brittle.
>
>
> If it's the downloads test fixture that's fragile, I'd like to at least look
> into fixing it.  Do you have a sense as to where the race/fragility is
> coming
> from?
>
>
>
>> On Wed, Sep 5, 2012 at 3:34 PM,  <mailto:commit-bot@chromium.org> wrote:
>> > Try job failure for 10836164-42002 (retry) on linux_chromeos for step
>> > "unit_tests".
>> > It's a second try, previously, step "unit_tests" failed.
>> >
>
>
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
>>
>> >
>> >
>> > https://chromiumcodereview.appspot.com/10836164/
>
>
>
>
> http://codereview.chromium.org/10836164/

[Randy Smith (Not in Mondays), 2012-09-06 19:29:20]

I'm not sure I have the right expertise; I only know the downloads code.  What
code would you like me to look at?

[Greg Billock, 2012-09-06 20:53:14]

On 2012/09/06 19:29:20, rdsmith wrote:
> I'm not sure I have the right expertise; I only know the downloads code.  What
> code would you like me to look at?

Just the diffs to the unit test to make sure you are OK with them. The
interesting difference is the new superclass.

[Randy Smith (Not in Mondays), 2012-09-07 14:30:49]

So the basic testing you're doing in the context of the CDMD looks good to me. 
But I don't understand the use of the superclass and the null
MockWebContentsDelegate.  Why didn't the old declaration of the TestingProfile
do what you need?  And what do you get out of the null delegate class?

[Greg Billock, 2012-09-07 15:10:03]

Basically my code relies on the WebContents having a delegate. The
WebContents fixtures come with a lot of particular baggage, especially
in chromeos, that is hard to disentangle. The TestingProfile doesn't
come with a TestWebContents. The reason I took out some fields is that
the new superclass already has them (profile, message loop) and they
cause problems (i.e. crashes for MessageLoop) if they're hidden.


On Fri, Sep 7, 2012 at 7:30 AM,  <rdsmith@chromium.org> wrote:
> So the basic testing you're doing in the context of the CDMD looks good to
> me.
> But I don't understand the use of the superclass and the null
> MockWebContentsDelegate.  Why didn't the old declaration of the
> TestingProfile
> do what you need?  And what do you get out of the null delegate class?
>
>
> http://codereview.chromium.org/10836164/

[Randy Smith (Not in Mondays), 2012-09-07 15:42:02]

Thanks!  LGTM.

[Greg Billock, 2012-09-07 15:46:53]

On 2012/09/07 15:42:02, rdsmith wrote:
> Thanks!  LGTM.

Belatedly realized that my other change
(http://codereview.chromium.org/10880074/) tears this out fixing the TODO. Shall
I just collapse these CLs and re-simplify?

[Greg Billock, 2012-09-07 16:59:21]

On 2012/09/07 15:46:53, Greg Billock wrote:
> On 2012/09/07 15:42:02, rdsmith wrote:
> > Thanks!  LGTM.
> 
> Belatedly realized that my other change
> (http://codereview.chromium.org/10880074/) tears this out fixing the TODO.
Shall
> I just collapse these CLs and re-simplify?

nm. It still needs this delegate. Overly optimistic. :-)

[commit-bot: I haz the power, 2012-09-07 17:00:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10836164/43009

[commit-bot: I haz the power, 2012-09-07 19:10:13]

Change committed as 155443