[Android] Width constrain bookmarks and downloads UIs

[Android] Width constrain bookmarks and downloads UIs

Unify download and bookmark UIs with  history UI by:

 * Width constrain to 600dp
 * Make tool shadow visible on tablets
 * Use list item background on all item views
 * Use MarginResizers to correctly position items in wide and regular
   displays
 * Account for navigation button when updating SelectableListToolbar
   display style
 * Hide downloads space display while searching and reduce padding
   above space display
 * Update DownloadHistoryAdapterTest to explicitly check for header

BUG=687640
Review-Url: https://codereview.chromium.org/2854643002
Cr-Commit-Position: refs/heads/master@{#468761}
Committed: https://chromium.googlesource.com/chromium/src/+/41405b15a8b1851ee473c6af6185ec49fce04eeb

[Theresa, 2017-05-01 17:23:24]

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

[commit-bot: I haz the power, 2017-05-01 17:23:47]

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-01 18:28:30]

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

[commit-bot: I haz the power, 2017-05-01 18:28:31]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-05-01 18:52:23]

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

[Theresa, 2017-05-01 18:52:24]

ptal

This CL is focused on visual consistency and ensuring correct behavior for M60.
I plan to do more refactoring (when time allows) to create some additional base
classes that remove the need for bookmarks/history/downloads to each set style
for their items.

[Theresa, 2017-05-01 20:12:24]

A link to screenshots of all three surfaces is in the bug.

[gone, 2017-05-02 17:49:49]

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/signin_and_sync_view.xml (right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/signin_and_sync_view.xml:11:
android:padding="@dimen/signin_and_sign_view_padding" >
signin_and_sync_view_padding

https://codereview.chromium.org/2854643002/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/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:160:
private UiConfig mUiConfig;
yank these two lines out into their own block?  the forced separation of the
annotation looks extra odd here with the comment.

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

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:306:
mHistoryAdapter.removeHeader();
Is this a permanent deal?  Does the header ever get re-added?

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:75:
/**
Reformat this so that there's a description instead of it all being in the
@return?

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:287:
public void setHasWideDisplayStyle() {
Can you think of a better name for this?  I got really confused thinking it
would take in a boolean to turn it on/off instead of initializing the display
style.

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:393:

no newline?

https://codereview.chromium.org/2854643002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java:67:
private static Integer sHeader = -1;
Leave a comment about where this -1 comes from?  Couldn't find where you change
it, so I'm assuming it's always set to -1 and I'm not sure what that means.

[Theresa, 2017-05-02 18:15:11]

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

[Theresa, 2017-05-02 18:15:26]

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/signin_and_sync_view.xml (right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/signin_and_sync_view.xml:11:
android:padding="@dimen/signin_and_sign_view_padding" >
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> signin_and_sync_view_padding

Done.

https://codereview.chromium.org/2854643002/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/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:160:
private UiConfig mUiConfig;
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> yank these two lines out into their own block?  the forced separation of the
> annotation looks extra odd here with the comment.

Done.

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

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:306:
mHistoryAdapter.removeHeader();
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> Is this a permanent deal?  Does the header ever get re-added?

The header gets re-added in DonwloadHistoryAdapter#filter() which is called when
the search is ended. It is explicitly removed here because it should be removed
as soon as the search is started (before the user types anything). I added a
comment.

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:75:
/**
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> Reformat this so that there's a description instead of it all being in the
> @return?

Done.

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:287:
public void setHasWideDisplayStyle() {
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> Can you think of a better name for this?  I got really confused thinking it
> would take in a boolean to turn it on/off instead of initializing the display
> style.

Renamed to "configureWideDisplayStyle"

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:393:

On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> no newline?

I like new lines after early returns. I removed the one after mIsSearching so
that there are fewer blocks.

https://codereview.chromium.org/2854643002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java
(right):

https://codereview.chromium.org/2854643002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java:67:
private static Integer sHeader = -1;
On 2017/05/02 17:49:48, slow (dfalcantara) wrote:
> Leave a comment about where this -1 comes from?  Couldn't find where you
change
> it, so I'm assuming it's always set to -1 and I'm not sure what that means.

I changed it to final. It doesn't change, and it means nothing. I can't use the
TYPE_HEADER int because findbugs complains about int to Object comparison, so
this is just a helper object to denote the header.

[commit-bot: I haz the power, 2017-05-02 18:15:51]

Dry run: CQ is trying da patch.

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

[gone, 2017-05-02 18:17:27]

lgtm

[commit-bot: I haz the power, 2017-05-02 19:03:29]

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

[commit-bot: I haz the power, 2017-05-02 19:03:30]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-05-02 20:17:59]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2017-05-02 20:18: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-05-02 20:24:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493756279063590,
"parent_rev": "a8da3b572593e1a166bc2e3a22821a17feb0af27", "commit_rev":
"41405b15a8b1851ee473c6af6185ec49fce04eeb"}

[commit-bot: I haz the power, 2017-05-02 20:24:58]

Description was changed from

==========
[Android] Width constrain bookmarks and downloads UIs

Unify download and bookmark UIs with  history UI by:

 * Width constrain to 600dp
 * Make tool shadow visible on tablets
 * Use list item background on all item views
 * Use MarginResizers to correctly position items in wide and regular
   displays
 * Account for navigation button when updating SelectableListToolbar
   display style
 * Hide downloads space display while searching and reduce padding
   above space display
 * Update DownloadHistoryAdapterTest to explicitly check for header

BUG=687640
==========

to

==========
[Android] Width constrain bookmarks and downloads UIs

Unify download and bookmark UIs with  history UI by:

 * Width constrain to 600dp
 * Make tool shadow visible on tablets
 * Use list item background on all item views
 * Use MarginResizers to correctly position items in wide and regular
   displays
 * Account for navigation button when updating SelectableListToolbar
   display style
 * Hide downloads space display while searching and reduce padding
   above space display
 * Update DownloadHistoryAdapterTest to explicitly check for header

BUG=687640

Review-Url: https://codereview.chromium.org/2854643002
Cr-Commit-Position: refs/heads/master@{#468761}
Committed:
https://chromium.googlesource.com/chromium/src/+/41405b15a8b1851ee473c6af6185...
==========

[commit-bot: I haz the power, 2017-05-02 20:24:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/41405b15a8b1851ee473c6af6185...