Download Home : Remove drawer UI

Download Home : Remove drawer UI

Removed navigation drawer from the download home. The drawer was replaced
by a spinner. Space display moved in-page as a header in the download
history adapter.

BUG=712912
Review-Url: https://codereview.chromium.org/2834453007
Cr-Commit-Position: refs/heads/master@{#467752}
Committed: https://chromium.googlesource.com/chromium/src/+/bdf153cfc2bce1eb1b22e0d00e0e3580d54b152d

[shaktisahu, 2017-04-21 21:03:43]

Description was changed from

==========
Download Home : Remove drawer UI

Removed navigation drawer from the download home. The drawer was replaced
by a spinner. Space display moved in-page as a header in the download
history adapter.

BUG=712912
==========

to

==========
Download Home : Remove drawer UI

Removed navigation drawer from the download home. The drawer was replaced
by a spinner. Space display moved in-page as a header in the download
history adapter.

BUG=712912
==========

[shaktisahu, 2017-04-21 21:03:43]

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

[shaktisahu, 2017-04-21 22:19:39]

Patchset #2 (id:20001) has been deleted

[shaktisahu, 2017-04-21 22:20:40]

PTAL.

[Theresa, 2017-04-24 20:48:18]

Thank you for taking this on!

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_main.xml:11:
android:background="@color/appbar_background"/>
The background color should just be primary_default now. appbar_background is
white on tablets.

This is actually the last legitimate use of appbar_background so I'll be
removing it soon.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:33:
android:layout_marginEnd="-2dp"
Why does these need -2dp margin end?

Will you please upload a couple of screenshots to the bug?

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:41: <color
name="black_38">#61000000</color>
We have similarly named colors above, e.g. black_alpha_30. Let's call these
black_alpha_87, etc. and either move the other alpha shades down or move these
up.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:183: <color
name="black_24">#3c000000</color>
#424242 isn't a valid text style. Let's try 87% black instead.

Instead of 24% black for the progress bar background, let's make it 54% black to
make the "available" text.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:333:
protected BasicViewHolder createHeader(ViewGroup parent) {
This may be called multiple times as views are recycled and re-used. We should
check that mSpaceDisplay == null before recreating it.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java
(left):

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:93:
if (mSelectedIndex == filter) return;
Is this early return not needed because the spinner will call
onNothingSelected() if the user taps on the item that's already selected?
\

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:95:
notifyDataSetChanged();
I think we can drop notifyDataSetChanged() since DownloadHistoryAdapter will
call loadItems() from it's onFilterChanged() observer which also calls
notifyDataSetChanged()

[shaktisahu, 2017-04-24 22:07:09]

Thanks. PTAL again.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_main.xml:11:
android:background="@color/appbar_background"/>
On 2017/04/24 20:48:17, Theresa wrote:
> The background color should just be primary_default now. appbar_background is
> white on tablets.
> 
> This is actually the last legitimate use of appbar_background so I'll be
> removing it soon.

Done.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:33:
android:layout_marginEnd="-2dp"
On 2017/04/24 20:48:17, Theresa wrote:
> Why does these need -2dp margin end?
> 
> Will you please upload a couple of screenshots to the bug?

Attached screenshots. The margin in the spec is 14dp while the margin of the
containing Linearlayout is downloads_item_margin, i.e. 16dp.
So I needed -2dp here. Let me know if you think we have to tweak this.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:41: <color
name="black_38">#61000000</color>
On 2017/04/24 20:48:17, Theresa wrote:
> We have similarly named colors above, e.g. black_alpha_30. Let's call these
> black_alpha_87, etc. and either move the other alpha shades down or move these
> up.

Done.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:183: <color
name="black_24">#3c000000</color>
On 2017/04/24 20:48:17, Theresa wrote:
> #424242 isn't a valid text style. Let's try 87% black instead.
> 
> Instead of 24% black for the progress bar background, let's make it 54% black
to
> make the "available" text.

Done. 

Changed #424242 to 87% black.
For the progressbar background, changed 24% black to use the existing 30% black
instead. 54% black is too dark for this.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:333:
protected BasicViewHolder createHeader(ViewGroup parent) {
On 2017/04/24 20:48:17, Theresa wrote:
> This may be called multiple times as views are recycled and re-used. We should
> check that mSpaceDisplay == null before recreating it.

I think this will be called only once during the creation i.e. after the call to
loadItems(). Next time onwards onBindViewHolder will be called which should take
care of the binding. HistoryAdapter#createHeader also acts likewise.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java
(left):

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:93:
if (mSelectedIndex == filter) return;
On 2017/04/24 20:48:17, Theresa wrote:
> Is this early return not needed because the spinner will call
> onNothingSelected() if the user taps on the item that's already selected?
> \

Done. 
Removed.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:95:
notifyDataSetChanged();
On 2017/04/24 20:48:17, Theresa wrote:
> I think we can drop notifyDataSetChanged() since DownloadHistoryAdapter will
> call loadItems() from it's onFilterChanged() observer which also calls
> notifyDataSetChanged()

Done. That seems correct. Thanks!

[Theresa, 2017-04-24 22:45:16]

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:33:
android:layout_marginEnd="-2dp"
On 2017/04/24 22:07:09, shaktisahu wrote:
> On 2017/04/24 20:48:17, Theresa wrote:
> > Why does these need -2dp margin end?
> > 
> > Will you please upload a couple of screenshots to the bug?
> 
> Attached screenshots. The margin in the spec is 14dp while the margin of the
> containing Linearlayout is downloads_item_margin, i.e. 16dp.
> So I needed -2dp here. Let me know if you think we have to tweak this.

Let's just change the LinearLayout margin to 14dp.

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:183: <color
name="black_24">#3c000000</color>
On 2017/04/24 22:07:09, shaktisahu wrote:
> On 2017/04/24 20:48:17, Theresa wrote:
> > #424242 isn't a valid text style. Let's try 87% black instead.
> > 
> > Instead of 24% black for the progress bar background, let's make it 54%
black
> to
> > make the "available" text.
> 
> Done. 
> 
> Changed #424242 to 87% black.
> For the progressbar background, changed 24% black to use the existing 30%
black
> instead. 54% black is too dark for this.

30% black isn't a valid text color either. We shouldn't introduce new uses of
text colors that don't match the defined list even if the color is currently
being used. 38% black might work. If not, we need to ask cleer@ and hannahs@ to
pick a color that's in our set of styles or update the set to include a new
color that's acceptable to use here.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:333:
protected BasicViewHolder createHeader(ViewGroup parent) {
On 2017/04/24 22:07:09, shaktisahu wrote:
> On 2017/04/24 20:48:17, Theresa wrote:
> > This may be called multiple times as views are recycled and re-used. We
should
> > check that mSpaceDisplay == null before recreating it.
> 
> I think this will be called only once during the creation i.e. after the call
to
> loadItems(). Next time onwards onBindViewHolder will be called which should
take
> care of the binding. HistoryAdapter#createHeader also acts likewise.

We can loadItems() multiple times, though. That method is called every time the
filter changes. 

I also think that the whole nature of a RecyclerView is to free memory for items
that aren't being used. If you scroll down really far, the header viewhold will
eventually get freed. When you scroll back up, it will need to be recreated.

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:40: <color
name="black_alpha_38">#61000000</color>
Let's order these by ascending alpha value:

black_alpha_30
black_alpha_38
black_alpha_40
etc.

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java
(right):

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:78:
}
nit: this curly brace should be on the same line as the method definition.

[shaktisahu, 2017-04-25 21:14:26]

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:33:
android:layout_marginEnd="-2dp"
On 2017/04/24 22:45:15, Theresa wrote:
> On 2017/04/24 22:07:09, shaktisahu wrote:
> > On 2017/04/24 20:48:17, Theresa wrote:
> > > Why does these need -2dp margin end?
> > > 
> > > Will you please upload a couple of screenshots to the bug?
> > 
> > Attached screenshots. The margin in the spec is 14dp while the margin of the
> > containing Linearlayout is downloads_item_margin, i.e. 16dp.
> > So I needed -2dp here. Let me know if you think we have to tweak this.
> 
> Let's just change the LinearLayout margin to 14dp.

Done.

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

https://codereview.chromium.org/2834453007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:333:
protected BasicViewHolder createHeader(ViewGroup parent) {
On 2017/04/24 22:45:16, Theresa wrote:
> On 2017/04/24 22:07:09, shaktisahu wrote:
> > On 2017/04/24 20:48:17, Theresa wrote:
> > > This may be called multiple times as views are recycled and re-used. We
> should
> > > check that mSpaceDisplay == null before recreating it.
> > 
> > I think this will be called only once during the creation i.e. after the
call
> to
> > loadItems(). Next time onwards onBindViewHolder will be called which should
> take
> > care of the binding. HistoryAdapter#createHeader also acts likewise.
> 
> We can loadItems() multiple times, though. That method is called every time
the
> filter changes. 
> 
> I also think that the whole nature of a RecyclerView is to free memory for
items
> that aren't being used. If you scroll down really far, the header viewhold
will
> eventually get freed. When you scroll back up, it will need to be recreated.

Done.

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:40: <color
name="black_alpha_38">#61000000</color>
On 2017/04/24 22:45:16, Theresa wrote:
> Let's order these by ascending alpha value:
> 
> black_alpha_30
> black_alpha_38
> black_alpha_40
> etc.

Done.

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java
(right):

https://codereview.chromium.org/2834453007/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java:78:
}
On 2017/04/24 22:45:16, Theresa wrote:
> nit: this curly brace should be on the same line as the method definition.

Done.

[Theresa, 2017-04-25 21:48:50]

lgtm

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/download_manager_ui_space_widget.xml (left):

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:45:
android:textColor="@color/google_grey_600"
I was thinking we could keep the colors exactly the same (use google_grey_600,
google_grey_400, and light_active_color) until we hear from UX what they'd like
the colors to be. But I'm also okay with the change as is.

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

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:435:
unregisterAdapterDataObserver(mSpaceDisplay);
We should probably have a mSpaceDisplay != null check here in case the manager
has no items (no header created) and is then destroyed.

[shaktisahu, 2017-04-25 22:02:38]

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

[commit-bot: I haz the power, 2017-04-25 22:03:25]

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-04-25 22:08:54]

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

[commit-bot: I haz the power, 2017-04-25 22:08:56]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[shaktisahu, 2017-04-25 22:18:28]

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

[commit-bot: I haz the power, 2017-04-25 22:18:52]

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-04-25 22:24:14]

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

[shaktisahu, 2017-04-25 22:24:15]

+nyquist for owners approval on android_chrome_strings.grd and DownloadPage.java

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/download_manager_ui_space_widget.xml (left):

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/download_manager_ui_space_widget.xml:45:
android:textColor="@color/google_grey_600"
On 2017/04/25 21:48:49, Theresa wrote:
> I was thinking we could keep the colors exactly the same (use google_grey_600,
> google_grey_400, and light_active_color) until we hear from UX what they'd
like
> the colors to be. But I'm also okay with the change as is.

I would wait to hear from the UX.

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

https://codereview.chromium.org/2834453007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:435:
unregisterAdapterDataObserver(mSpaceDisplay);
On 2017/04/25 21:48:49, Theresa wrote:
> We should probably have a mSpaceDisplay != null check here in case the manager
> has no items (no header created) and is then destroyed.

Done. Thanks!

[commit-bot: I haz the power, 2017-04-25 22:42:33]

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

[commit-bot: I haz the power, 2017-04-25 22:42:33]

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

[nyquist, 2017-04-25 23:19:38]

those two files lgtm

[shaktisahu, 2017-04-26 01:42:30]

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

[commit-bot: I haz the power, 2017-04-26 01:43:15]

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-04-26 03:18:43]

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

[commit-bot: I haz the power, 2017-04-26 03:18:44]

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-04-26 05:17:32]

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

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

Dry run: CQ is trying da patch.

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

[shaktisahu, 2017-04-26 05:18:06]

Patchset #7 (id:140001) has been deleted

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

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

[commit-bot: I haz the power, 2017-04-26 05:57:51]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-04-27 18:07:09]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-04-27 18:07:10]

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

[commit-bot: I haz the power, 2017-04-27 18:07:34]

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-27 19:34:54]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493316429854010,
"parent_rev": "9ff354462a0bd48001bc59aef6910447b3b7af7a", "commit_rev":
"bdf153cfc2bce1eb1b22e0d00e0e3580d54b152d"}

[commit-bot: I haz the power, 2017-04-27 19:35:14]

Description was changed from

==========
Download Home : Remove drawer UI

Removed navigation drawer from the download home. The drawer was replaced
by a spinner. Space display moved in-page as a header in the download
history adapter.

BUG=712912
==========

to

==========
Download Home : Remove drawer UI

Removed navigation drawer from the download home. The drawer was replaced
by a spinner. Space display moved in-page as a header in the download
history adapter.

BUG=712912

Review-Url: https://codereview.chromium.org/2834453007
Cr-Commit-Position: refs/heads/master@{#467752}
Committed:
https://chromium.googlesource.com/chromium/src/+/bdf153cfc2bce1eb1b22e0d00e0e...
==========

[commit-bot: I haz the power, 2017-04-27 19:35:15]

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