[Home] Toolbar shadow v1

[Home] Toolbar shadow v1

Add a basic implementation for the toolbar shadow.
On the SuggestionsBottomSheetContent, a new FadingShadowView
shows up on scroll. The toolbar's shadow is currently transitioned
when the omnibox is focused/unfocused. That logic has been left in
place (for now), but the unfocused drawable is an empty shape as
there should not be a shadow when the omnibox is unfocused.

History/downloads/bookmarks all use existing logic in
SelectableListLayout that determines when their toolbar shadow
should be shown.

BUG=705728
Review-Url: https://codereview.chromium.org/2780693002
Cr-Commit-Position: refs/heads/master@{#460246}
Committed: https://chromium.googlesource.com/chromium/src/+/9c82d662ed9d8c1a2a22702e43bdc6d6a3a67972

[Theresa, 2017-03-27 23:03:30]

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

[commit-bot: I haz the power, 2017-03-27 23:04:11]

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

[Theresa, 2017-03-27 23:07:11]

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

[Theresa, 2017-03-27 23:07:11]

ptal -- video is in the bug

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/selectable_list_layout.xml (left):

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/selectable_list_layout.xml:49: </FrameLayout>
nit: add this space back

[commit-bot: I haz the power, 2017-03-28 00:50:49]

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

[commit-bot: I haz the power, 2017-03-28 00:50:50]

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

[mdjones, 2017-03-28 02:07:17]

lgtm

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:96:
boolean shadowVisible = mRecyclerView.computeVerticalScrollOffset() != 0;
It might be cheaper to call canScrollVertically(-1) anywhere we use
computeVerticalScrollOffset(). It's not something that needs to change here, I
just stumbled upon it.

[dgn, 2017-03-28 09:22:35]

drive by: For consistency's sake, should there be a TODO to move the shadow to
the toolbar itself? 

The regular toolbar takes care of the shadow, I guess it would make sense for
that to happen in the bottom one too. AFAICT you didn't implement it that way
just because the other views already had a shadow since they were in a separate
activity and need to keep supporting that until Home rolls out 100%

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:41:
private FadingShadowView mShadowView;
nit: why not final?

[Theresa, 2017-03-28 16:13:18]

On 2017/03/28 09:22:35, dgn wrote:
> drive by: For consistency's sake, should there be a TODO to move the shadow to
> the toolbar itself? 

There's a TODO in bottom_toolbar_shadow to revisit this after UX decides on what
the omnibox un/focus animation should like. This is a v1 implementation of a
shadow
to get us to user testing soon.

> The regular toolbar takes care of the shadow, I guess it would make sense for
> that to happen in the bottom one too. AFAICT you didn't implement it that way
> just because the other views already had a shadow since they were in a
separate
> activity and need to keep supporting that until Home rolls out 100%

I think ideally there would be one shadow shared by all CH panes.
This is unfortunately complicated by the need for history/bookmarks/downloads to
indefinitely need to own and manage a separate shadow unless we do a complete
tablet redesign. We do have the option to force hide their shadows and use a
shared CH shadow, though.

The fading shadow added to SuggestionsBottomSheetContent could (and probably
should)
long-term be owned by BottomSheet. The toolbar doesn't know about bottom sheet
content, so
it doesn't have the information it needs to control the shadow visibility. The
shadow
for history/bookmarks/downloads has more complex logic for when it shows (on
scroll,
on selection, and on search), so there's some plumbing and interface work
required there.

[Theresa, 2017-03-28 16:49:48]

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

[commit-bot: I haz the power, 2017-03-28 16:50:13]

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

[Theresa, 2017-03-28 16:50:27]

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

[Theresa, 2017-03-28 16:50:28]

+dgn@ - ptal at
	chrome/android/java/src/org/chromium/chrome/browser/suggestions/*

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:41:
private FadingShadowView mShadowView;
On 2017/03/28 09:22:34, dgn wrote:
> nit: why not final?

Done.

https://codereview.chromium.org/2780693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:96:
boolean shadowVisible = mRecyclerView.computeVerticalScrollOffset() != 0;
On 2017/03/28 02:07:16, mdjones wrote:
> It might be cheaper to call canScrollVertically(-1) anywhere we use
> computeVerticalScrollOffset(). It's not something that needs to change here, I
> just stumbled upon it.

Done.

[dgn, 2017-03-28 17:59:37]

lgtm

[Theresa, 2017-03-28 18:08:21]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2017-03-28 18:08:22]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-28 18:08:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2780693002/#ps20001
(title: "Changes from mjones@ review & fix tests")

[commit-bot: I haz the power, 2017-03-28 18:08:32]

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-03-28 18:16:16]

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

[commit-bot: I haz the power, 2017-03-28 18:16:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Theresa, 2017-03-28 18:32:32]

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

[Theresa, 2017-03-28 18:32:33]

+dfalcantara@ for chrome/Android OWNERS

[gone, 2017-03-28 20:21:07]

lgtm

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:93:
mRecyclerView.addOnScrollListener(new OnScrollListener() {
Might be worth investigating making this fade in, based on how far the user has
scrolled down.  The FadingEdgeScrollView used to do this by checking the fading
edge strength and keying the alpha value off of that.

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRecyclerView.java
(right):

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRecyclerView.java:77:
public SuggestionsRecyclerView(Context context, AttributeSet attrs) {
Put this under the other one, make the other one call this one with null attrs?

[Theresa, 2017-03-28 21:04:02]

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

[commit-bot: I haz the power, 2017-03-28 21:04: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-03-28 21:14:45]

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

[commit-bot: I haz the power, 2017-03-28 21:14:46]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[Theresa, 2017-03-28 21:51:14]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-28 21:51:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dgn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2780693002/#ps40001
(title: "dfalcantara@ review")

[commit-bot: I haz the power, 2017-03-28 21:51:35]

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-03-28 22:34:27]

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

[commit-bot: I haz the power, 2017-03-28 22:34:28]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Theresa, 2017-03-28 23:09:58]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-28 23:10:04]

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
(right):

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:93:
mRecyclerView.addOnScrollListener(new OnScrollListener() {
On 2017/03/28 20:21:07, dfalcantara (load balance plz) wrote:
> Might be worth investigating making this fade in, based on how far the user
has
> scrolled down.  The FadingEdgeScrollView used to do this by checking the
fading
> edge strength and keying the alpha value off of that.

Acknowledged. I added a note to the bug.

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRecyclerView.java
(right):

https://codereview.chromium.org/2780693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsRecyclerView.java:77:
public SuggestionsRecyclerView(Context context, AttributeSet attrs) {
On 2017/03/28 20:21:07, dfalcantara (load balance plz) wrote:
> Put this under the other one, make the other one call this one with null
attrs?

Done.

[commit-bot: I haz the power, 2017-03-28 23:10:24]

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-03-29 00:17:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490742598194650,
"parent_rev": "5f7733dd5fc1bcc2bc979c83cea73f351695083d", "commit_rev":
"9c82d662ed9d8c1a2a22702e43bdc6d6a3a67972"}

[commit-bot: I haz the power, 2017-03-29 00:19:17]

Description was changed from

==========
[Home] Toolbar shadow v1

Add a basic implementation for the toolbar shadow.
On the SuggestionsBottomSheetContent, a new FadingShadowView
shows up on scroll. The toolbar's shadow is currently transitioned
when the omnibox is focused/unfocused. That logic has been left in
place (for now), but the unfocused drawable is an empty shape as
there should not be a shadow when the omnibox is unfocused.

History/downloads/bookmarks all use existing logic in
SelectableListLayout that determines when their toolbar shadow
should be shown.

BUG=705728
==========

to

==========
[Home] Toolbar shadow v1

Add a basic implementation for the toolbar shadow.
On the SuggestionsBottomSheetContent, a new FadingShadowView
shows up on scroll. The toolbar's shadow is currently transitioned
when the omnibox is focused/unfocused. That logic has been left in
place (for now), but the unfocused drawable is an empty shape as
there should not be a shadow when the omnibox is unfocused.

History/downloads/bookmarks all use existing logic in
SelectableListLayout that determines when their toolbar shadow
should be shown.

BUG=705728

Review-Url: https://codereview.chromium.org/2780693002
Cr-Commit-Position: refs/heads/master@{#460246}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c82d662ed9d8c1a2a22702e43bd...
==========

[commit-bot: I haz the power, 2017-03-29 00:19:18]

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