[Downloads] Expose basic download history to UI

[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324
Committed: https://crrev.com/194e406dedb9ce73fdf698deb4ec6d082931299e
Cr-Commit-Position: refs/heads/master@{#405607}

[gone, 2016-07-14 00:49:08]

Description was changed from

==========
Download backend

BUG=
==========

to

==========
[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324
==========

[gone, 2016-07-14 00:49:59]

dfalcantara@chromium.org changed reviewers:
+ ianwen@chromium.org, qinmin@chromium.org

[gone, 2016-07-14 00:50:01]

Min: What do you think about the calls I've added here to the
DownloadManagerService?  We'll have to add more as the Download Home
functionality is fleshed out.

Ian: What do you think about the Adapter and other classes?

[qinmin, 2016-07-14 17:08:05]

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:96: JNIEnv* env =
base::android::AttachCurrentThread();
Since we always call onAllDownloadsRetrieved, can we just skipping creating the
empty list and pass an array of objects from native to java, and create the list
in java at that time?

[gone, 2016-07-14 17:11:08]

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:96: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/07/14 17:08:05, qinmin wrote:
> Since we always call onAllDownloadsRetrieved, can we just skipping creating
the
> empty list and pass an array of objects from native to java, and create the
list
> in java at that time?

This is actually what the DownloadsBridge does, so we're just mimicking what's
going on there.

But how would you actually pass the objects?  Would we create 4 jstring arrays
and 2 jlong arrays?  Is that a thing we can do?

[qinmin, 2016-07-14 18:06:54]

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:96: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/07/14 17:11:08, dfalcantara wrote:
> On 2016/07/14 17:08:05, qinmin wrote:
> > Since we always call onAllDownloadsRetrieved, can we just skipping creating
> the
> > empty list and pass an array of objects from native to java, and create the
> list
> > in java at that time?
> 
> This is actually what the DownloadsBridge does, so we're just mimicking what's
> going on there.
> 
> But how would you actually pass the objects?  Would we create 4 jstring arrays
> and 2 jlong arrays?  Is that a thing we can do?

You can call a jni call to pack the fields into a jobject and return it, so at
the end, just an array of jobjects

[Ian Wen, 2016-07-14 18:16:26]

Awesome change!! I appreciate the speed and the quality!

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/download_history_adapter_view.xml (right):

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
chrome/android/java/res/layout/download_history_adapter_view.xml:8:
xmlns:android="http://schemas.android.com/apk/res/android"
Possibly simplify our namings? Such as download_item_view?

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/download_main.xml (right):

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
chrome/android/java/res/layout/download_main.xml:12: <RelativeLayout
This layout could be simplified to LinearLayout. You might need a trick such as
marginTop="-10dp" in shadow so that it does not take vertical space.

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:119:
private static int convertMimeTypeToFileType(String mimeType) {
I don't see this function used in other places. Shall we change it to
getIconForFileType()? This way we could remove the switch() in #86.

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:224:
getDownloadManagerService().getAllDownloads();
Nit: Shall we inline this function instead?

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/browser/a...
File chrome/browser/android/download/download_manager_service.cc (right):

https://chromiumcodereview.appspot.com/2143303002/diff/20001/chrome/browser/a...
chrome/browser/android/download/download_manager_service.cc:96: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/07/14 17:11:08, dfalcantara wrote:
> On 2016/07/14 17:08:05, qinmin wrote:
> > Since we always call onAllDownloadsRetrieved, can we just skipping creating
> the
> > empty list and pass an array of objects from native to java, and create the
> list
> > in java at that time?
> 
> This is actually what the DownloadsBridge does, so we're just mimicking what's
> going on there.
> 
> But how would you actually pass the objects?  Would we create 4 jstring arrays
> and 2 jlong arrays?  Is that a thing we can do?

Aha! I found a trick that BookmarkBridge did to simplify the code by a tiny
margin: in GetAllDownloas(), java side passes a java list directly to native and
let it cache it for returning.

This is a nit though. orz

[gone, 2016-07-14 19:11:57]

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/download_history_adapter_view.xml (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/download_history_adapter_view.xml:8:
xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/07/14 18:16:25, Ian Wen wrote:
> Possibly simplify our namings? Such as download_item_view?

Eh, I'm leaning towards keeping it this way because it's part of the
DownloadHistoryAdapter.  Do you feel strongly about it?

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/download_main.xml (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/download_main.xml:12: <RelativeLayout
On 2016/07/14 18:16:25, Ian Wen wrote:
> This layout could be simplified to LinearLayout. You might need a trick such
as
> marginTop="-10dp" in shadow so that it does not take vertical space.

Done.

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:119:
private static int convertMimeTypeToFileType(String mimeType) {
On 2016/07/14 18:16:25, Ian Wen wrote:
> I don't see this function used in other places. Shall we change it to
> getIconForFileType()? This way we could remove the switch() in #86.

We're going to need it (soonish) for figuring out whether we want to kick off a
thumbnail fetch from native.

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:224:
getDownloadManagerService().getAllDownloads();
On 2016/07/14 18:16:25, Ian Wen wrote:
> Nit: Shall we inline this function instead?

Done.

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:96: JNIEnv* env =
base::android::AttachCurrentThread();
On 2016/07/14 18:16:26, Ian Wen wrote:
> On 2016/07/14 17:11:08, dfalcantara wrote:
> > On 2016/07/14 17:08:05, qinmin wrote:
> > > Since we always call onAllDownloadsRetrieved, can we just skipping
creating
> > the
> > > empty list and pass an array of objects from native to java, and create
the
> > list
> > > in java at that time?
> > 
> > This is actually what the DownloadsBridge does, so we're just mimicking
what's
> > going on there.
> > 
> > But how would you actually pass the objects?  Would we create 4 jstring
arrays
> > and 2 jlong arrays?  Is that a thing we can do?
> 
> Aha! I found a trick that BookmarkBridge did to simplify the code by a tiny
> margin: in GetAllDownloas(), java side passes a java list directly to native
and
> let it cache it for returning.
> 
> This is a nit though. orz

Changed it around to create the jobjectarray like Min suggested; lmk if this
looks right.

[qinmin, 2016-07-14 19:48:18]

lgtm % comments

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:111:
ScopedJavaLocalRef<jclass> j_download_item_class = base::android::GetClass(
This looks a little worse than the previous patch, i forgot to take jni array
creation into consideration. 
Sorry for all the inconvenience, revert this to the old patch would be better.

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:185:
GetAllDownloadsInternal();
Do you need to check whether user has closed the download home activity at this
moment?

[Ian Wen, 2016-07-14 20:13:50]

lgtm

https://codereview.chromium.org/2143303002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/download_history_adapter_view.xml (right):

https://codereview.chromium.org/2143303002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_history_adapter_view.xml:7:
<RelativeLayout
Nit: Shall we shorten the filename a bit?

[gone, 2016-07-14 20:22:58]

https://codereview.chromium.org/2143303002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/download_history_adapter_view.xml (right):

https://codereview.chromium.org/2143303002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_history_adapter_view.xml:7:
<RelativeLayout
On 2016/07/14 20:13:50, Ian Wen wrote:
> Nit: Shall we shorten the filename a bit?

Done.

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:111:
ScopedJavaLocalRef<jclass> j_download_item_class = base::android::GetClass(
On 2016/07/14 19:48:18, qinmin wrote:
> This looks a little worse than the previous patch, i forgot to take jni array
> creation into consideration. 
> Sorry for all the inconvenience, revert this to the old patch would be better.

No worries; I reverted most of it, but kept the function for
ShouldShowDownloadItem.  It might be helpful later.

https://codereview.chromium.org/2143303002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:185:
GetAllDownloadsInternal();
On 2016/07/14 19:48:18, qinmin wrote:
> Do you need to check whether user has closed the download home activity at
this
> moment?

Should be fine: the Adapter is owned by the DownloadManagerService, so checking
if that java_ref is valid is OK.

[gone, 2016-07-14 20:23:02]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-07-14 20:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2143303002/#ps80001
(title: "Renamed file")

[commit-bot: I haz the power, 2016-07-14 20:23:45]

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

[gone, 2016-07-14 20:27:00]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-07-14 20:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2143303002/#ps100001
(title: "Lint")

[commit-bot: I haz the power, 2016-07-14 20:27:48]

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-07-14 22:34:41]

Description was changed from

==========
[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324
==========

to

==========
[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324
==========

[commit-bot: I haz the power, 2016-07-14 22:34:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-14 22:34:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 22:36:44]

Description was changed from

==========
[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324
==========

to

==========
[Downloads] Expose basic download history to UI

* Adds hooks in DownloadManagerService for calling GetAllDownloads()
  and sending the information back to Java.  This creates a
  List of DownloadItems that contain the basic information needed
  for the UI.

* Adds a basic RecyclerView to display the files in the user's
  download history.

* Adds a DownloadHistoryAdapter that is used to track information
  about the download history.  This *should* be reusable among
  multiple RecyclerViews in case the user opens up multi-window
  mode.

- This currently doesn't monitor any changes to the history and is
  used solely for getting a skeleton working.

BUG=616324

Committed: https://crrev.com/194e406dedb9ce73fdf698deb4ec6d082931299e
Cr-Commit-Position: refs/heads/master@{#405607}
==========

[commit-bot: I haz the power, 2016-07-14 22:36:45]

Patchset 6 (id:??) landed as
https://crrev.com/194e406dedb9ce73fdf698deb4ec6d082931299e
Cr-Commit-Position: refs/heads/master@{#405607}