Use DownloadManager to download non-html res when download icon pressed

Use DownloadManager to download non-html res when download icon pressed

We check the content's MIME type. If it is not meant for HTML page,we
route to DownloadManager.

BUG=673635
Review-Url: https://codereview.chromium.org/2841823004
Cr-Commit-Position: refs/heads/master@{#467931}
Committed: https://chromium.googlesource.com/chromium/src/+/030745885a307ced7964ab2288cbd36ecd9ec969

[jianli, 2017-04-26 01:02:54]

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

[commit-bot: I haz the power, 2017-04-26 01:03:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2017-04-26 01:03:36]

jianli@chromium.org changed reviewers:
+ dimich@chromium.org

[commit-bot: I haz the power, 2017-04-26 01:44:09]

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

[commit-bot: I haz the power, 2017-04-26 01:44:10]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2017-04-27 23:02:46]

LGTM with a comment.
Also, you'll need to merge with https://codereview.chromium.org/2836163003

https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
(right):

https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:361:
DCHECK(entry);
I'd just do if(!entry) return here...
Otherwise it looks like a crash you sometimes expect.

[jianli, 2017-04-28 00:22:00]

https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
File
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
(right):

https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:361:
DCHECK(entry);
On 2017/04/27 23:02:45, Dmitry Titov wrote:
> I'd just do if(!entry) return here...
> Otherwise it looks like a crash you sometimes expect.

GetLastCommittedURL should return an empty URL if the entry does not exist. So I
added a check at line 343. Using DCHECK here should be safe.

[Dmitry Titov, 2017-04-28 00:26:02]

On 2017/04/28 00:22:00, jianli wrote:
>
https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
> File
> chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
> (right):
> 
>
https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
>
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:361:
> DCHECK(entry);
> On 2017/04/27 23:02:45, Dmitry Titov wrote:
> > I'd just do if(!entry) return here...
> > Otherwise it looks like a crash you sometimes expect.
> 
> GetLastCommittedURL should return an empty URL if the entry does not exist. So
I
> added a check at line 343. Using DCHECK here should be safe.

Could you add a comment to that effect?

[jianli, 2017-04-28 00:32:21]

On 2017/04/28 00:22:00, jianli wrote:
>
https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
> File
> chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
> (right):
> 
>
https://codereview.chromium.org/2841823004/diff/1/chrome/browser/android/offl...
>
chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:361:
> DCHECK(entry);
> On 2017/04/27 23:02:45, Dmitry Titov wrote:
> > I'd just do if(!entry) return here...
> > Otherwise it looks like a crash you sometimes expect.
> 
> GetLastCommittedURL should return an empty URL if the entry does not exist. So
I
> added a check at line 343. Using DCHECK here should be safe.

Done.

[Dmitry Titov, 2017-04-28 04:39:04]

still lgtm.

[jianli, 2017-04-28 07:52:27]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2017-04-28 07:52:46]

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-04-28 08:35:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493365947200460,
"parent_rev": "a7273fae139189164a0a6733cd717e570ebe343e", "commit_rev":
"030745885a307ced7964ab2288cbd36ecd9ec969"}

[commit-bot: I haz the power, 2017-04-28 08:35:32]

Description was changed from

==========
Use DownloadManager to download non-html res when download icon pressed

We check the content's MIME type. If it is not meant for HTML page,we
route to DownloadManager.

BUG=673635
==========

to

==========
Use DownloadManager to download non-html res when download icon pressed

We check the content's MIME type. If it is not meant for HTML page,we
route to DownloadManager.

BUG=673635

Review-Url: https://codereview.chromium.org/2841823004
Cr-Commit-Position: refs/heads/master@{#467931}
Committed:
https://chromium.googlesource.com/chromium/src/+/030745885a307ced7964ab2288cb...
==========

[commit-bot: I haz the power, 2017-04-28 08:35:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/030745885a307ced7964ab2288cb...