move file access permission logic to DownloadResourceThrottle

move file access permission logic to DownloadResourceThrottle

As previously discussed, DownloadResouceThrottle is the best place to handle file access requests.
This CL moves that logic from DownloadTargetDeterminer to DownloadResourceThrottle.
Unit test is also added for DownloadResourceThrottle.

BUG=501606
Committed: https://crrev.com/58ad68abb6710ce2e9381132b26c70845d777c12
Cr-Commit-Position: refs/heads/master@{#339808}

[qinmin, 2015-07-10 20:01:58]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, tedchoc@chromium.org

[qinmin, 2015-07-10 20:04:10]

qinmin@chromium.org changed reviewers:
+ jochen@chromium.org

[qinmin, 2015-07-10 20:04:10]

PTAL

@asanka for download/
@tedchoc for android/
@jochen for chrome/browser/renderer_host/

[jochen (gone - plz use gerrit), 2015-07-13 11:05:54]

jochen@chromium.org changed reviewers:
+ rdsmith@chromium.org

[jochen (gone - plz use gerrit), 2015-07-13 11:05:54]

+rdsmith because downloads

[Randy Smith (Not in Mondays), 2015-07-13 12:44:32]

On 2015/07/13 11:05:54, jochen wrote:
> +rdsmith because downloads

Asanka's the right person here; his review of
https://codereview.chromium.org/1217223006/ prompted this change.

[asanka, 2015-07-13 19:13:42]

https://codereview.chromium.org/1229933010/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_resource_throttle.cc:59: url,
request_method)));
Let's try to reduce thread hopping.

Currently we start on the IO thread.
  1. IO: DownloadResourceThrottle created. Post to UI thread.
  2. UI: AcquireFileAccessPermission invokes DCA::AcquireFileAccessPermission().
  3. UI: response received from call in 2.
  4. IO: replay response on IO thread by invoking FileAccessPermissionGranted.
  5. IO: Invoke DownloadRequestLimiter::CanDownloadOnIOThread() which posts to
UI thread.
  6. UI: DownloadRequestLimiter::CanDownload() invoked here.
  7. UI: Response from DRL posted to IO thread.
  8. IO: DRT::ContinueDownload() invoked with result from DRL.

How about:
  1. IO: DownloadResourceThrottle created. Post to UI thread.
  2. UI: AcquireFileAccessPermission invokes DCA::AcquireFileAccessPermission().
  3. UI: response received from call in 2.
  4. UI: If response is positive, invoke DownloadRequestLimiter::CanDownload().
  5. UI: Response from DRL posted to IO thread.
  6. IO: DRT::ContinueDownload() invoked with result from DRL.

?

This involves changing DownloadRequestLimiter so that it can be invoked on the
UI thread. This should make it simpler since it's doing all its work on the UI
thread.

DownloadResourceThrottler could then post a task to the UI thread on creation to
do all the work and then invoke ContinueDownload() on the IO thread once a
result is available.

https://codereview.chromium.org/1229933010/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1229933010/diff/1/chrome/chrome_tests_unit.gy...
chrome/chrome_tests_unit.gypi:92:
'browser/download/download_resource_throttle_unittest.cc',
Missing file.

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:53: return NULL;
nullptr

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:55: return
render_view_host->GetDelegate()->GetAsWebContents();
WebContents::FromRenderViewHost()

It does basically the same thing, but we should use use it instead.

[qinmin, 2015-07-14 00:30:57]

https://codereview.chromium.org/1229933010/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_resource_throttle.cc:59: url,
request_method)));
On 2015/07/13 19:13:42, asanka wrote:
> Let's try to reduce thread hopping.
> 
> Currently we start on the IO thread.
>   1. IO: DownloadResourceThrottle created. Post to UI thread.
>   2. UI: AcquireFileAccessPermission invokes
DCA::AcquireFileAccessPermission().
>   3. UI: response received from call in 2.
>   4. IO: replay response on IO thread by invoking FileAccessPermissionGranted.
>   5. IO: Invoke DownloadRequestLimiter::CanDownloadOnIOThread() which posts to
> UI thread.
>   6. UI: DownloadRequestLimiter::CanDownload() invoked here.
>   7. UI: Response from DRL posted to IO thread.
>   8. IO: DRT::ContinueDownload() invoked with result from DRL.
> 
> How about:
>   1. IO: DownloadResourceThrottle created. Post to UI thread.
>   2. UI: AcquireFileAccessPermission invokes
DCA::AcquireFileAccessPermission().
>   3. UI: response received from call in 2.
>   4. UI: If response is positive, invoke
DownloadRequestLimiter::CanDownload().
>   5. UI: Response from DRL posted to IO thread.
>   6. IO: DRT::ContinueDownload() invoked with result from DRL.
> 
> ?
> 
> This involves changing DownloadRequestLimiter so that it can be invoked on the
> UI thread. This should make it simpler since it's doing all its work on the UI
> thread.
> 
> DownloadResourceThrottler could then post a task to the UI thread on creation
to
> do all the work and then invoke ContinueDownload() on the IO thread once a
> result is available.

Done.

https://codereview.chromium.org/1229933010/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1229933010/diff/1/chrome/chrome_tests_unit.gy...
chrome/chrome_tests_unit.gypi:92:
'browser/download/download_resource_throttle_unittest.cc',
On 2015/07/13 19:13:42, asanka wrote:
> Missing file.

Done.

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:53: return NULL;
On 2015/07/13 19:13:42, asanka wrote:
> nullptr

Done.

https://codereview.chromium.org/1229933010/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:55: return
render_view_host->GetDelegate()->GetAsWebContents();
On 2015/07/13 19:13:42, asanka wrote:
> WebContents::FromRenderViewHost()
> 
> It does basically the same thing, but we should use use it instead.

Done.

[asanka, 2015-07-15 20:23:01]

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:438: void
DownloadRequestLimiter::ScheduleNotification(const Callback& callback,
Remove. The notification is now documented to be invoked on the UI thread.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:16: using
content::BrowserThread;
Looks like non-Android code below also depend on this.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:117: void
DownloadResourceThrottle::ContinueDownload(bool allow) {
Add a thread assertion here.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:25: const char
kTestUrl[] = "http://www.test.com/";
Nit: example.com

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:84:
base::RunLoop().RunUntilIdle();
The code as writtten assumes that the thread bundle only has one runner.

It'd be safer to use something like:

  base::RunLoop run_loop;
  content::BrowserThread::PostTask(IO, FROM_HERE, ..., run_loop.QuitClosure());
  run_loop.Run();

This ensures that the specific posted task is run to completion on the target
thread.

If you are waiting for any additional callbacks, then it's best to expect those
callbacks explicitly rather than depending that to happen prior to
RunUntilIdle() stops pumping messages.

[qinmin, 2015-07-17 00:38:52]

Patchset #3 (id:40001) has been deleted

[qinmin, 2015-07-17 00:42:36]

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:438: void
DownloadRequestLimiter::ScheduleNotification(const Callback& callback,
On 2015/07/15 20:23:00, asanka wrote:
> Remove. The notification is now documented to be invoked on the UI thread.

TabDownloadState still calls this method.
But since this is just simply posting a task, Removed this function and moved
the call into TabDownloadState.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:16: using
content::BrowserThread;
On 2015/07/15 20:23:00, asanka wrote:
> Looks like non-Android code below also depend on this.

Done.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:117: void
DownloadResourceThrottle::ContinueDownload(bool allow) {
On 2015/07/15 20:23:00, asanka wrote:
> Add a thread assertion here.

Done.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:25: const char
kTestUrl[] = "http://www.test.com/";
On 2015/07/15 20:23:01, asanka wrote:
> Nit: http://example.com

Done.

https://codereview.chromium.org/1229933010/diff/20001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:84:
base::RunLoop().RunUntilIdle();
On 2015/07/15 20:23:00, asanka wrote:
> The code as writtten assumes that the thread bundle only has one runner.
> 
> It'd be safer to use something like:
> 
>   base::RunLoop run_loop;
>   content::BrowserThread::PostTask(IO, FROM_HERE, ...,
run_loop.QuitClosure());
>   run_loop.Run();
> 
> This ensures that the specific posted task is run to completion on the target
> thread.
> 
> If you are waiting for any additional callbacks, then it's best to expect
those
> callbacks explicitly rather than depending that to happen prior to
> RunUntilIdle() stops pumping messages.

Done. Test will now post QuitClosure() to the UI thread to signal that it has
completed.

[qinmin, 2015-07-17 16:33:44]

Patchset #3 (id:60001) has been deleted

[qinmin, 2015-07-17 16:33:52]

Patchset #3 (id:80001) has been deleted

[Ted C, 2015-07-17 21:51:15]

android - lgtm

[asanka, 2015-07-20 20:15:25]

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.cc:251:
BrowserThread::PostTask(
Why invoke this on the IO thread? The callback is expecting to be invoked on the
UI thread.

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:66:
DownloadRequestLimiter::Callback continue_callback = base::Bind(
Given that none of this logic needs to happen on the IO thread (save for the
AsWeakPtr() call, which can still be done here), how about moving this logic to
a function on the UI thread?

That way we can avoid all the abstract callbacks and have concrete function
calls.

[qinmin, 2015-07-20 22:11:02]

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.cc:251:
BrowserThread::PostTask(
On 2015/07/20 20:15:24, asanka wrote:
> Why invoke this on the IO thread? The callback is expecting to be invoked on
the
> UI thread.

typo, fixed.

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:66:
DownloadRequestLimiter::Callback continue_callback = base::Bind(
On 2015/07/20 20:15:24, asanka wrote:
> Given that none of this logic needs to happen on the IO thread (save for the
> AsWeakPtr() call, which can still be done here), how about moving this logic
to
> a function on the UI thread?
> 
> That way we can avoid all the abstract callbacks and have concrete function
> calls.

Done. Still leaves DownloadResourceThrottle on the IO thread, and refactored the
static functions on the UI thread

[asanka, 2015-07-21 01:14:00]

LGTM modulo nits.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.cc:250: for (size_t i = 0; i <
callbacks.size(); ++i) {
Nit:
  for (const auto& callback : callbacks)
    BrowserThread::PostTask(UI, ... base::Bind(callback, allow));

Probably toss in a comment that doing callback.Run(allow) can cause the
WebContents to be destroyed. It doesn't now, but as far as APIs go, we might as
well play it safe.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:93:
&CanDownloadOnUIThread, limiter, render_process_id, render_view_id,
Nit: When you find yourself binding / unpacking / repacking the same set of
parameters, it's probably cleaner to put them in a struct and pass around a
scoped_ptr<> to the struct.

That way you'll avoid much of the boilerplate.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:94: url,
request_method,base::Bind(
Nit: space after ,

Also run through 'git cl format'

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:95:
&DownloadResourceThrottle::ContinueDownload, AsWeakPtr())));
Nit: Rather than passing up a callback that must be invoked on the IO thread,
it'll be cleaner to pass in a callback that can be invoked on the UI thread.
E.g. if was a OnCanDownloadDecided(WeakPtr<DownloadResourceThrottle>, bool) that
can be invoked on the UI thread.

[jochen (gone - plz use gerrit), 2015-07-21 14:10:59]

lgtm

[qinmin, 2015-07-21 22:42:17]

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.cc:250: for (size_t i = 0; i <
callbacks.size(); ++i) {
On 2015/07/21 01:14:00, asanka wrote:
> Nit:
>   for (const auto& callback : callbacks)
>     BrowserThread::PostTask(UI, ... base::Bind(callback, allow));
> 
> Probably toss in a comment that doing callback.Run(allow) can cause the
> WebContents to be destroyed. It doesn't now, but as far as APIs go, we might
as
> well play it safe.

Done.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:93:
&CanDownloadOnUIThread, limiter, render_process_id, render_view_id,
On 2015/07/21 01:14:00, asanka wrote:
> Nit: When you find yourself binding / unpacking / repacking the same set of
> parameters, it's probably cleaner to put them in a struct and pass around a
> scoped_ptr<> to the struct.
> 
> That way you'll avoid much of the boilerplate.

Done.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:94: url,
request_method,base::Bind(
On 2015/07/21 01:14:00, asanka wrote:
> Nit: space after ,
> 
> Also run through 'git cl format'

Done.

https://codereview.chromium.org/1229933010/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:95:
&DownloadResourceThrottle::ContinueDownload, AsWeakPtr())));
On 2015/07/21 01:14:00, asanka wrote:
> Nit: Rather than passing up a callback that must be invoked on the IO thread,
> it'll be cleaner to pass in a callback that can be invoked on the UI thread.
> E.g. if was a OnCanDownloadDecided(WeakPtr<DownloadResourceThrottle>, bool)
that
> can be invoked on the UI thread.

In that case, I need to make DownloadResourceThrottle::ContinueDownload()
public, or pass DownloadResourceThrottle::ContinueDownload() as a callback into
OnCandownloadDecided(), or make OnCanDownloadDecided a member function of
DownloadResourceThrottle.
Since the last one makes DownloadResourceThrottle to have a member function on
UI thread, choosing the 1st option instead.

[qinmin, 2015-07-21 22:42:22]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-07-21 22:42:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, asanka@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1229933010/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2015-07-21 22:42:35]

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

[commit-bot: I haz the power, 2015-07-21 22:59:32]

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

[commit-bot: I haz the power, 2015-07-21 22:59:33]

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

[qinmin, 2015-07-21 23:30:50]

Patchset #5 (id:140001) has been deleted

[qinmin, 2015-07-21 23:30:57]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-07-21 23:30:58]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
tedchoc@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1229933010/#ps160001
(title: "nits")

[commit-bot: I haz the power, 2015-07-21 23:33:25]

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

[commit-bot: I haz the power, 2015-07-22 01:08:39]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-07-22 01:09:30]

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