Fix a couple issues with sticky headers

Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
Committed: https://crrev.com/01c344c1cb0a6083024642ac9cfbd20112553707
Cr-Commit-Position: refs/heads/master@{#431887}

[Evan Stade, 2016-11-11 06:53:24]

estade@chromium.org changed reviewers:
+ fukino@chromium.org, tdanderson@chromium.org

[Evan Stade, 2016-11-11 06:53:25]

this is sorta a WIP because I've found it triggers a crash (which I believe is
unrelated to this change, but is somehow triggered by it --- haven't fully
sorted it out yet). Still, sending it out as an FYI. Unfortunately, nothing's
ever simple.

[Evan Stade, 2016-11-11 06:53:36]

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

[commit-bot: I haz the power, 2016-11-11 06:53:55]

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

[Evan Stade, 2016-11-11 06:54:13]

Description was changed from

==========
Fix a couple issues with sticky headers

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
==========

to

==========
Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
==========

[fukino, 2016-11-11 06:59:58]

Thank you for correcting the top padding of scroll contents!

[Evan Stade, 2016-11-11 07:07:37]

also, just so we are on the same page about the bug described in (2), here's a
screenshot: https://screenshot.googleplex.com/oORG0jzbV19.png

[tdanderson, 2016-11-11 19:49:51]

Evan, please re-ping when this is ready for review.

[Evan Stade, 2016-11-11 23:42:45]

ptal

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

https://codereview.chromium.org/2489883005/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:300:
container()->SetLayoutManager(
this was the source of the crash --- container() is the scroll contents. I
haven't the slightest clue why the normal layout manager is being overridden
here.

[tdanderson, 2016-11-12 01:45:21]

LGTM

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

https://codereview.chromium.org/2489883005/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:300:
container()->SetLayoutManager(
On 2016/11/11 23:42:45, Evan Stade wrote:
> this was the source of the crash --- container() is the scroll contents. I
> haven't the slightest clue why the normal layout manager is being overridden
> here.

Acknowledged.

[Evan Stade, 2016-11-12 02:53:41]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-12 02:53:52]

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-11-12 03:06:09]

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

[commit-bot: I haz the power, 2016-11-12 03:06:10]

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

[Evan Stade, 2016-11-13 03:35:57]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-13 03:36:04]

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-11-13 04:09:08]

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

[commit-bot: I haz the power, 2016-11-13 04:09:09]

Failed to apply patch for ash/common/system/tray/tray_popup_utils.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/system/tray/tray_popup_utils.cc:190
  error: ash/common/system/tray/tray_popup_utils.cc: patch does not apply

Patch:       ash/common/system/tray/tray_popup_utils.cc
Index: ash/common/system/tray/tray_popup_utils.cc
diff --git a/ash/common/system/tray/tray_popup_utils.cc
b/ash/common/system/tray/tray_popup_utils.cc
index
3864fa8dd9891be18cc259886a6ad09ca8ffa947..bb201d1c85b7effe5ecc0238e265bb8748b646e2
100644
--- a/ash/common/system/tray/tray_popup_utils.cc
+++ b/ash/common/system/tray/tray_popup_utils.cc
@@ -5,6 +5,7 @@
 #include "ash/common/system/tray/tray_popup_utils.h"
 
 #include "ash/common/ash_constants.h"
+#include "ash/common/ash_view_ids.h"
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/session/session_state_delegate.h"
 #include "ash/common/system/tray/fixed_sized_image_view.h"
@@ -13,6 +14,7 @@
 #include "ash/common/system/tray/tray_popup_label_button_border.h"
 #include "ash/common/wm_shell.h"
 #include "ui/views/animation/ink_drop_impl.h"
+#include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/label_button.h"
@@ -190,6 +192,14 @@ views::Slider*
TrayPopupUtils::CreateSlider(views::SliderListener* listener) {
   return slider;
 }
 
+void TrayPopupUtils::ConfigureAsStickyHeader(views::View* view) {
+  view->set_id(VIEW_ID_STICKY_HEADER);
+  view->set_background(
+      views::Background::CreateSolidBackground(kBackgroundColor));
+  view->SetBorder(
+      views::CreateEmptyBorder(gfx::Insets(kMenuSeparatorVerticalPadding, 0)));
+}
+
 void TrayPopupUtils::ConfigureContainer(TriView::Container container,
                                         views::View* container_view) {
   container_view->SetBorder(CreateDefaultBorder(container));

[Evan Stade, 2016-11-14 15:34:20]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-14 15:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2489883005/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-14 15:34: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, 2016-11-14 19:11:21]

Description was changed from

==========
Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
==========

to

==========
Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
==========

[commit-bot: I haz the power, 2016-11-14 19:11:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-14 19:34:57]

Description was changed from

==========
Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154
==========

to

==========
Fix a couple issues with sticky headers in CrOS system menu

1. header rows are actually supposed to be 56dp tall (48 + 4 above and below)
2. there should be padding at the top of the scroll contents (as recently
   added) but not when the first row is a header. You can see how bad this
   currently looks by forcing the IME's keyboard toggle row to be visible.

BUG=663154

Committed: https://crrev.com/01c344c1cb0a6083024642ac9cfbd20112553707
Cr-Commit-Position: refs/heads/master@{#431887}
==========

[commit-bot: I haz the power, 2016-11-14 19:34:58]

Patchset 4 (id:??) landed as
https://crrev.com/01c344c1cb0a6083024642ac9cfbd20112553707
Cr-Commit-Position: refs/heads/master@{#431887}