[ash-md] Stacks child layers properly for sticky header rows

[ash-md] Stacks child layers properly for sticky header rows

The CL implements sticky section header rows in VPN page. Care is taken
to stack the rows with child controls that have layers below the sticky
section header rows.

BUG=664244
TEST=Run with --material-design-ink-drop-animation-speed=slow
     Touch a Wi-Fi connection row and while ripple is growing
     drag-scroll the list of connections up.
     Verify that the ripple is stacked below the Wi-Fi header row.

Committed: https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580
Cr-Commit-Position: refs/heads/master@{#438689}

[varkha, 2016-12-08 18:46:08]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org, tdanderson@chromium.org

[varkha, 2016-12-08 18:46:08]

This CL should fix a bug but I am thinking a better solution would be to
abstract away the order of iteration in the following methods:
 View::GetTooltipHandlerForPoint
 View::PaintChildren
 View::ReorderChildLayers
 ViewTargeterDelegate::TargetForRect
This order is now hard-coded as the order of children. Having a View subclass
being able to override and provide its own iterator would allow the child class
to control stacking for its children.
This could replace the need to override:
 PaintChildren
 ReorderChildLayers
 TargetForRect
in a class that needs this behavior and replace it with just the iterator
provider.
Still I think this change (ignore the change to shill that is just for
debugging) could be simpler to merge into M-56.

[sadrul, 2016-12-09 00:55:47]

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:114: if (child->id() ==
VIEW_ID_STICKY_HEADER && child->layer()) {
if child->id() == VIEW_ID_STICKY_HEADER, will child->layer() be ever null?

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:255: // Recursively iterates through
children to return the topmost child layer.
Just to confirm: 'topmost' == topmost in stacking order?

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:256: ui::Layer*
TopmostLayer(views::View* view, ui::Layer* parent_layer) {
DCHECK() that parent_layer is not null?

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:257: if (view->layer() &&
view->layer() != parent_layer) {
The first view->layer() check isn't necessary.

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:260: }
Shouldn't this go after iterating over the children (since the parent is below
the children)?

[varkha, 2016-12-09 02:20:38]

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:114: if (child->id() ==
VIEW_ID_STICKY_HEADER && child->layer()) {
On 2016/12/09 00:55:47, sadrul wrote:
> if child->id() == VIEW_ID_STICKY_HEADER, will child->layer() be ever null?

Done.

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:255: // Recursively iterates through
children to return the topmost child layer.
On 2016/12/09 00:55:47, sadrul wrote:
> Just to confirm: 'topmost' == topmost in stacking order?

Yes, mentioned this in a comment above TopmostLayer().

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:256: ui::Layer*
TopmostLayer(views::View* view, ui::Layer* parent_layer) {
On 2016/12/09 00:55:47, sadrul wrote:
> DCHECK() that parent_layer is not null?

Done.

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:257: if (view->layer() &&
view->layer() != parent_layer) {
On 2016/12/09 00:55:47, sadrul wrote:
> The first view->layer() check isn't necessary.

Once we move this (see the next comment) I think we actually need this check.

https://codereview.chromium.org/2557333003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:260: }
On 2016/12/09 00:55:47, sadrul wrote:
> Shouldn't this go after iterating over the children (since the parent is below
> the children)?

Done. Good catch, just not something that was exercised by the current code.
Since we are recursing first we need to check and only return layers that are
direct children of parent_layer (there may be deeper children that we might
visit first).

https://codereview.chromium.org/2557333003/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/2557333003/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:41: int s_extra_wifi_networks = 20;
Changes here to be reverted before committing.

[varkha, 2016-12-09 18:59:32]

https://codereview.chromium.org/2557333003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:81: did_draw_shadow =
PaintDelineation(header, context) || did_draw_shadow;
Self review: This is broken now that the headers have their own layers since the
separator is not painted by the headers themselves. The shadows seem fine.

[varkha, 2016-12-12 18:24:05]

PTAL - fixed separators.

https://codereview.chromium.org/2557333003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:81: did_draw_shadow =
PaintDelineation(header, context) || did_draw_shadow;
On 2016/12/09 18:59:32, varkha wrote:
> Self review: This is broken now that the headers have their own layers since
the
> separator is not painted by the headers themselves. The shadows seem fine.

Should be fixed now that a separator is painted by the row view's border as
other separators rather than by the parent view in this file.

[varkha, 2016-12-12 18:24:15]

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

[commit-bot: I haz the power, 2016-12-12 18:24:36]

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, 2016-12-12 23:24:19]

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

[commit-bot: I haz the power, 2016-12-12 23:24:19]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-14 00:48:29]

lgtm

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (left):

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:374: // If there is a
connected or connecting network, show that network first.
Is this taken care of elsewhere? Or do we not want this UX anymore?

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:120: }
This is really subtle. Can you look at having a test for this?

[varkha, 2016-12-14 17:46:42]

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (left):

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:374: // If there is a
connected or connecting network, show that network first.
On 2016/12/14 00:48:28, sadrul wrote:
> Is this taken care of elsewhere? Or do we not want this UX anymore?

No, we don't want this anymore. The connected VPN will be the top item in the
top section because of the logic around line 485 in this file. So bubbling it
out of sticky sections was superficial.

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2557333003/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:120: }
On 2016/12/14 00:48:29, sadrul wrote:
> This is really subtle. Can you look at having a test for this?

I will.

[varkha, 2016-12-14 20:51:17]

Patchset #4 (id:60001) has been deleted

[varkha, 2016-12-14 20:54:06]

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

[commit-bot: I haz the power, 2016-12-14 20:55:06]

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, 2016-12-14 21:29:22]

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

[commit-bot: I haz the power, 2016-12-14 21:29:23]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-12-14 23:58:45]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-12-14 23:58:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2557333003/#ps100001
(title: "[ash-md] Stacks child layers properly for sticky header rows (no shill
changes)")

[commit-bot: I haz the power, 2016-12-14 23:59:17]

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-12-15 01:01:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481759925191810,
"parent_rev": "71d5d934c63b0d522a24dcb244e8d801fed60ea1", "commit_rev":
"0dee5c134e8c2b2c4dee435dfc47ccc0a3f558a5"}

[commit-bot: I haz the power, 2016-12-15 01:01:34]

Description was changed from

==========
[ash-md] Stacks child layers properly for sticky header rows

The CL implements sticky section header rows in VPN page. Care is taken
to stack the rows with child controls that have layers below the sticky
section header rows.

BUG=664244
TEST=Run with --material-design-ink-drop-animation-speed=slow
     Touch a Wi-Fi connection row and while ripple is growing
     drag-scroll the list of connections up.
     Verify that the ripple is stacked below the Wi-Fi header row.
==========

to

==========
[ash-md] Stacks child layers properly for sticky header rows

The CL implements sticky section header rows in VPN page. Care is taken
to stack the rows with child controls that have layers below the sticky
section header rows.

BUG=664244
TEST=Run with --material-design-ink-drop-animation-speed=slow
     Touch a Wi-Fi connection row and while ripple is growing
     drag-scroll the list of connections up.
     Verify that the ripple is stacked below the Wi-Fi header row.

Review-Url: https://codereview.chromium.org/2557333003
==========

[commit-bot: I haz the power, 2016-12-15 01:01:34]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-15 01:03:32]

Description was changed from

==========
[ash-md] Stacks child layers properly for sticky header rows

The CL implements sticky section header rows in VPN page. Care is taken
to stack the rows with child controls that have layers below the sticky
section header rows.

BUG=664244
TEST=Run with --material-design-ink-drop-animation-speed=slow
     Touch a Wi-Fi connection row and while ripple is growing
     drag-scroll the list of connections up.
     Verify that the ripple is stacked below the Wi-Fi header row.

Review-Url: https://codereview.chromium.org/2557333003
==========

to

==========
[ash-md] Stacks child layers properly for sticky header rows

The CL implements sticky section header rows in VPN page. Care is taken
to stack the rows with child controls that have layers below the sticky
section header rows.

BUG=664244
TEST=Run with --material-design-ink-drop-animation-speed=slow
     Touch a Wi-Fi connection row and while ripple is growing
     drag-scroll the list of connections up.
     Verify that the ripple is stacked below the Wi-Fi header row.

Committed: https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580
Cr-Commit-Position: refs/heads/master@{#438689}
==========

[commit-bot: I haz the power, 2016-12-15 01:03:33]

Patchset 5 (id:??) landed as
https://crrev.com/cd9913053dc56f12271b0c73558635ffbc26c580
Cr-Commit-Position: refs/heads/master@{#438689}