[downloads] Move platform specific code out of DownloadTargetDeterminer.

[downloads] Move platform specific code out of DownloadTargetDeterminer.

This CL cleans up the download target determination logic so that:


*  DownloadTargetDeterminer provides a reason for requesting user
   confirmation for a download path.

   The caller can thus use platform specific logic to handle the
   confirmation request. Chromium on Android doesn't always prompt the
   user for a download location, and hence needs to know *why* a
   confirmation is requested in order to decide how to proceed.

*  DownloadTargetDeterminer allows the caller to return a finer grained
   result than a boolean following a confirmation request.

   Just knowing whether the user confirmed or rejected is insufficient.
   On Android, for example, there are cases where the platform specific
   code will automatically accept a confirmation request raised by the
   DownloadTargetDeterminer. In such cases, it is important to note
   whether the result indicates explicit user consent or not, which is a
   signal used elsewhere for determining download security.

*  DownloadPathReservationTracker provides a detailed result after
   completing target determination.

   The extra detail allows DownloadTargetDeterminer to indicate *why*
   the target determination failed rather than just the fact that it
   failed. This result can then be translated to an interrupt reason.

*  DownloadItem accepts an interrupt reason during download target
   determination.

   More things can go wrong during this stage of the download than user
   cancellation.

Consequently, the download target conflict resolution logic can now be
moved entirely into ChromeDownloadManagerDelegate. Previously the logic
was split between DownloadTargetDeterminer,
DownloadPathReservationTracker, and ChromeDownloadManagerDelegate on
Android.

BUG=334474

[asanka, 2016-10-27 19:23:01]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 19:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asanka, 2016-10-27 19:35:59]

Description was changed from

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

This makes it possible to have consistent testing for
DownloadTargetDeterminer and also isolate Android specific logic in
ChromeDownloadManagerDelegate.

BUG=none
==========

to

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

The download target conflict resolution logic was split between
DownloadTargetDeterminer, DownloadPathReservationTracker, and
ChromeDownloadManagerDelegate on Android.  This change moves this logic
to ChromeDownloadManagerDelegate so that the logic is testable and
readable.

In addition, this CL changes the functionality of
DownloadPathReservationTracker to return a detailed result code instead
of a boolean. This will allow us to get a richer signal out of
DownloadPathReservationTracker in upcoming changes.

BUG=none
==========

[asanka, 2016-10-27 19:45:21]

Description was changed from

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

The download target conflict resolution logic was split between
DownloadTargetDeterminer, DownloadPathReservationTracker, and
ChromeDownloadManagerDelegate on Android.  This change moves this logic
to ChromeDownloadManagerDelegate so that the logic is testable and
readable.

In addition, this CL changes the functionality of
DownloadPathReservationTracker to return a detailed result code instead
of a boolean. This will allow us to get a richer signal out of
DownloadPathReservationTracker in upcoming changes.

BUG=none
==========

to

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

The download target conflict resolution logic was split between
DownloadTargetDeterminer, DownloadPathReservationTracker, and
ChromeDownloadManagerDelegate on Android.  This change moves this logic
to ChromeDownloadManagerDelegate so that the logic is testable and
readable.

In addition, this CL changes the functionality of
DownloadPathReservationTracker to return a detailed result code instead
of a boolean. This will allow us to get a richer signal out of
DownloadPathReservationTracker in upcoming changes.

BUG=334474
==========

[asanka, 2016-10-27 19:45:39]

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

[asanka, 2016-10-27 19:47:31]

Let me know if this is too big. I can break it up, but didn't because there are
interdependent changes for Android code.

The goal here is to clean up Android code, add tests, and clear the path for
better handling of duplicate downloads.

[svaldez, 2016-10-28 14:57:13]

On 2016/10/27 19:47:31, asanka wrote:
> Let me know if this is too big. I can break it up, but didn't because there
are
> interdependent changes for Android code.
> 
> The goal here is to clean up Android code, add tests, and clear the path for
> better handling of duplicate downloads.

How urgent is this? I'll probably start looking at this today and get through
reviewing it by Monday, but can prioritize higher if needed.

[svaldez, 2016-10-28 15:32:06]

On 2016/10/28 14:57:13, svaldez wrote:
> On 2016/10/27 19:47:31, asanka wrote:
> > Let me know if this is too big. I can break it up, but didn't because there
> are
> > interdependent changes for Android code.
> > 
> > The goal here is to clean up Android code, add tests, and clear the path for
> > better handling of duplicate downloads.
> 
> How urgent is this? I'll probably start looking at this today and get through
> reviewing it by Monday, but can prioritize higher if needed.

Actually nvm, I'll probably get through this in the next couple hours.

[svaldez, 2016-10-28 17:29:36]

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:645: case
DownloadConfirmationReason::TARGET_NO_SPACE:
Should this actually continue?

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:832: NOTREACHED();
Can these lines be removed so that there's a compile warning whenever new
DownloadTargetResult values are added without this switch being updated?

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:861:
EXPECT_EQ(1, infobar_counter.infobar_count());
Possible check this within the loop, so that you are testing the specific
expected test generated the infobar, instead of that one of the tests generated
an info bar.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_path_reservation_tracker.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_path_reservation_tracker.h:36: typedef
base::Callback<void(const base::FilePath& target_path,
Might want to be consistent about whether the "Download...Result" is the first
or last argument across all the callbacks.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_delegate.h:42: typedef
base::Callback<void(const base::FilePath& reserved_path,
Might want to align ReservedPath... and FileSelected... to both have the
Download...Result as the first (or last) argument.

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:121:
EXPECT_EMPTY,  // Expect empty path. Only for downloads which will be marked
Align comments?

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:897: // "Save as"
is not supported on Android.
Should this move up one test?

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:957:
EXPECT_CRDOWNLOAD}};
Extra whitespace?

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:980: size_t
CountDownloadTargetResultValues() {
Seems excessive since if we forget to add a test case in kTestCases and here, we
won't notice.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1050:
ASSERT_EQ(CountDownloadTargetResultValues(), arraysize(kTestCases))
This seems kind of fragile. Maybe something like:

for (int i = 0; i < kNumDownloadConfirmationReason; i++) {
  DownloadConfirmationReason expected_reason;
  DownloadTargetResult result = static_cast<DownloadTargetResult>(i);
  switch(result) {
    case SUCCESS: expected_reason = NONE; break;
    ...
    // No default case should cause a compile error when a new enum is added?
  }
}

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1859:
DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.kindabad",
Should this be ".bad"?

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_info.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_result.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_result.h:34: PATH_NOT_WRITEABLE
*writable

https://codereview.chromium.org/2453633006/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_download_manager_delegate.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_download_manager_delegate.cc:59: }  // namespace
chromecast
Stray whitespace?

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1274: DVLOG(20) << __func__ <<
"() target_path:" << target_path.value()
Should be moved up since otherwise this will never get non-REASON_NONE
interruptions.

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl_unittest.cc:618:
callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
With this I think you should be changing the for statement to loop through
kMaxAutoResumeAttempts instead of kMax+1?

[asanka, 2016-11-05 16:12:50]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-05 16:12:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 16:38:18]

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

[commit-bot: I haz the power, 2016-11-05 16:38:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asanka, 2016-11-07 18:00:24]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 18:00:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 18:50:43]

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

[commit-bot: I haz the power, 2016-11-07 18:50:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asanka, 2016-11-07 19:23:59]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 19:24:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asanka, 2016-11-07 19:50:16]

Second pass. I broke up some renames and moves into a separate CL so that it
won't clutter this already large CL.

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:645: case
DownloadConfirmationReason::TARGET_NO_SPACE:
On 2016/10/28 at 17:29:35, svaldez wrote:
> Should this actually continue?

This one's a bit odd in that the purpose of continuing is to force the download
back into its interrupted state. I.e. attempting to continue to download will
interrupt it again if there's still no space.

On Android there's [currently] only one download location. Hence the user
doesn't have much of a choice on where to download. Hence we show the download
as interrupted and force a manual resumption, but without the extra prompt that
we do on desktop.

Added a comment to this effect.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:832: NOTREACHED();
On 2016/10/28 at 17:29:35, svaldez wrote:
> Can these lines be removed so that there's a compile warning whenever new
DownloadTargetResult values are added without this switch being updated?

This is already the case. I.e. the switch above still needs to be exhaustive for
the compilation to succeed.

The extra lines are here to appease clang which complains that control reaches
end of non-void function if I don't have them.

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:861:
EXPECT_EQ(1, infobar_counter.infobar_count());
On 2016/10/28 at 17:29:35, svaldez wrote:
> Possible check this within the loop, so that you are testing the specific
expected test generated the infobar, instead of that one of the tests generated
an info bar.

Good point. Done.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_path_reservation_tracker.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_path_reservation_tracker.h:36: typedef
base::Callback<void(const base::FilePath& target_path,
On 2016/10/28 at 17:29:35, svaldez wrote:
> Might want to be consistent about whether the "Download...Result" is the first
or last argument across all the callbacks.

Yeah. I made the result be the first argument since it dictates how to interpret
the path argument.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_delegate.h:42: typedef
base::Callback<void(const base::FilePath& reserved_path,
On 2016/10/28 at 17:29:36, svaldez wrote:
> Might want to align ReservedPath... and FileSelected... to both have the
Download...Result as the first (or last) argument.

Done.

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:121:
EXPECT_EMPTY,  // Expect empty path. Only for downloads which will be marked
On 2016/10/28 at 17:29:36, svaldez wrote:
> Align comments?

Done. The former was what clang-format did on its own given a long line. Turns
out if I manually align and wrap it, clang-format leaves it alone.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:897: // "Save as"
is not supported on Android.
On 2016/10/28 at 17:29:36, svaldez wrote:
> Should this move up one test?

Should be removed. At the DTD layer, Android deals correctly with Save As. The
lack of supports is from the higher layers.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:957:
EXPECT_CRDOWNLOAD}};
On 2016/10/28 at 17:29:36, svaldez wrote:
> Extra whitespace?

Reformatted.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:980: size_t
CountDownloadTargetResultValues() {
On 2016/10/28 at 17:29:36, svaldez wrote:
> Seems excessive since if we forget to add a test case in kTestCases and here,
we won't notice.

This one fails to compile if we forget to add an enum value here, and the test
fails if we forget to add a test case.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1050:
ASSERT_EQ(CountDownloadTargetResultValues(), arraysize(kTestCases))
On 2016/10/28 at 17:29:36, svaldez wrote:
> This seems kind of fragile. Maybe something like:
> 
> for (int i = 0; i < kNumDownloadConfirmationReason; i++) {
>   DownloadConfirmationReason expected_reason;
>   DownloadTargetResult result = static_cast<DownloadTargetResult>(i);
>   switch(result) {
>     case SUCCESS: expected_reason = NONE; break;
>     ...
>     // No default case should cause a compile error when a new enum is added?
>   }
> }

Hmm. I tried something like that here. Better?

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:1859:
DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.kindabad",
On 2016/10/28 at 17:29:36, svaldez wrote:
> Should this be ".bad"?

Added another test case for .bad. The semantics are subtly different.

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

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_info.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/10/28 at 17:29:36, svaldez wrote:
> 2016

Done.

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_result.h (right):

https://codereview.chromium.org/2453633006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_result.h:34: PATH_NOT_WRITEABLE
On 2016/10/28 at 17:29:36, svaldez wrote:
> *writable

Done. Apparently "writable" is much more common in Chromium code than
"writeable" both of which are correct spelling.

https://codereview.chromium.org/2453633006/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_download_manager_delegate.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_download_manager_delegate.cc:59: }  // namespace
chromecast
On 2016/10/28 at 17:29:36, svaldez wrote:
> Stray whitespace?

Not sure what happened here. ¯\_(ツ)_/¯

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1274: DVLOG(20) << __func__ <<
"() target_path:" << target_path.value()
On 2016/10/28 at 17:29:36, svaldez wrote:
> Should be moved up since otherwise this will never get non-REASON_NONE
interruptions.

Done.

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2453633006/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl_unittest.cc:618:
callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
On 2016/10/28 at 17:29:36, svaldez wrote:
> With this I think you should be changing the for statement to loop through
kMaxAutoResumeAttempts instead of kMax+1?

It needed more massaging. PTAL? :-)

[asanka, 2016-12-02 19:59:02]

Ping

[asanka, 2017-03-13 23:12:22]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 23:13:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:27:48]

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

[commit-bot: I haz the power, 2017-03-13 23:27:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2017-03-14 01:04:57]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 01:05:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 01:19:52]

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

[commit-bot: I haz the power, 2017-03-14 01:19:53]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asanka, 2017-03-14 01:36:22]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 01:36:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 02:05:19]

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

[commit-bot: I haz the power, 2017-03-14 02:05:20]

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

[asanka, 2017-03-14 22:08:58]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 22:09:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 23:18:59]

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

[commit-bot: I haz the power, 2017-03-14 23:19:00]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[asanka, 2017-03-22 21:26:00]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 21:26:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 21:47:47]

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

[commit-bot: I haz the power, 2017-03-22 21:47:48]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[asanka, 2017-03-23 21:28:21]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 21:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 23:37:09]

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

[commit-bot: I haz the power, 2017-03-23 23:37:10]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[asanka, 2017-03-27 18:05:46]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 18:06:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 18:18:51]

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

[commit-bot: I haz the power, 2017-03-27 18:18:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[asanka, 2017-03-27 20:56:32]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:57:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asanka, 2017-03-27 20:57:32]

Patchset #3 (id:40001) has been deleted

[asanka, 2017-03-27 20:59:58]

Patchset #3 (id:60001) has been deleted

[asanka, 2017-03-27 21:00:13]

Patchset #3 (id:80001) has been deleted

[asanka, 2017-03-27 21:00:32]

Patchset #3 (id:100001) has been deleted

[asanka, 2017-03-27 21:24:49]

Patchset #3 (id:120001) has been deleted

[asanka, 2017-03-27 21:30:02]

Patchset #3 (id:140001) has been deleted

[asanka, 2017-03-27 21:30:18]

Patchset #3 (id:160001) has been deleted

[asanka, 2017-03-27 21:30:28]

Patchset #3 (id:180001) has been deleted

[asanka, 2017-03-27 21:30:41]

Patchset #3 (id:200001) has been deleted

[asanka, 2017-03-27 21:30:50]

Patchset #3 (id:220001) has been deleted

[asanka, 2017-03-27 21:31:01]

Patchset #3 (id:240001) has been deleted

[asanka, 2017-03-27 21:31:12]

Patchset #3 (id:260001) has been deleted

[asanka, 2017-03-27 21:50:02]

Description was changed from

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

The download target conflict resolution logic was split between
DownloadTargetDeterminer, DownloadPathReservationTracker, and
ChromeDownloadManagerDelegate on Android.  This change moves this logic
to ChromeDownloadManagerDelegate so that the logic is testable and
readable.

In addition, this CL changes the functionality of
DownloadPathReservationTracker to return a detailed result code instead
of a boolean. This will allow us to get a richer signal out of
DownloadPathReservationTracker in upcoming changes.

BUG=334474
==========

to

==========
[downloads] Move platform specific code out of DownloadTargetDeterminer.

This CL cleans up the download target determination logic so that:


*  DownloadTargetDeterminer provides a reason for requesting user
   confirmation for a download path.

   The caller can thus use platform specific logic to handle the
   confirmation request. Chromium on Android doesn't always prompt the
   user for a download location, and hence needs to know *why* a
   confirmation is requested in order to decide how to proceed.

*  DownloadTargetDeterminer allows the caller to return a finer grained
   result than a boolean following a confirmation request.

   Just knowing whether the user confirmed or rejected is insufficient.
   On Android, for example, there are cases where the platform specific
   code will automatically accept a confirmation request raised by the
   DownloadTargetDeterminer. In such cases, it is important to note
   whether the result indicates explicit user consent or not, which is a
   signal used elsewhere for determining download security.

*  DownloadPathReservationTracker provides a detailed result after
   completing target determination.

   The extra detail allows DownloadTargetDeterminer to indicate *why*
   the target determination failed rather than just the fact that it
   failed. This result can then be translated to an interrupt reason.

*  DownloadItem accepts an interrupt reason during download target
   determination.

   More things can go wrong during this stage of the download than user
   cancellation.

Consequently, the download target conflict resolution logic can now be
moved entirely into ChromeDownloadManagerDelegate. Previously the logic
was split between DownloadTargetDeterminer,
DownloadPathReservationTracker, and ChromeDownloadManagerDelegate on
Android.

BUG=334474
==========

[commit-bot: I haz the power, 2017-03-27 21:59:33]

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

[commit-bot: I haz the power, 2017-03-27 21:59:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asanka, 2017-03-27 22:03:05]

Patchset #3 (id:280001) has been deleted

[asanka, 2017-03-28 01:55:02]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 01:55:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 03:04:27]

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

[commit-bot: I haz the power, 2017-03-28 03:04:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)