📰Open Downloads suggestions externally when appropriate

[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212
Committed: https://crrev.com/0fb174ee48dca5a03dc244670cf394fd1f544608
Cr-Commit-Position: refs/heads/master@{#429721}

[dgn, 2016-11-02 13:59:01]

Description was changed from

==========
[NTP Client][WIP] Open Downloads suggestions externally when appropriate

BUG=661212
==========

to

==========
[NTP Client][WIP] Open Downloads suggestions externally when appropriate

BUG=661212
==========

[dgn, 2016-11-02 17:41:25]

dgn@chromium.org changed reviewers:
+ dfalcantara@chromium.org, tschumann@chromium.org

[dgn, 2016-11-02 17:41:26]

PTAL. Very WIP, but we don't have all the necessary info from our backend yet.
Coming soon. In the meantime, can you please validate the changes to
//.../download?

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:352: new
File(article.mUrl), "unused", "application/epub+zip", false);
tschumann@ is working on exposing the info we need (proper file path and mime
type). what is the ID used for? it's passed to determine if Chrome supports
opening the file, but since it's currently unused for now, I'm not sure what is
expected here.

[gone, 2016-11-03 00:39:45]

It's about what I was expecting from a high level.  I'd move things around,
though.

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1039:
File file, String id, String plainMimeType, boolean isOffTheRecord) {
This should probably go into DownloadUtils instead of DownloadManagerService.

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1045:
if
(DownloadManagerService.getDownloadManagerService(context).isDownloadOpenableInBrowser(
You're already in the DownloadManagerService, so you shouldn't need to ask the
DownloadManagerService to get itself.

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:352: new
File(article.mUrl), "unused", "application/epub+zip", false);
On 2016/11/02 17:41:25, dgn wrote:
> tschumann@ is working on exposing the info we need (proper file path and mime
> type). what is the ID used for? it's passed to determine if Chrome supports
> opening the file, but since it's currently unused for now, I'm not sure what
is
> expected here.

The ID used to be required to ask the native DownloadManager about the MIME
type; if it's not currently being used then you could probably just take it out
of the function params.

[dgn, 2016-11-03 19:58:00]

Description was changed from

==========
[NTP Client][WIP] Open Downloads suggestions externally when appropriate

BUG=661212
==========

to

==========
[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212
==========

[dgn, 2016-11-03 19:58:14]

PTAL

[dgn, 2016-11-03 19:59:08]

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1039:
File file, String id, String plainMimeType, boolean isOffTheRecord) {
On 2016/11/03 00:39:45, dfalcantara (slow 10.24) wrote:
> This should probably go into DownloadUtils instead of DownloadManagerService.

Done.

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1045:
if
(DownloadManagerService.getDownloadManagerService(context).isDownloadOpenableInBrowser(
On 2016/11/03 00:39:45, dfalcantara (slow 10.24) wrote:
> You're already in the DownloadManagerService, so you shouldn't need to ask the
> DownloadManagerService to get itself.

Now in DownloadUtils, so it's needed.

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2469303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:352: new
File(article.mUrl), "unused", "application/epub+zip", false);
On 2016/11/03 00:39:45, dfalcantara (slow 10.24) wrote:
> On 2016/11/02 17:41:25, dgn wrote:
> > tschumann@ is working on exposing the info we need (proper file path and
mime
> > type). what is the ID used for? it's passed to determine if Chrome supports
> > opening the file, but since it's currently unused for now, I'm not sure what
> is
> > expected here.
> 
> The ID used to be required to ask the native DownloadManager about the MIME
> type; if it's not currently being used then you could probably just take it
out
> of the function params.

Okay, removed it.

[dgn, 2016-11-03 19:59:14]

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

[commit-bot: I haz the power, 2016-11-03 19:59:58]

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

[gone, 2016-11-03 20:01:58]

lgtm

https://codereview.chromium.org/2469303002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2469303002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:74:
/** The downloaded asset (only for download asset articles).*/
nit: add a space before the */ to separate it out (here and other nearby member
variables)

[commit-bot: I haz the power, 2016-11-03 21:17:52]

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

[commit-bot: I haz the power, 2016-11-03 21:17:53]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-11-03 22:11:38]

Thanks!

https://codereview.chromium.org/2469303002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2469303002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:74:
/** The downloaded asset (only for download asset articles).*/
On 2016/11/03 20:01:58, dfalcantara (slow 10.24) wrote:
> nit: add a space before the */ to separate it out (here and other nearby
member
> variables)

Done.

[dgn, 2016-11-03 22:11:41]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-11-03 22:11:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2469303002/#ps80001
(title: "whitespace")

[commit-bot: I haz the power, 2016-11-03 22:13:00]

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-03 23:14:10]

Description was changed from

==========
[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212
==========

to

==========
[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-03 23:18:44]

Description was changed from

==========
[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212
==========

to

==========
[NTP Client] Open Downloads suggestions externally when appropriate

BUG=661212

Committed: https://crrev.com/0fb174ee48dca5a03dc244670cf394fd1f544608
Cr-Commit-Position: refs/heads/master@{#429721}
==========

[commit-bot: I haz the power, 2016-11-03 23:18:46]

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