Update DownloadRequestLimiter tests for the permission bubble

chrome/browser/download/download_permission_request.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_permission_request.h"
 
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 10 matching lines @@
 
 base::string16 DownloadPermissionRequest::GetMessageText() const {
   return l10n_util::GetStringUTF16(IDS_MULTI_DOWNLOAD_WARNING);
 }
 
 base::string16 DownloadPermissionRequest::GetMessageTextFragment() const {
   return l10n_util::GetStringUTF16(IDS_MULTI_DOWNLOAD_PERMISSION_FRAGMENT);
 }
 
 bool DownloadPermissionRequest::HasUserGesture() const {
-  // TODO(gbillock): user gesture for multiple downloads is difficult to
-  // propagate, and the normal thing is that it is background.
-  return false;
+  // Only non-gestured requests go through the path of the download request

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

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.

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

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.

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

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]

"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]

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 :-}.

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

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.

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

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.

+  // limiter. However, the DownloadRequestLimiter has its own logic for
+  // determining whether to show a prompt. Setting this to true lets the
+  // DownloadRequestLimiter control the policy and override the user gesture
+  // requirement.
+  // TODO(felt): Investigate whether this policy should be subsumed by an
+  // overall bubble manager policy.
+  return true;

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

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]

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?

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

I think it calls for a "ForceDisplay" API, which will force pending permission
requests to be flushed into a new bubble.
That strikes me as the logical conclusion. Unless we can adjust the policy - why
are downloads behaving differently?
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 :)

 }
 
 GURL DownloadPermissionRequest::GetRequestingHostname() const {
   const content::WebContents* web_contents = host_->web_contents();
   if (web_contents) {
     return web_contents->GetURL();
   }
   return GURL();
 }
 
@@ skipping 12 matching lines @@
 }
 
 void DownloadPermissionRequest::Cancelled() {
   // TODO(gbillock): There's currently no suitable method for telling the host
   // that a request is cancelled.
 }
 
 void DownloadPermissionRequest::RequestFinished() {
   delete this;
 }