Stop offline page resource throttle from intercepting non http(s) urls

Stop offline page resource throttle from intercepting non http(s) urls

offline_page_model.cc doesn't handle non http(s) urls.
As a result, there is no point for the resource throttle to intercept the request.

BUG=713309
Review-Url: https://codereview.chromium.org/2836163003
Cr-Commit-Position: refs/heads/master@{#467356}
Committed: https://chromium.googlesource.com/chromium/src/+/6b48b5c45ce1ee5396f157999fc46082f2b7622b

[qinmin, 2017-04-24 21:00:32]

qinmin@chromium.org changed reviewers:
+ dimich@chromium.org, dtrainor@chromium.org

[qinmin, 2017-04-24 21:00:33]

PTAL

[David Trainor- moved to gerrit, 2017-04-25 03:31:17]

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

https://codereview.chromium.org/2836163003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:60: if
(CanDownloadAsOfflinePage(mime_type)) {
Would it be cleaner to bake that into CanDownloadAsOfflinePage?  e.g.

CanDownloadAsOfflinePage(url, mime_type)?

[Dmitry Titov, 2017-04-25 07:03:01]

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

https://codereview.chromium.org/2836163003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:51: if
(request_->url_chain().empty())
URLRequest is constructed with at least one URL, the chain is never empty.
I think this check can be safely omitted and instead of url_chain().back() it
can just fetch request_->url() in the next line.

https://codereview.chromium.org/2836163003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:60: if
(CanDownloadAsOfflinePage(mime_type)) {
On 2017/04/25 03:31:16, David Trainor-ping if over 24h wrote:
> Would it be cleaner to bake that into CanDownloadAsOfflinePage?  e.g.
> 
> CanDownloadAsOfflinePage(url, mime_type)?

+1, good suggestion.

[qinmin, 2017-04-25 18:52:46]

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

https://codereview.chromium.org/2836163003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:51: if
(request_->url_chain().empty())
On 2017/04/25 07:03:01, Dmitry Titov wrote:
> URLRequest is constructed with at least one URL, the chain is never empty.
> I think this check can be safely omitted and instead of url_chain().back() it
> can just fetch request_->url() in the next line.

Done.

https://codereview.chromium.org/2836163003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:60: if
(CanDownloadAsOfflinePage(mime_type)) {
On 2017/04/25 03:31:16, David Trainor-ping if over 24h wrote:
> Would it be cleaner to bake that into CanDownloadAsOfflinePage?  e.g.
> 
> CanDownloadAsOfflinePage(url, mime_type)?

Done.

[David Trainor- moved to gerrit, 2017-04-26 16:05:53]

lgtm

[qinmin, 2017-04-26 16:22:38]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-26 16:22:45]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2017-04-26 16:23:04]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-04-26 16:23:40]

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-26 17:11:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493223784176280,
"parent_rev": "49ce38e06e2307125474c6e9f2db0f47ae45b3e1", "commit_rev":
"6b48b5c45ce1ee5396f157999fc46082f2b7622b"}

[commit-bot: I haz the power, 2017-04-26 17:12:11]

Description was changed from

==========
Stop offline page resource throttle from intercepting non http(s) urls

offline_page_model.cc doesn't handle non http(s) urls.
As a result, there is no point for the resource throttle to intercept the
request.

BUG=713309
==========

to

==========
Stop offline page resource throttle from intercepting non http(s) urls

offline_page_model.cc doesn't handle non http(s) urls.
As a result, there is no point for the resource throttle to intercept the
request.

BUG=713309

Review-Url: https://codereview.chromium.org/2836163003
Cr-Commit-Position: refs/heads/master@{#467356}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b48b5c45ce1ee5396f157999fc4...
==========

[commit-bot: I haz the power, 2017-04-26 17:12:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6b48b5c45ce1ee5396f157999fc4...