Download home : Added info menu icon

Download home : Added info menu icon

Added an info menu icon to the download home toolbar which will toggle
the visibility of disk space usage info header. This info is disabled
by default at the start of download home.

Also added back the icons for the download filter types.

Also fixed a NullPointerException in the filter adapter.

BUG=720141, 717495
Review-Url: https://codereview.chromium.org/2874563002
Cr-Commit-Position: refs/heads/master@{#471840}
Committed: https://chromium.googlesource.com/chromium/src/+/62a0b7c64ca4191c494bf2fde98e8a9a72ff57fa

[shaktisahu, 2017-05-09 22:55:01]

shaktisahu@chromium.org changed reviewers:
+ twellington@chromium.org

[shaktisahu, 2017-05-09 22:55:01]

PTAL

[Theresa, 2017-05-09 23:32:23]

Looks good overall, thank you!

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
File chrome/android/java/res/menu/download_manager_menu.xml (right):

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
chrome/android/java/res/menu/download_manager_menu.xml:13:
android:title="@string/info"
The title is used as fallback when there's not room to display the action. and
as the content description for talk back. We should have a more descriptive
title, e.g. "Show size display" and "Hide size display". srahim@ is our UX
string writer, so she can probably advise.

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

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:164:
private boolean mShowSpaceDisplayHeader;
nit: mShowingSpaceDisplayHeader or mShouldShowSpaceDisplayHeader.

[Theresa, 2017-05-10 00:05:44]

I just remembered that the (i) toggle should be sticky. Once the user hides it,
it should stay hidden when the user exits and reopens Chrome/downloads until the
use hits the (i) again. Jon had also talked about experimenting with whether the
space display is showing by default or not. We should ask if he wants an
experiment flag for that.

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
File chrome/android/java/res/menu/download_manager_menu.xml (right):

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
chrome/android/java/res/menu/download_manager_menu.xml:13:
android:title="@string/info"
On 2017/05/09 23:32:23, Theresa wrote:
> The title is used as fallback when there's not room to display the action. and
> as the content description for talk back. We should have a more descriptive
> title, e.g. "Show size display" and "Hide size display". srahim@ is our UX
> string writer, so she can probably advise.

Another potential is "Show info" and "Hide info", which could be shared with
history.

[shaktisahu, 2017-05-10 04:37:41]

Description was changed from

==========
Download home : Added info menu icon

Added an info menu icon to the download home toolbar which will toggle
the visibility of disk space usage info header. This info is disabled
by default at the start of download home.

Also added back the icons for the download filter types.

BUG=720141
==========

to

==========
Download home : Added info menu icon

Added an info menu icon to the download home toolbar which will toggle
the visibility of disk space usage info header. This info is disabled
by default at the start of download home.

Also added back the icons for the download filter types.

Also fixed a NullPointerException in the filter adapter.

BUG=720141, 717495
==========

[shaktisahu, 2017-05-11 19:55:47]

Thanks for the feedback. I have added a shared pref to save the user selection
on the toggle button. Also added a feature for experiment and a corresponding
UMA histogram.

PTAL

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
File chrome/android/java/res/menu/download_manager_menu.xml (right):

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/res/men...
chrome/android/java/res/menu/download_manager_menu.xml:13:
android:title="@string/info"
On 2017/05/10 00:05:44, Theresa wrote:
> On 2017/05/09 23:32:23, Theresa wrote:
> > The title is used as fallback when there's not room to display the action.
and
> > as the content description for talk back. We should have a more descriptive
> > title, e.g. "Show size display" and "Hide size display". srahim@ is our UX
> > string writer, so she can probably advise.
> 
> Another potential is "Show info" and "Hide info", which could be shared with
> history.
> 

While waiting on an updated string request, I have added Show info / Hide info
for now.

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

https://codereview.chromium.org/2874563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:164:
private boolean mShowSpaceDisplayHeader;
On 2017/05/09 23:32:23, Theresa wrote:
> nit: mShowingSpaceDisplayHeader or mShouldShowSpaceDisplayHeader.

Done.

[Theresa, 2017-05-11 22:14:46]

lgtm. Thank you!

https://codereview.chromium.org/2874563002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2874563002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:656: +    the storage info.
nit: Let's document that "enabled" is recorded when the user shows the storage
info and "disabled" when the user hides the storage info. Alternatively, we
could add a new BooleanShown to metrics/histograms/enums.xml that uses "Hidden"
and "Shown" as labels.

[shaktisahu, 2017-05-11 22:35:28]

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

[commit-bot: I haz the power, 2017-05-11 22:36:26]

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-05-12 00:02:12]

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

[shaktisahu, 2017-05-12 00:02:49]

shaktisahu@chromium.org changed reviewers:
+ nyquist@chromium.org

[shaktisahu, 2017-05-12 00:03:24]

shaktisahu@chromium.org changed reviewers:
+ isherman@chromium.org

[commit-bot: I haz the power, 2017-05-12 00:04:23]

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-05-12 00:04:23]

nyquist@ - PTAL at android_chrome_strings.grd file
isherman@ - PTAL at histograms.xml

[Ilya Sherman, 2017-05-12 00:08:27]

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:652: +<histogram
name="Android.DownloadManager.StorageInfo" enum="BooleanEnabled">
nit: I'd name this histogram "Android.DownloadManager.ShowStorageInfo"

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:657: +    when it is hidden.
I think this second sentence is describing how to interpret the enum labels?  If
so, why not just use an enum with clearer labels?

[shaktisahu, 2017-05-12 00:34:08]

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:652: +<histogram
name="Android.DownloadManager.StorageInfo" enum="BooleanEnabled">
On 2017/05/12 00:08:27, Ilya Sherman wrote:
> nit: I'd name this histogram "Android.DownloadManager.ShowStorageInfo"

Done.

https://codereview.chromium.org/2874563002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:657: +    when it is hidden.
On 2017/05/12 00:08:27, Ilya Sherman wrote:
> I think this second sentence is describing how to interpret the enum labels? 
If
> so, why not just use an enum with clearer labels?

Done. Using BooleanVisible.

[Ilya Sherman, 2017-05-12 06:27:05]

Metrics LGTM, thanks.

[nyquist, 2017-05-12 06:45:31]

chrome/android/java/strings/android_chrome_strings.grd lgtm

[shaktisahu, 2017-05-12 06:47:54]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-05-12 06:47:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/2874563002/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2017-05-12 06:48:13]

CQ is trying da patch.

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

[shaktisahu, 2017-05-12 06:48:16]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2017-05-12 06:48:36]

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

[commit-bot: I haz the power, 2017-05-12 06:49:04]

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, 2017-05-12 07:30:17]

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

[commit-bot: I haz the power, 2017-05-12 07:30:19]

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

[shaktisahu, 2017-05-14 04:49:59]

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

[commit-bot: I haz the power, 2017-05-14 04:50:05]

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, 2017-05-14 06:55:56]

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

[commit-bot: I haz the power, 2017-05-14 06:55:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shaktisahu, 2017-05-14 20:01:07]

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

[commit-bot: I haz the power, 2017-05-14 20:01:14]

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-05-14 20:05:00]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2017-05-14 20:07:29]

Patchset #8 (id:140001) has been deleted

[shaktisahu, 2017-05-14 20:08:16]

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

[commit-bot: I haz the power, 2017-05-14 20:08:32]

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-05-14 20:08:45]

Patchset #7 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-05-14 22:12:13]

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

[commit-bot: I haz the power, 2017-05-14 22:12:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[shaktisahu, 2017-05-15 16:54:31]

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

[commit-bot: I haz the power, 2017-05-15 16:55:04]

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, 2017-05-15 17:56:19]

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

[commit-bot: I haz the power, 2017-05-15 17:56:20]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-05-15 18:20:37]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-05-15 18:20:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, isherman@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2874563002/#ps160001
(title: "Fixed tests")

[commit-bot: I haz the power, 2017-05-15 18:21: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-05-15 18:35:35]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494872437022720,
"parent_rev": "4d4db58f0615aa162f870821f1bba6815a8b8dd3", "commit_rev":
"62a0b7c64ca4191c494bf2fde98e8a9a72ff57fa"}

[commit-bot: I haz the power, 2017-05-15 18:35:49]

Description was changed from

==========
Download home : Added info menu icon

Added an info menu icon to the download home toolbar which will toggle
the visibility of disk space usage info header. This info is disabled
by default at the start of download home.

Also added back the icons for the download filter types.

Also fixed a NullPointerException in the filter adapter.

BUG=720141, 717495
==========

to

==========
Download home : Added info menu icon

Added an info menu icon to the download home toolbar which will toggle
the visibility of disk space usage info header. This info is disabled
by default at the start of download home.

Also added back the icons for the download filter types.

Also fixed a NullPointerException in the filter adapter.

BUG=720141, 717495

Review-Url: https://codereview.chromium.org/2874563002
Cr-Commit-Position: refs/heads/master@{#471840}
Committed:
https://chromium.googlesource.com/chromium/src/+/62a0b7c64ca4191c494bf2fde98e...
==========

[commit-bot: I haz the power, 2017-05-15 18:35:54]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/62a0b7c64ca4191c494bf2fde98e...