[Android Downloads] Long-press menu item "Download Link" should delegate job to OfflinePages backen…

[Android Downloads] Long-press menu item "Download Link" should delegate job to OfflinePages backend.
This patch interrupts the download and hands it off to the OfflinePage backend once the mime type of the download is detected and it is text/html.
Other downloads not affected.

BUG=643731
Committed: https://crrev.com/4f6b28049376a0e309fda4cbeb6e4d7aac98965c
Cr-Commit-Position: refs/heads/master@{#439630}

[Dmitry Titov, 2016-11-23 01:02:22]

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

[commit-bot: I haz the power, 2016-11-23 01:03:39]

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

[Dmitry Titov, 2016-11-23 01:13:29]

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

[Dmitry Titov, 2016-11-23 01:13:29]

Asanka, could you please take a look if this is a reasonable hook up to Download
backend?

Jian: please take a look overall and specifically Offline Pages.

Thanks!

[Dmitry Titov, 2016-11-23 01:15:52]

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

[commit-bot: I haz the power, 2016-11-23 01:16:44]

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

[jianli, 2016-11-23 01:27:33]

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:220:
RequestCoordinator* request_coordinator =
RequestCoordinator instance will be null (per
https://codereview.chromium.org/2522073002/) under incognito mode since
OfflinePageModel is not created. Do we want to handle this?

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:223: ClientId
client_id;
Probably better to say:
  ClientId client_id(kDownloadNamespace, base::GenerateGUID());

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

https://codereview.chromium.org/2528483003/diff/20001/content/browser/downloa...
content/browser/download/download_request_core.cc:270: result =
DOWNLOAD_INTERRUPT_REASON_PAGE_DOWNLOAD_HANDOFF;
Do we want to return instead?

[Pete Williamson, 2016-11-23 01:42:40]

petewil@chromium.org changed reviewers:
+ petewil@chromium.org

[Pete Williamson, 2016-11-23 01:42:41]

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:220:
RequestCoordinator* request_coordinator =
On 2016/11/23 01:27:32, jianli wrote:
> RequestCoordinator instance will be null (per
> https://codereview.chromium.org/2522073002/) under incognito mode since
> OfflinePageModel is not created. Do we want to handle this?

Drive-by:  May I suggest returning early if the request_coordinator pointer is
null?

[commit-bot: I haz the power, 2016-11-23 02:08:02]

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

[commit-bot: I haz the power, 2016-11-23 02:08:03]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dmitry Titov, 2016-11-23 20:17:31]

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

[Dmitry Titov, 2016-11-23 20:18:00]

PTAL

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:220:
RequestCoordinator* request_coordinator =
On 2016/11/23 01:27:32, jianli wrote:
> RequestCoordinator instance will be null (per
> https://codereview.chromium.org/2522073002/) under incognito mode since
> OfflinePageModel is not created. Do we want to handle this?

Done.

https://codereview.chromium.org/2528483003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:223: ClientId
client_id;
On 2016/11/23 01:27:32, jianli wrote:
> Probably better to say:
>   ClientId client_id(kDownloadNamespace, base::GenerateGUID());

Done.

[commit-bot: I haz the power, 2016-11-23 20:18:09]

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-23 20:35:41]

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

[commit-bot: I haz the power, 2016-11-23 20:35:42]

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

[asanka, 2016-11-23 20:47:13]

Let's not put download handoff logic in DownloadRequestCore. Whether or not a
request corresponds to a download is a decision made elsewhere. In addition, the
code in //content is shared across all embedders, not just Chrome. A decision to
hand off the request to a Chrome specific backend cannot be made there.

The usual approach for doing this sort of Chrome specific interception is to
write a content::ResourceThrottle that cancels the corresponding ResourceLoader
and hands off the job to somewhere else.

It would look like this:

  class InterceptWebPageDownloadAsOfflinePagesRequest : public
content::ResourceThrottle {
    ...
    void WillProcessResponse(bool* defer) override {
      if (... /* this is a webpage download */) {
        controller()->Cancel();
        ... // Create offline page download
        return false;
      }
      return true;
    }
  };

Then in ChromeResourceDispatcherHostDelegage::DownloadStarting(), you can add
the new resource throttle on Android. This method is called by
ResourceDispatcherHost when starting a new download so that the embedder can add
resource throttles for download handling.

Note that it's important to add your throttle *after* CRDHD::DownloadStarting()
calls AppendStandardResourceThrottles(). The latter is what adds the
SafeBrowsing resource throttles. We should allow SafeBrowsing to intervene
before deciding whether or not to make the page available offline.

A side-effect of this method is that Chrome never creates a download for
intercepted requests. So you don't need to deal with special handling using a
new interrupt reason.

[Dmitry Titov, 2016-11-24 02:04:55]

On 2016/11/23 20:47:13, asanka wrote:
> Let's not put download handoff logic in DownloadRequestCore. Whether or not a
> request corresponds to a download is a decision made elsewhere. In addition,
the
> code in //content is shared across all embedders, not just Chrome. A decision
to
> hand off the request to a Chrome specific backend cannot be made there.
> 
> The usual approach for doing this sort of Chrome specific interception is to
> write a content::ResourceThrottle that cancels the corresponding
ResourceLoader
> and hands off the job to somewhere else.
> 
> It would look like this:
> 
>   class InterceptWebPageDownloadAsOfflinePagesRequest : public
> content::ResourceThrottle {
>     ...
>     void WillProcessResponse(bool* defer) override {
>       if (... /* this is a webpage download */) {
>         controller()->Cancel();
>         ... // Create offline page download
>         return false;
>       }
>       return true;
>     }
>   };
> 
> Then in ChromeResourceDispatcherHostDelegage::DownloadStarting(), you can add
> the new resource throttle on Android. This method is called by
> ResourceDispatcherHost when starting a new download so that the embedder can
add
> resource throttles for download handling.
> 
> Note that it's important to add your throttle *after*
CRDHD::DownloadStarting()
> calls AppendStandardResourceThrottles(). The latter is what adds the
> SafeBrowsing resource throttles. We should allow SafeBrowsing to intervene
> before deciding whether or not to make the page available offline.
> 
> A side-effect of this method is that Chrome never creates a download for
> intercepted requests. So you don't need to deal with special handling using a
> new interrupt reason.

Thanks Asanka for a good suggestion! 

One trouble, I can't see how I can access the mime_type that is a result of
MimeSniffingResourceHandler in the ResourceThrottle..
It seems I need it and not just a Content-Type header value, since the value may
not be there or be correct. Perhaps I need to add a member 'override_mime_type'
to ResourceRequestInfoImpl...

I'll be OOO until Dec 5th, but will figure it out after that and update the
patch, no need to look at it until then.
Thanks!

[asanka, 2016-11-28 19:42:05]

On 2016/11/24 at 02:04:55, dimich wrote:
> On 2016/11/23 20:47:13, asanka wrote:
> > Let's not put download handoff logic in DownloadRequestCore. Whether or not
a
> > request corresponds to a download is a decision made elsewhere. In addition,
the
> > code in //content is shared across all embedders, not just Chrome. A
decision to
> > hand off the request to a Chrome specific backend cannot be made there.
> > 
> > The usual approach for doing this sort of Chrome specific interception is to
> > write a content::ResourceThrottle that cancels the corresponding
ResourceLoader
> > and hands off the job to somewhere else.
> > 
> > It would look like this:
> > 
> >   class InterceptWebPageDownloadAsOfflinePagesRequest : public
> > content::ResourceThrottle {
> >     ...
> >     void WillProcessResponse(bool* defer) override {
> >       if (... /* this is a webpage download */) {
> >         controller()->Cancel();
> >         ... // Create offline page download
> >         return false;
> >       }
> >       return true;
> >     }
> >   };
> > 
> > Then in ChromeResourceDispatcherHostDelegage::DownloadStarting(), you can
add
> > the new resource throttle on Android. This method is called by
> > ResourceDispatcherHost when starting a new download so that the embedder can
add
> > resource throttles for download handling.
> > 
> > Note that it's important to add your throttle *after*
CRDHD::DownloadStarting()
> > calls AppendStandardResourceThrottles(). The latter is what adds the
> > SafeBrowsing resource throttles. We should allow SafeBrowsing to intervene
> > before deciding whether or not to make the page available offline.
> > 
> > A side-effect of this method is that Chrome never creates a download for
> > intercepted requests. So you don't need to deal with special handling using
a
> > new interrupt reason.
> 
> Thanks Asanka for a good suggestion! 
> 
> One trouble, I can't see how I can access the mime_type that is a result of
MimeSniffingResourceHandler in the ResourceThrottle..
> It seems I need it and not just a Content-Type header value, since the value
may not be there or be correct. Perhaps I need to add a member
'override_mime_type' to ResourceRequestInfoImpl...

The "good" news is that there's no MIME sniffing done for explicit downloads :-/

MIME sniffing is currently only done for navigation initiated requests that are
slated to be handled by a renderer. MimeSniffingResourceHandler is responsible
for sniffing the MIME type AND switching the ResourceHandler chain to treat the
response as a download if applicable. For this bundling of responsibility to
cause a problem for downloads all of the following would need to be true:

*) The response can be sniffed. (e.g. no 'X-Content-Type-Options: nosniff'
header).
*) The Content-Type for the response is non-existent or warrants sniffing.
*) The filename determined for the resource doesn't result in correct opening
behavior.
*) The filename determined for the resource based on a sniffed MIME type
would've resulted in correct opening behavior.

So far the class of downloads that tick all the boxes has been small enough to
not rise up above the noise of random failures. Historically the MIME type
hasn't been a reliable input for filename determination for regular (non-HTML)
downloads. Confoundingly, incorporating the MIME type is known to result in
worse results than ignoring it.

But now we want to determine the download mechanism based on the MIME type.
Hence it's important to know the correct MIME type for a resource that we've
already decided to download.

Doing so would involve decoupling the roles of MIME sniffing and handler
determination (the task of looking at a sniffed response and decide whether it
should be handled by a renderer or be downloaded). Then we can use MIME sniffing
for explicit downloads without confusing the ResourceHandler chain. We can also
hack around by adding a no-op InterceptingResourceHandler to
MimeSniffingResourceHandler for use with downloads, but that's going to make it
difficult to reason about the ResourceHandler chain since we can't tell what
happens after invoking InterceptingResourceHandler. :-(

All of this assumes that explicit downloads of HTML pages that are served
without a correct Content-Type are numerous enough to justify the extra work.

> I'll be OOO until Dec 5th, but will figure it out after that and update the
patch, no need to look at it until then.
> Thanks!

[Dmitry Titov, 2016-12-07 03:45:53]

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

[commit-bot: I haz the power, 2016-12-07 03:46:14]

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

[Dmitry Titov, 2016-12-07 03:47:53]

asanka, PTAL. I think this is closer to what you had in mind...

[commit-bot: I haz the power, 2016-12-07 04:51:56]

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

[commit-bot: I haz the power, 2016-12-07 04:51:57]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-12-07 16:37:40]

Code LGTM with % comments. I'd suggest adding a test and I've noted an existing
"Save link as" test which you might be able to use as a base.

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/downloads/resource_throttle.cc
(right):

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:5: #include
"chrome/browser/android/offline_pages/downloads/resource_throttle.h"
You could use the SaveLinkAsReferrerPolicyOrigin test in
//chrome/browser/download/download_browsertest.cc as a template to build a test
for this.

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:15:
namespace {
Nit: whitespace. Perhaps run this through 'git cl format'.

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:18: const
char kHTMLMimeType[] = "text/html";
SavePackage considers both text/html and application/xhtml+xml as HTML (see
https://cs.chromium.org/chromium/src/content/browser/download/save_package.cc...).

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.cc:49:
controller()->Cancel();
Let's defer the Cancel() call until after you're done accessing request_ and its
ResourceRequestInfo. Doing so makes the code robust against state changes that
would happen as a result of the Cancel() call.

The cancellation is currently always asynchronous and doesn't affect the
lifetime of |*this|, |request_| and its ResourceRequestInfo, but that depends on
behavior deep in the guts of //net.

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/downloads/resource_throttle.h (right):

https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Nit: copyright header is different now:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Dmitry Titov, 2016-12-17 02:31:35]

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

[commit-bot: I haz the power, 2016-12-17 02:32:18]

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

[Dmitry Titov, 2016-12-17 03:02:04]

On 2016/12/07 16:37:40, asanka wrote:
> You could use the SaveLinkAsReferrerPolicyOrigin test in
> //chrome/browser/download/download_browsertest.cc as a template to build a
test
> for this.

Unfortunately, this feature is #ifdef for Android and the chrome browsertests
are not compilable on Android yet...
The content/browser/download/download_browsertest.cc is compilable on Android,
but it does not include throttlers and most of chrome/browser/loader stuff...

So I am not sure how to implement a test that would run on Android. I've added a
unit test for the Part of the functionality of the patch, and plan to continue
to think how to test the end-to-end.


> 
>
https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
> chrome/browser/android/offline_pages/downloads/resource_throttle.cc:15:
> namespace {
> Nit: whitespace. Perhaps run this through 'git cl format'.

Done.

>
https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
> chrome/browser/android/offline_pages/downloads/resource_throttle.cc:18: const
> char kHTMLMimeType[] = "text/html";
> SavePackage considers both text/html and application/xhtml+xml as HTML (see
>
https://cs.chromium.org/chromium/src/content/browser/download/save_package.cc...).

Done.

>
https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
> chrome/browser/android/offline_pages/downloads/resource_throttle.cc:49:
> controller()->Cancel();
> Let's defer the Cancel() call until after you're done accessing request_ and
its
> ResourceRequestInfo. Doing so makes the code robust against state changes that
> would happen as a result of the Cancel() call.
> 
> The cancellation is currently always asynchronous and doesn't affect the
> lifetime of |*this|, |request_| and its ResourceRequestInfo, but that depends
on
> behavior deep in the guts of //net.

Done.

>
https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
> File chrome/browser/android/offline_pages/downloads/resource_throttle.h
(right):
> 
>
https://codereview.chromium.org/2528483003/diff/100001/chrome/browser/android...
> chrome/browser/android/offline_pages/downloads/resource_throttle.h:1: //
> Copyright (c) 2012 The Chromium Authors. All rights reserved.
> Nit: copyright header is different now:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

[commit-bot: I haz the power, 2016-12-17 04:14:48]

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

[commit-bot: I haz the power, 2016-12-17 04:14:49]

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_...)

[Dmitry Titov, 2016-12-19 19:46:03]

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

[Dmitry Titov, 2016-12-19 19:46:04]

+rdsmith as OWNER of chrome/browser/loader.

[Pete Williamson, 2016-12-19 21:48:58]

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
File chrome/browser/android/offline_pages/downloads/resource_throttle.h (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.h:16: // file
download.
It might be nice to beef up this comment a bit to include why we are using a
resource throttle to catch this attempt, since we aren't really throttling.

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.cc:243:
notification_bridge.ShowDownloadingToast();
Do we need this?  I was thinking that the toast was created by adding the save
page later request.

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:554:
throttles->push_back(
Should the throttle be pushed back *before* we call
AppendStandardResourceThrottles?

[Randy Smith (Not in Mondays), 2016-12-19 22:09:41]

content/browser/loader LGTM

[Dmitry Titov, 2016-12-19 22:39:32]

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
File chrome/browser/android/offline_pages/downloads/resource_throttle.h (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/downloads/resource_throttle.h:16: // file
download.
On 2016/12/19 21:48:58, Pete Williamson wrote:
> It might be nice to beef up this comment a bit to include why we are using a
> resource throttle to catch this attempt, since we aren't really throttling.

Done.

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.cc:243:
notification_bridge.ShowDownloadingToast();
On 2016/12/19 21:48:58, Pete Williamson wrote:
> Do we need this?  I was thinking that the toast was created by adding the save
> page later request.

Yes, this is independent from Notification (which do get created when a request
is added). This is a self-timing "Downloading..." bubble which only has to be
shown, it takes care about disappearing itself. It basically just indicates that
download of a page started in the background. We do the same on Download button
presses.

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2528483003/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:554:
throttles->push_back(
On 2016/12/19 21:48:58, Pete Williamson wrote:
> Should the throttle be pushed back *before* we call
> AppendStandardResourceThrottles?

See the earlier comment by asanka on this CL. We need to do it in this order
because SafeBrowsing throttle is added as "Standard" and it should be able to
intercept the download before us.

[Pete Williamson, 2016-12-19 22:42:49]

lgtm

[Dmitry Titov, 2016-12-19 22:45:24]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-12-19 22:45:25]

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

[commit-bot: I haz the power, 2016-12-19 22:45:59]

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-12-19 22:48:23]

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

[commit-bot: I haz the power, 2016-12-19 22:48:24]

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

[Dmitry Titov, 2016-12-19 23:00:23]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-12-19 23:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
petewil@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2528483003/#ps160001
(title: "git merge resolved")

[commit-bot: I haz the power, 2016-12-19 23:00:51]

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-12-20 00:18:36]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482188423788590,
"parent_rev": "83dccfb1c310074c21c57e9c5b8a70744d494894", "commit_rev":
"01e9f8bc0c3e6a62a64b5cde6fdf7cc81f72b5b9"}

[commit-bot: I haz the power, 2016-12-20 00:19:08]

Description was changed from

==========
[Android Downloads] Long-press menu item "Download Link" should delegate job to
OfflinePages backend.
This patch interrupts the download and hands it off to the OfflinePage backend
once the mime type of the download is detected and it is text/html.
Other downloads not affected.

BUG=643731
==========

to

==========
[Android Downloads] Long-press menu item "Download Link" should delegate job to
OfflinePages backend.
This patch interrupts the download and hands it off to the OfflinePage backend
once the mime type of the download is detected and it is text/html.
Other downloads not affected.

BUG=643731

Review-Url: https://codereview.chromium.org/2528483003
==========

[commit-bot: I haz the power, 2016-12-20 00:19:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-20 00:22:51]

Description was changed from

==========
[Android Downloads] Long-press menu item "Download Link" should delegate job to
OfflinePages backend.
This patch interrupts the download and hands it off to the OfflinePage backend
once the mime type of the download is detected and it is text/html.
Other downloads not affected.

BUG=643731

Review-Url: https://codereview.chromium.org/2528483003
==========

to

==========
[Android Downloads] Long-press menu item "Download Link" should delegate job to
OfflinePages backend.
This patch interrupts the download and hands it off to the OfflinePage backend
once the mime type of the download is detected and it is text/html.
Other downloads not affected.

BUG=643731

Committed: https://crrev.com/4f6b28049376a0e309fda4cbeb6e4d7aac98965c
Cr-Commit-Position: refs/heads/master@{#439630}
==========

[commit-bot: I haz the power, 2016-12-20 00:22:53]

Patchset 9 (id:??) landed as
https://crrev.com/4f6b28049376a0e309fda4cbeb6e4d7aac98965c
Cr-Commit-Position: refs/heads/master@{#439630}