Update DownloadRequestLimiter tests for the permission bubble

Update DownloadRequestLimiter tests for the permission bubble

This makes the download request limiter test work with both permission
bubbles and infobars. The unit test is now parameterized so both types
of prompts are now tested.

Extra credit: if the user gesture policy changes in the future, look at
PS1 to see how to make the tests pass with a different user gesture
policy.

Supercedes parts of:
https://codereview.chromium.org/411503005/
https://codereview.chromium.org/341833004/

BUG=438758
Committed: https://crrev.com/702ee6ef4a7e13bafc0f3e204dfacf08d59a0fd3
Cr-Commit-Position: refs/heads/master@{#315008}

[felt, 2014-12-19 08:33:26]

felt@chromium.org changed reviewers:
+ gbillock@chromium.org, rdsmith@chromium.org

[felt, 2014-12-19 08:35:18]

felt@chromium.org changed reviewers:
- gbillock@chromium.org

[felt, 2014-12-19 08:35:40]

rdsmith@, can you please review this update to the DownloadRequestLimiter tests?

[Randy Smith (Not in Mondays), 2014-12-22 18:30:28]

High level discomforts :-}.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
Why do you say this?  I'm not 100% confident in my knowledge of the code, but it
looks to me (from ChromeResourceDispatcherHostDelegate::DownloadStarting) that
any content initiated link would go through this path, and I'd think that the
user clicking on a link marked as a download would be content initiated.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:38: return true;
So this architecture-smells bad to me, and I'm not certain what the right path
is.  If the only way to get around an interface is to say "Yes, this request is
user initiated" when it isn't, that strikes me as indicating a bug in the
interface.  And in an area with security implications, I'm more uncomfortable
than usually with letting misleadingly named interfaces live.  Can we change the
interface name to something less misleading?

[felt, 2014-12-22 18:37:33]

+groby, asanka

When updating this code, I found some weirdness with how permission bubbles
interact with the DownloadRequestLimiter. The DownloadRequestLimiter seems to
have its own complex logic for throttling when a prompt is shown, which differs
from how the permission bubble decides to show a prompt (based entirely on
presence/absence of user gesture). As a result there's some oddness when mashing
these two policies together. Would appreciate your thoughts, as rdsmith pointed
out that my attempt at a fix might not be the best route.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 18:30:28, rdsmith wrote:
> Why do you say this?  I'm not 100% confident in my knowledge of the code, but
it
> looks to me (from ChromeResourceDispatcherHostDelegate::DownloadStarting) that
> any content initiated link would go through this path, and I'd think that the
> user clicking on a link marked as a download would be content initiated.

That's my understanding of Asanka's comments here:
https://codereview.chromium.org/411503005/

It's possible that I misunderstood, though. cc'ing Asanka now.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:38: return true;
On 2014/12/22 18:30:28, rdsmith wrote:
> So this architecture-smells bad to me, and I'm not certain what the right path
> is.  If the only way to get around an interface is to say "Yes, this request
is
> user initiated" when it isn't, that strikes me as indicating a bug in the
> interface.  And in an area with security implications, I'm more uncomfortable
> than usually with letting misleadingly named interfaces live.  Can we change
the
> interface name to something less misleading?

This also admittedly feels odd to me, but I don't think renaming solves it. More
ideas:

* Maybe the permission bubble shouldn't be used for this at all, if it wants to
have its own policy that differs from other permissions?

* Maybe this speaks to the need for a more general bubble manager policy, where
UserGesture is only one pieces of information among many used to decide the
policy for whether to show the bubble?

[Randy Smith (Not in Mondays), 2014-12-22 18:46:51]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 18:37:33, felt wrote:
> On 2014/12/22 18:30:28, rdsmith wrote:
> > Why do you say this?  I'm not 100% confident in my knowledge of the code,
but
> it
> > looks to me (from ChromeResourceDispatcherHostDelegate::DownloadStarting)
that
> > any content initiated link would go through this path, and I'd think that
the
> > user clicking on a link marked as a download would be content initiated.
> 
> That's my understanding of Asanka's comments here:
> https://codereview.chromium.org/411503005/
> 
> It's possible that I misunderstood, though. cc'ing Asanka now.

Which comment?  What I see Asanka saying is "DownloadRequestLimiter is
per-download. Each instance will have a somewhat accurate flag indicating
whether there's an associated user gesture.", which is my understanding as well.

[felt, 2014-12-22 18:50:11]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 18:46:51, rdsmith wrote:
> On 2014/12/22 18:37:33, felt wrote:
> > On 2014/12/22 18:30:28, rdsmith wrote:
> > > Why do you say this?  I'm not 100% confident in my knowledge of the code,
> but
> > it
> > > looks to me (from ChromeResourceDispatcherHostDelegate::DownloadStarting)
> that
> > > any content initiated link would go through this path, and I'd think that
> the
> > > user clicking on a link marked as a download would be content initiated.
> > 
> > That's my understanding of Asanka's comments here:
> > https://codereview.chromium.org/411503005/
> > 
> > It's possible that I misunderstood, though. cc'ing Asanka now.
> 
> Which comment?  What I see Asanka saying is "DownloadRequestLimiter is
> per-download. Each instance will have a somewhat accurate flag indicating
> whether there's an associated user gesture.", which is my understanding as
well.

"Why is it necessary to invert this? Downloads that are initiated due to an
explicit user requests ('Save as' etc...) don't make it here. The ones that are
throttled are those that are considered "content initiated." I.e. the decision
to download was made by the content rather than the browser. "

I interpreted this to mean that user gestures here are irrelevant because the
only ones that end up in the request limiter are the ones w/o user gestures.

[Randy Smith (Not in Mondays), 2014-12-22 18:55:14]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 18:50:11, felt wrote:
> On 2014/12/22 18:46:51, rdsmith wrote:
> > On 2014/12/22 18:37:33, felt wrote:
> > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > Why do you say this?  I'm not 100% confident in my knowledge of the
code,
> > but
> > > it
> > > > looks to me (from
ChromeResourceDispatcherHostDelegate::DownloadStarting)
> > that
> > > > any content initiated link would go through this path, and I'd think
that
> > the
> > > > user clicking on a link marked as a download would be content initiated.
> > > 
> > > That's my understanding of Asanka's comments here:
> > > https://codereview.chromium.org/411503005/
> > > 
> > > It's possible that I misunderstood, though. cc'ing Asanka now.
> > 
> > Which comment?  What I see Asanka saying is "DownloadRequestLimiter is
> > per-download. Each instance will have a somewhat accurate flag indicating
> > whether there's an associated user gesture.", which is my understanding as
> well.
> 
> "Why is it necessary to invert this? Downloads that are initiated due to an
> explicit user requests ('Save as' etc...) don't make it here. The ones that
are
> throttled are those that are considered "content initiated." I.e. the decision
> to download was made by the content rather than the browser. "
> 
> I interpreted this to mean that user gestures here are irrelevant because the
> only ones that end up in the request limiter are the ones w/o user gestures.

Ah!  Thank you.  My guess/understanding is that that comment is making a
distinction between right clicking and choosing a menu option/choosing a menu
option from the hotdog menu ("explicit user request") versus clicking a link
(which is what produces the user-gesture flag).  But Asanka can provide more
precise detail.

Mind you, I don't think any of this matters a lot, because I doubt that we
actually want to plumb the user-gesture information through this code.  But I'd
like the comment to reflect reality, whatever reality is :-}.

[groby-ooo-7-16, 2014-12-22 18:55:38]

groby@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2014-12-22 18:55:39]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:38: return true;
On 2014/12/22 18:37:33, felt wrote:
> On 2014/12/22 18:30:28, rdsmith wrote:
> > So this architecture-smells bad to me, and I'm not certain what the right
path
> > is.  If the only way to get around an interface is to say "Yes, this request
> is
> > user initiated" when it isn't, that strikes me as indicating a bug in the
> > interface.  And in an area with security implications, I'm more
uncomfortable
> > than usually with letting misleadingly named interfaces live.  Can we change
> the
> > interface name to something less misleading?
> 
> This also admittedly feels odd to me, but I don't think renaming solves it.

I think it calls for a "ForceDisplay" API, which will force pending permission
requests to be flushed into a new bubble.

> More ideas:
> 
> * Maybe the permission bubble shouldn't be used for this at all, if it wants
to
> have its own policy that differs from other permissions?

That strikes me as the logical conclusion. Unless we can adjust the policy - why
are downloads behaving differently?

> 
> * Maybe this speaks to the need for a more general bubble manager policy,
where
> UserGesture is only one pieces of information among many used to decide the
> policy for whether to show the bubble?

While we're trying to figure out a more general manager policy, that's far from
done. I wouldn't wait for it to solve this :)

[Randy Smith (Not in Mondays), 2014-12-22 19:04:17]

On 2014/12/22 18:55:39, groby wrote:
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> File chrome/browser/download/download_permission_request.cc (right):
> 
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> chrome/browser/download/download_permission_request.cc:38: return true;
> On 2014/12/22 18:37:33, felt wrote:
> > On 2014/12/22 18:30:28, rdsmith wrote:
> > > So this architecture-smells bad to me, and I'm not certain what the right
> path
> > > is.  If the only way to get around an interface is to say "Yes, this
request
> > is
> > > user initiated" when it isn't, that strikes me as indicating a bug in the
> > > interface.  And in an area with security implications, I'm more
> uncomfortable
> > > than usually with letting misleadingly named interfaces live.  Can we
change
> > the
> > > interface name to something less misleading?
> > 
> > This also admittedly feels odd to me, but I don't think renaming solves it.
> 
> I think it calls for a "ForceDisplay" API, which will force pending permission
> requests to be flushed into a new bubble.
> 
> > More ideas:
> > 
> > * Maybe the permission bubble shouldn't be used for this at all, if it wants
> to
> > have its own policy that differs from other permissions?
> 
> That strikes me as the logical conclusion. Unless we can adjust the policy -
why
> are downloads behaving differently?

Because downloads are a very messy territory in the user-convenience vs.
security war :-J. 

Policy: Often, when a user initiates a download of (say) a software package, the
web site sends them to a new page indicating that their download is starting,
and initiates the download in the background.  Popping up a UI asking the user
permission would be a frustrating user experience; from their perspective, they
just said "Download this!" and they don't want to have to say it again.  But we
can't allow websites to download arbitrary files without user involvement.  So
we compromise: We allow each site a single download (user-initiated or not), and
bug the user after that download.  Note that this occurs in the context of other
constraints; if the download is considered "dangerous" and we don't have a
user-gesture associated with it (<-- oversimplified; actually logic much more
complicated), the user will need to explicitly allow the download to complete,
and the file won't show up in their filesystem under a non-random name until
they do.  

Mechanism: When DownloadRequestLimiter was created, we didn't have permissions
bubbles, so we had do to all this work ourselves with infobars.  Permissions
bubbles were grafted onto the infobar implementation afterwards, and the
assumptions of the two interfaces don't quite match.  

Future: I'm happy to have all of this fixed in some grand unified
user-permission re-architecture, but when you do that, please make sure to get
security and downloads (== Asanka or my) input; this code is *riddled* with
corner cases.  And no, I'm not happy about that being true in security sensitive
code either.

> 
> > 
> > * Maybe this speaks to the need for a more general bubble manager policy,
> where
> > UserGesture is only one pieces of information among many used to decide the
> > policy for whether to show the bubble?
> 
> While we're trying to figure out a more general manager policy, that's far
from
> done. I wouldn't wait for it to solve this :)

[groby-ooo-7-16, 2014-12-22 19:30:42]

On 2014/12/22 19:04:17, rdsmith wrote:
> On 2014/12/22 18:55:39, groby wrote:
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > File chrome/browser/download/download_permission_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > chrome/browser/download/download_permission_request.cc:38: return true;
> > On 2014/12/22 18:37:33, felt wrote:
> > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > So this architecture-smells bad to me, and I'm not certain what the
right
> > path
> > > > is.  If the only way to get around an interface is to say "Yes, this
> request
> > > is
> > > > user initiated" when it isn't, that strikes me as indicating a bug in
the
> > > > interface.  And in an area with security implications, I'm more
> > uncomfortable
> > > > than usually with letting misleadingly named interfaces live.  Can we
> change
> > > the
> > > > interface name to something less misleading?
> > > 
> > > This also admittedly feels odd to me, but I don't think renaming solves
it.
> > 
> > I think it calls for a "ForceDisplay" API, which will force pending
permission
> > requests to be flushed into a new bubble.
> > 
> > > More ideas:
> > > 
> > > * Maybe the permission bubble shouldn't be used for this at all, if it
wants
> > to
> > > have its own policy that differs from other permissions?
> > 
> > That strikes me as the logical conclusion. Unless we can adjust the policy -
> why
> > are downloads behaving differently?
> 
> Because downloads are a very messy territory in the user-convenience vs.
> security war :-J. 
> 
> Policy: Often, when a user initiates a download of (say) a software package,
the
> web site sends them to a new page indicating that their download is starting,
> and initiates the download in the background.  Popping up a UI asking the user
> permission would be a frustrating user experience; from their perspective,
they
> just said "Download this!" and they don't want to have to say it again.  But
we
> can't allow websites to download arbitrary files without user involvement.  So
> we compromise: We allow each site a single download (user-initiated or not),
and
> bug the user after that download.  Note that this occurs in the context of
other
> constraints; if the download is considered "dangerous" and we don't have a
> user-gesture associated with it (<-- oversimplified; actually logic much more
> complicated), the user will need to explicitly allow the download to complete,
> and the file won't show up in their filesystem under a non-random name until
> they do.  
> 
> Mechanism: When DownloadRequestLimiter was created, we didn't have permissions
> bubbles, so we had do to all this work ourselves with infobars.  Permissions
> bubbles were grafted onto the infobar implementation afterwards, and the
> assumptions of the two interfaces don't quite match.  
> 
> Future: I'm happy to have all of this fixed in some grand unified
> user-permission re-architecture, but when you do that, please make sure to get
> security and downloads (== Asanka or my) input; this code is *riddled* with
> corner cases.  And no, I'm not happy about that being true in security
sensitive
> code either.
> 
> > 
> > > 
> > > * Maybe this speaks to the need for a more general bubble manager policy,
> > where
> > > UserGesture is only one pieces of information among many used to decide
the
> > > policy for whether to show the bubble?
> > 
> > While we're trying to figure out a more general manager policy, that's far
> from
> > done. I wouldn't wait for it to solve this :)

Thanks for the explanation! Putting this into a "bubble manager" framework, it
sounds like you need to be able to mark some requests as "display always". 
Which sits better with me than overriding HasUserGesture. (Everybody calling
that function will assume it is actually user-initiated, and I really don't want
to introduce an accidental exploit. There's got to be a better way to fame! ;)

DownloadRequestLimiter will probably still need to carry that part of the policy
- it's way to specific to handle that in the grand unified permission manager.
So, as a proposal: What if bubble request gained a priority setting?
(DISPLAY_ALWAYS, MAY_COALESCE)

[felt, 2014-12-22 19:34:52]

On 2014/12/22 19:30:42, groby wrote:
> On 2014/12/22 19:04:17, rdsmith wrote:
> > On 2014/12/22 18:55:39, groby wrote:
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > File chrome/browser/download/download_permission_request.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > chrome/browser/download/download_permission_request.cc:38: return true;
> > > On 2014/12/22 18:37:33, felt wrote:
> > > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > > So this architecture-smells bad to me, and I'm not certain what the
> right
> > > path
> > > > > is.  If the only way to get around an interface is to say "Yes, this
> > request
> > > > is
> > > > > user initiated" when it isn't, that strikes me as indicating a bug in
> the
> > > > > interface.  And in an area with security implications, I'm more
> > > uncomfortable
> > > > > than usually with letting misleadingly named interfaces live.  Can we
> > change
> > > > the
> > > > > interface name to something less misleading?
> > > > 
> > > > This also admittedly feels odd to me, but I don't think renaming solves
> it.
> > > 
> > > I think it calls for a "ForceDisplay" API, which will force pending
> permission
> > > requests to be flushed into a new bubble.
> > > 
> > > > More ideas:
> > > > 
> > > > * Maybe the permission bubble shouldn't be used for this at all, if it
> wants
> > > to
> > > > have its own policy that differs from other permissions?
> > > 
> > > That strikes me as the logical conclusion. Unless we can adjust the policy
-
> > why
> > > are downloads behaving differently?
> > 
> > Because downloads are a very messy territory in the user-convenience vs.
> > security war :-J. 
> > 
> > Policy: Often, when a user initiates a download of (say) a software package,
> the
> > web site sends them to a new page indicating that their download is
starting,
> > and initiates the download in the background.  Popping up a UI asking the
user
> > permission would be a frustrating user experience; from their perspective,
> they
> > just said "Download this!" and they don't want to have to say it again.  But
> we
> > can't allow websites to download arbitrary files without user involvement. 
So
> > we compromise: We allow each site a single download (user-initiated or not),
> and
> > bug the user after that download.  Note that this occurs in the context of
> other
> > constraints; if the download is considered "dangerous" and we don't have a
> > user-gesture associated with it (<-- oversimplified; actually logic much
more
> > complicated), the user will need to explicitly allow the download to
complete,
> > and the file won't show up in their filesystem under a non-random name until
> > they do.  
> > 
> > Mechanism: When DownloadRequestLimiter was created, we didn't have
permissions
> > bubbles, so we had do to all this work ourselves with infobars.  Permissions
> > bubbles were grafted onto the infobar implementation afterwards, and the
> > assumptions of the two interfaces don't quite match.  
> > 
> > Future: I'm happy to have all of this fixed in some grand unified
> > user-permission re-architecture, but when you do that, please make sure to
get
> > security and downloads (== Asanka or my) input; this code is *riddled* with
> > corner cases.  And no, I'm not happy about that being true in security
> sensitive
> > code either.
> > 
> > > 
> > > > 
> > > > * Maybe this speaks to the need for a more general bubble manager
policy,
> > > where
> > > > UserGesture is only one pieces of information among many used to decide
> the
> > > > policy for whether to show the bubble?
> > > 
> > > While we're trying to figure out a more general manager policy, that's far
> > from
> > > done. I wouldn't wait for it to solve this :)
> 
> Thanks for the explanation! Putting this into a "bubble manager" framework, it
> sounds like you need to be able to mark some requests as "display always". 
> Which sits better with me than overriding HasUserGesture. (Everybody calling
> that function will assume it is actually user-initiated, and I really don't
want
> to introduce an accidental exploit. There's got to be a better way to fame! ;)
> 
> DownloadRequestLimiter will probably still need to carry that part of the
policy
> - it's way to specific to handle that in the grand unified permission manager.
> So, as a proposal: What if bubble request gained a priority setting?
> (DISPLAY_ALWAYS, MAY_COALESCE)

That's a good idea, I will add the ability to mark a request like this.

This is opening a closely related can of worms, which is that right now some
APIs
(eg notifications) do not require a user gesture to display an infobar. I
suspect
there will be demand to therefore put DISPLAY_ALWAYS on many requests, which
might make the coalescing policy a moot point. A TODO for Q1 will probably be
to figure out the right process for this.

[groby-ooo-7-16, 2014-12-22 20:01:47]

On 2014/12/22 19:34:52, felt wrote:
> On 2014/12/22 19:30:42, groby wrote:
> > On 2014/12/22 19:04:17, rdsmith wrote:
> > > On 2014/12/22 18:55:39, groby wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > > File chrome/browser/download/download_permission_request.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > > chrome/browser/download/download_permission_request.cc:38: return true;
> > > > On 2014/12/22 18:37:33, felt wrote:
> > > > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > > > So this architecture-smells bad to me, and I'm not certain what the
> > right
> > > > path
> > > > > > is.  If the only way to get around an interface is to say "Yes, this
> > > request
> > > > > is
> > > > > > user initiated" when it isn't, that strikes me as indicating a bug
in
> > the
> > > > > > interface.  And in an area with security implications, I'm more
> > > > uncomfortable
> > > > > > than usually with letting misleadingly named interfaces live.  Can
we
> > > change
> > > > > the
> > > > > > interface name to something less misleading?
> > > > > 
> > > > > This also admittedly feels odd to me, but I don't think renaming
solves
> > it.
> > > > 
> > > > I think it calls for a "ForceDisplay" API, which will force pending
> > permission
> > > > requests to be flushed into a new bubble.
> > > > 
> > > > > More ideas:
> > > > > 
> > > > > * Maybe the permission bubble shouldn't be used for this at all, if it
> > wants
> > > > to
> > > > > have its own policy that differs from other permissions?
> > > > 
> > > > That strikes me as the logical conclusion. Unless we can adjust the
policy
> -
> > > why
> > > > are downloads behaving differently?
> > > 
> > > Because downloads are a very messy territory in the user-convenience vs.
> > > security war :-J. 
> > > 
> > > Policy: Often, when a user initiates a download of (say) a software
package,
> > the
> > > web site sends them to a new page indicating that their download is
> starting,
> > > and initiates the download in the background.  Popping up a UI asking the
> user
> > > permission would be a frustrating user experience; from their perspective,
> > they
> > > just said "Download this!" and they don't want to have to say it again. 
But
> > we
> > > can't allow websites to download arbitrary files without user involvement.

> So
> > > we compromise: We allow each site a single download (user-initiated or
not),
> > and
> > > bug the user after that download.  Note that this occurs in the context of
> > other
> > > constraints; if the download is considered "dangerous" and we don't have a
> > > user-gesture associated with it (<-- oversimplified; actually logic much
> more
> > > complicated), the user will need to explicitly allow the download to
> complete,
> > > and the file won't show up in their filesystem under a non-random name
until
> > > they do.  
> > > 
> > > Mechanism: When DownloadRequestLimiter was created, we didn't have
> permissions
> > > bubbles, so we had do to all this work ourselves with infobars. 
Permissions
> > > bubbles were grafted onto the infobar implementation afterwards, and the
> > > assumptions of the two interfaces don't quite match.  
> > > 
> > > Future: I'm happy to have all of this fixed in some grand unified
> > > user-permission re-architecture, but when you do that, please make sure to
> get
> > > security and downloads (== Asanka or my) input; this code is *riddled*
with
> > > corner cases.  And no, I'm not happy about that being true in security
> > sensitive
> > > code either.
> > > 
> > > > 
> > > > > 
> > > > > * Maybe this speaks to the need for a more general bubble manager
> policy,
> > > > where
> > > > > UserGesture is only one pieces of information among many used to
decide
> > the
> > > > > policy for whether to show the bubble?
> > > > 
> > > > While we're trying to figure out a more general manager policy, that's
far
> > > from
> > > > done. I wouldn't wait for it to solve this :)
> > 
> > Thanks for the explanation! Putting this into a "bubble manager" framework,
it
> > sounds like you need to be able to mark some requests as "display always". 
> > Which sits better with me than overriding HasUserGesture. (Everybody calling
> > that function will assume it is actually user-initiated, and I really don't
> want
> > to introduce an accidental exploit. There's got to be a better way to fame!
;)
> > 
> > DownloadRequestLimiter will probably still need to carry that part of the
> policy
> > - it's way to specific to handle that in the grand unified permission
manager.
> > So, as a proposal: What if bubble request gained a priority setting?
> > (DISPLAY_ALWAYS, MAY_COALESCE)
> 
> That's a good idea, I will add the ability to mark a request like this.
> 
> This is opening a closely related can of worms, which is that right now some
> APIs
> (eg notifications) do not require a user gesture to display an infobar. I
> suspect
> there will be demand to therefore put DISPLAY_ALWAYS on many requests, which
> might make the coalescing policy a moot point. A TODO for Q1 will probably be
> to figure out the right process for this.

I've thrown it on my list as well. For now, I'd ignore notifications, since they
don't play into permission bubbles. (Or do they, and I miss something?)

[felt, 2014-12-22 20:03:35]

On 2014/12/22 20:01:47, groby wrote:
> On 2014/12/22 19:34:52, felt wrote:
> > On 2014/12/22 19:30:42, groby wrote:
> > > On 2014/12/22 19:04:17, rdsmith wrote:
> > > > On 2014/12/22 18:55:39, groby wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > > > File chrome/browser/download/download_permission_request.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
> > > > > chrome/browser/download/download_permission_request.cc:38: return
true;
> > > > > On 2014/12/22 18:37:33, felt wrote:
> > > > > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > > > > So this architecture-smells bad to me, and I'm not certain what
the
> > > right
> > > > > path
> > > > > > > is.  If the only way to get around an interface is to say "Yes,
this
> > > > request
> > > > > > is
> > > > > > > user initiated" when it isn't, that strikes me as indicating a bug
> in
> > > the
> > > > > > > interface.  And in an area with security implications, I'm more
> > > > > uncomfortable
> > > > > > > than usually with letting misleadingly named interfaces live.  Can
> we
> > > > change
> > > > > > the
> > > > > > > interface name to something less misleading?
> > > > > > 
> > > > > > This also admittedly feels odd to me, but I don't think renaming
> solves
> > > it.
> > > > > 
> > > > > I think it calls for a "ForceDisplay" API, which will force pending
> > > permission
> > > > > requests to be flushed into a new bubble.
> > > > > 
> > > > > > More ideas:
> > > > > > 
> > > > > > * Maybe the permission bubble shouldn't be used for this at all, if
it
> > > wants
> > > > > to
> > > > > > have its own policy that differs from other permissions?
> > > > > 
> > > > > That strikes me as the logical conclusion. Unless we can adjust the
> policy
> > -
> > > > why
> > > > > are downloads behaving differently?
> > > > 
> > > > Because downloads are a very messy territory in the user-convenience vs.
> > > > security war :-J. 
> > > > 
> > > > Policy: Often, when a user initiates a download of (say) a software
> package,
> > > the
> > > > web site sends them to a new page indicating that their download is
> > starting,
> > > > and initiates the download in the background.  Popping up a UI asking
the
> > user
> > > > permission would be a frustrating user experience; from their
perspective,
> > > they
> > > > just said "Download this!" and they don't want to have to say it again. 
> But
> > > we
> > > > can't allow websites to download arbitrary files without user
involvement.
> 
> > So
> > > > we compromise: We allow each site a single download (user-initiated or
> not),
> > > and
> > > > bug the user after that download.  Note that this occurs in the context
of
> > > other
> > > > constraints; if the download is considered "dangerous" and we don't have
a
> > > > user-gesture associated with it (<-- oversimplified; actually logic much
> > more
> > > > complicated), the user will need to explicitly allow the download to
> > complete,
> > > > and the file won't show up in their filesystem under a non-random name
> until
> > > > they do.  
> > > > 
> > > > Mechanism: When DownloadRequestLimiter was created, we didn't have
> > permissions
> > > > bubbles, so we had do to all this work ourselves with infobars. 
> Permissions
> > > > bubbles were grafted onto the infobar implementation afterwards, and the
> > > > assumptions of the two interfaces don't quite match.  
> > > > 
> > > > Future: I'm happy to have all of this fixed in some grand unified
> > > > user-permission re-architecture, but when you do that, please make sure
to
> > get
> > > > security and downloads (== Asanka or my) input; this code is *riddled*
> with
> > > > corner cases.  And no, I'm not happy about that being true in security
> > > sensitive
> > > > code either.
> > > > 
> > > > > 
> > > > > > 
> > > > > > * Maybe this speaks to the need for a more general bubble manager
> > policy,
> > > > > where
> > > > > > UserGesture is only one pieces of information among many used to
> decide
> > > the
> > > > > > policy for whether to show the bubble?
> > > > > 
> > > > > While we're trying to figure out a more general manager policy, that's
> far
> > > > from
> > > > > done. I wouldn't wait for it to solve this :)
> > > 
> > > Thanks for the explanation! Putting this into a "bubble manager"
framework,
> it
> > > sounds like you need to be able to mark some requests as "display always".

> > > Which sits better with me than overriding HasUserGesture. (Everybody
calling
> > > that function will assume it is actually user-initiated, and I really
don't
> > want
> > > to introduce an accidental exploit. There's got to be a better way to
fame!
> ;)
> > > 
> > > DownloadRequestLimiter will probably still need to carry that part of the
> > policy
> > > - it's way to specific to handle that in the grand unified permission
> manager.
> > > So, as a proposal: What if bubble request gained a priority setting?
> > > (DISPLAY_ALWAYS, MAY_COALESCE)
> > 
> > That's a good idea, I will add the ability to mark a request like this.
> > 
> > This is opening a closely related can of worms, which is that right now some
> > APIs
> > (eg notifications) do not require a user gesture to display an infobar. I
> > suspect
> > there will be demand to therefore put DISPLAY_ALWAYS on many requests, which
> > might make the coalescing policy a moot point. A TODO for Q1 will probably
be
> > to figure out the right process for this.
> 
> I've thrown it on my list as well. For now, I'd ignore notifications, since
they
> don't play into permission bubbles. (Or do they, and I miss something?)

The notification permission request uses a permission bubble, that's what I
meant.

[asanka, 2014-12-23 20:44:10]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2014-12-23 20:44:11]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_permission_request.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 at 18:55:14, rdsmith wrote:
> On 2014/12/22 18:50:11, felt wrote:
> > On 2014/12/22 18:46:51, rdsmith wrote:
> > > On 2014/12/22 18:37:33, felt wrote:
> > > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > > Why do you say this?  I'm not 100% confident in my knowledge of the
code,
> > > but
> > > > it
> > > > > looks to me (from
ChromeResourceDispatcherHostDelegate::DownloadStarting)
> > > that
> > > > > any content initiated link would go through this path, and I'd think
that
> > > the
> > > > > user clicking on a link marked as a download would be content
initiated.
> > > > 
> > > > That's my understanding of Asanka's comments here:
> > > > https://codereview.chromium.org/411503005/
> > > > 
> > > > It's possible that I misunderstood, though. cc'ing Asanka now.
> > > 
> > > Which comment?  What I see Asanka saying is "DownloadRequestLimiter is
> > > per-download. Each instance will have a somewhat accurate flag indicating
> > > whether there's an associated user gesture.", which is my understanding as
> > well.
> > 
> > "Why is it necessary to invert this? Downloads that are initiated due to an
> > explicit user requests ('Save as' etc...) don't make it here. The ones that
are
> > throttled are those that are considered "content initiated." I.e. the
decision
> > to download was made by the content rather than the browser. "
> > 
> > I interpreted this to mean that user gestures here are irrelevant because
the
> > only ones that end up in the request limiter are the ones w/o user gestures.
> 
> Ah!  Thank you.  My guess/understanding is that that comment is making a
distinction between right clicking and choosing a menu option/choosing a menu
option from the hotdog menu ("explicit user request") versus clicking a link
(which is what produces the user-gesture flag).  But Asanka can provide more
precise detail.
> 
> Mind you, I don't think any of this matters a lot, because I doubt that we
actually want to plumb the user-gesture information through this code.  But I'd
like the comment to reflect reality, whatever reality is :-}.

You guys are reading too much into a caffeine deprived comment :P. Let me try to
be clearer (and rdsmith's interpretation is correct).

We make a distinction between user gesture and user intent.

When a download is initiated due to a chrome side user interaction (e.g. user
picking "Save as..." from the menu) we have a high degree of confidence that the
user actually intends to download. Hence such downloads aren't throttled and
DownloadRequestLimiter isn't invoked. These are what I was referring to as
"explicit user requests".

On the other hand, if a download is initiated by content, we can't tell if the
user actually intended to download the resource. The underlying
ResourceRequestInfo may have HasUserGesture() set to true. But even then we are
still uncertain about the user's intent (e.g. the user clicked on a link to
navigate to a page, but the response had an unknown Content-Type, or the user
clicked on an anchor with a "download" attribute without expecting a download).
So we route all content initiated downloads through the DownloadRequestLimiter.
This uncertainty is also the reason why we aren't too concerned about plumbing
the HasUserGesture() property from the request info. That flag just doesn't
carry useful information at this point.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_permission_request.cc:31: // Only non-gestured
requests go through the path of the download request
On 2014/12/22 at 18:55:14, rdsmith wrote:
> On 2014/12/22 18:50:11, felt wrote:
> > On 2014/12/22 18:46:51, rdsmith wrote:
> > > On 2014/12/22 18:37:33, felt wrote:
> > > > On 2014/12/22 18:30:28, rdsmith wrote:
> > > > > Why do you say this?  I'm not 100% confident in my knowledge of the
code,
> > > but
> > > > it
> > > > > looks to me (from
ChromeResourceDispatcherHostDelegate::DownloadStarting)
> > > that
> > > > > any content initiated link would go through this path, and I'd think
that
> > > the
> > > > > user clicking on a link marked as a download would be content
initiated.
> > > > 
> > > > That's my understanding of Asanka's comments here:
> > > > https://codereview.chromium.org/411503005/
> > > > 
> > > > It's possible that I misunderstood, though. cc'ing Asanka now.
> > > 
> > > Which comment?  What I see Asanka saying is "DownloadRequestLimiter is
> > > per-download. Each instance will have a somewhat accurate flag indicating
> > > whether there's an associated user gesture.", which is my understanding as
> > well.
> > 
> > "Why is it necessary to invert this? Downloads that are initiated due to an
> > explicit user requests ('Save as' etc...) don't make it here. The ones that
are
> > throttled are those that are considered "content initiated." I.e. the
decision
> > to download was made by the content rather than the browser. "
> > 
> > I interpreted this to mean that user gestures here are irrelevant because
the
> > only ones that end up in the request limiter are the ones w/o user gestures.
> 
> Ah!  Thank you.  My guess/understanding is that that comment is making a
distinction between right clicking and choosing a menu option/choosing a menu
option from the hotdog menu ("explicit user request") versus clicking a link
(which is what produces the user-gesture flag).  But Asanka can provide more
precise detail.
> 
> Mind you, I don't think any of this matters a lot, because I doubt that we
actually want to plumb the user-gesture information through this code.  But I'd
like the comment to reflect reality, whatever reality is :-}.

You guys are reading too much into a caffeine deprived comment :P. Let me try to
be clearer (and rdsmith's interpretation is correct).

We make a distinction between user gesture and user intent.

When a download is initiated due to a chrome side user interaction (e.g. user
picking "Save as..." from the menu) we have a high degree of confidence that the
user actually intends to download. Hence such downloads aren't throttled and
DownloadRequestLimiter isn't invoked. These are what I was referring to as
"explicit user requests".

On the other hand, if a download is initiated by content, we can't tell if the
user actually intended to download the resource. The underlying
ResourceRequestInfo may have HasUserGesture() set to true. But even then we are
still uncertain about the user's intent (e.g. the user clicked on a link to
navigate to a page, but the response had an unknown Content-Type, or the user
clicked on an anchor with a "download" attribute without expecting a download).
So we route all content initiated downloads through the DownloadRequestLimiter.
This uncertainty is also the reason why we aren't too concerned about plumbing
the HasUserGesture() property from the request info. That flag just doesn't
carry useful information at this point.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:189:
DocumentOnLoadCompletedInMainFrame();
Curious: Why does this need to be called explicitly?

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:250: #endif
Mismatched } for OS_ANDROID.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:250: #endif
Mismatched } for OS_ANDROID.

[felt, 2015-02-05 18:33:43]

Hello dear reviewers! In the spirit of the new year, I have revived this CL for
42.

For the reasons discussed here (and others), I've disabled the user gesture
requirement for permission bubbles for now. I've updated the comment in this CL
to reflect that. So all that is left in this CL is to just update the tests.

PTAL!

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:189:
DocumentOnLoadCompletedInMainFrame();
On 2014/12/23 20:44:11, asanka wrote:
> Curious: Why does this need to be called explicitly?

There's a good comment here that explains what the bubble manager in the method:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

In the tests, that event will not fire on its own.

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:250: #endif
On 2014/12/23 20:44:11, asanka wrote:
> Mismatched } for OS_ANDROID.

Done.

[groby-ooo-7-16, 2015-02-05 19:15:35]

LGTM

[asanka, 2015-02-05 19:44:08]

LGTM

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:189:
DocumentOnLoadCompletedInMainFrame();
On 2015/02/05 18:33:43, felt wrote:
> On 2014/12/23 20:44:11, asanka wrote:
> > Curious: Why does this need to be called explicitly?
> 
> There's a good comment here that explains what the bubble manager in the
method:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

That comment is a bit scary because if there's any cross-thread asynchrony,
posting a task to the UI thread doesn't guarantee that all AddRequest() calls
have been seen by the time TriggerShowBubble() is called.

> In the tests, that event will not fire on its own.

Got it. My confusion was why NavigateAndCommit() doesn't automatically result in
DocumentOnLoadCompleted being called. It makes sense now that I looked at how
that event is fired.

https://codereview.chromium.org/811163004/diff/60001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/60001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:187: void
BubbleManagerDocumentLoadCompleted() {
Should this be a no-op if permission bubbles are disabled?

[felt, 2015-02-06 09:30:44]

New patchsets have been uploaded after l-g-t-m from
groby@chromium.org,asanka@chromium.org

[felt, 2015-02-06 09:31:28]

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/40001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:189:
DocumentOnLoadCompletedInMainFrame();
On 2015/02/05 19:44:08, asanka wrote:
> On 2015/02/05 18:33:43, felt wrote:
> > On 2014/12/23 20:44:11, asanka wrote:
> > > Curious: Why does this need to be called explicitly?
> > 
> > There's a good comment here that explains what the bubble manager in the
> method:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> That comment is a bit scary because if there's any cross-thread asynchrony,
> posting a task to the UI thread doesn't guarantee that all AddRequest() calls
> have been seen by the time TriggerShowBubble() is called.

OK. I will investigate. I suspect this is causing another bug that I'm seeing,
where sometimes requests are dropped. (I'm not the original author of this code
but I am going around trying to clean it up and make it nice.)

> 
> > In the tests, that event will not fire on its own.
> 
> Got it. My confusion was why NavigateAndCommit() doesn't automatically result
in
> DocumentOnLoadCompleted being called. It makes sense now that I looked at how
> that event is fired.

https://codereview.chromium.org/811163004/diff/60001/chrome/browser/download/...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/811163004/diff/60001/chrome/browser/download/...
chrome/browser/download/download_request_limiter_unittest.cc:187: void
BubbleManagerDocumentLoadCompleted() {
On 2015/02/05 19:44:08, asanka wrote:
> Should this be a no-op if permission bubbles are disabled?

Done.

[felt, 2015-02-06 09:31:43]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-02-06 09:32:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811163004/80001

[commit-bot: I haz the power, 2015-02-06 10:55:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-06 10:55:37]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[felt, 2015-02-06 11:31:10]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-02-06 11:32:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811163004/80001

[commit-bot: I haz the power, 2015-02-06 12:16:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-06 12:17:30]

Patchset 5 (id:??) landed as
https://crrev.com/702ee6ef4a7e13bafc0f3e204dfacf08d59a0fd3
Cr-Commit-Position: refs/heads/master@{#315008}