[Chrome OS MD] Update Cast and VPN to follow MD layout

Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507
Committed: https://crrev.com/a58f98c5be6b86fba82983a7a4f30a318d4524ec
Cr-Commit-Position: refs/heads/master@{#434419}

[yiyix, 2016-11-22 21:43:36]

Patchset #1 (id:1) has been deleted

[yiyix, 2016-11-22 21:43:44]

Patchset #1 (id:20001) has been deleted

[yiyix, 2016-11-22 21:43:49]

Patchset #1 (id:40001) has been deleted

[yiyix, 2016-11-22 21:43:55]

Patchset #1 (id:60001) has been deleted

[yiyix, 2016-11-22 21:56:27]

Patchset #1 (id:80001) has been deleted

[yiyix, 2016-11-22 21:56:32]

Patchset #1 (id:100001) has been deleted

[yiyix, 2016-11-22 22:02:06]

Description was changed from

==========
cast


cast


fix separator

BUG=
==========

to

==========
Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507
==========

[yiyix, 2016-11-22 22:02:07]

yiyix@chromium.org changed reviewers:
+ bruthig@chromium.org, tdanderson@chromium.org

[yiyix, 2016-11-22 22:02:29]

@bruthig, Could you please take a look at this change? Thank you.

[tdanderson, 2016-11-23 00:34:19]

On 2016/11/22 22:02:29, yiyix wrote:
> @bruthig, Could you please take a look at this change? Thank you.

I will wait until Ben has given this an initial review. Please
re-ping when Ben is happy and this is ready for owners review.

[bruthig, 2016-11-23 20:15:47]

Left some comments inline, it might make sense to digest them and follow-up
offline.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/network/vpn_list_view.cc:287:
views::CreateEmptyBorder(0, UseMd() ? 0 : kTrayPopupPaddingHorizontal,
Do the horizontal insets need to be updated for rows which are not
connected/connecting? i.e. on line 290?

I find it very suspicious that we are setting a border on |this| directly as
well as using a TriView for layout. Ideally it would be one or the other.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:49:
kSystemTrayScreenShareIcon, default_view_style.GetIconColor()));
FYI The ScreenTrayView is the one shown beside the clock in the shelf.  Correct
me if I'm wrong but I don't think it should have the system menu style applied
to it.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:69:
SetBorder(views::CreateEmptyBorder(0, 0, 0, kTrayPopupButtonEndMargin));
I think it would be better if we didn't set custom borders on TriView's returned
from CreateDefaultRowView() because it will make it more difficult to apply a
border within CreateDefaultRowView() if we ever need to do it for all clients. 
Would it work to use TriView::SetContainerBorder() for the END container
instead?

Also, this seems to be a recurring pattern. i.e. All buttons created with
TPIU::CreateTrayPopupButton() will need this layout.  Is that true?  If so it
might make sense to create a new function like
TPIU::CreateTrayPopupButtonBorder(). We can discuss this offline.

[yiyix, 2016-11-24 21:38:16]

@bruthig, Could you please take another look? Thanks.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/network/vpn_list_view.cc:287:
views::CreateEmptyBorder(0, UseMd() ? 0 : kTrayPopupPaddingHorizontal,
On 2016/11/23 20:15:47, bruthig wrote:
> Do the horizontal insets need to be updated for rows which are not
> connected/connecting? i.e. on line 290?
> 
> I find it very suspicious that we are setting a border on |this| directly as
> well as using a TriView for layout. Ideally it would be one or the other.

I updated the code path such that all border changes are specific to non-md path
only. I also investigated/tried to make non-md path to use tri_view as well,
this change results removing hundred of lines. I think it would make it harder
to merge to the branch and user probably doesn't see this visual change as MD is
turned on.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:49:
kSystemTrayScreenShareIcon, default_view_style.GetIconColor()));
On 2016/11/23 20:15:47, bruthig wrote:
> FYI The ScreenTrayView is the one shown beside the clock in the shelf. 
Correct
> me if I'm wrong but I don't think it should have the system menu style applied
> to it.

Sorry. you are you right.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:69:
SetBorder(views::CreateEmptyBorder(0, 0, 0, kTrayPopupButtonEndMargin));
On 2016/11/23 20:15:47, bruthig wrote:
> I think it would be better if we didn't set custom borders on TriView's
returned
> from CreateDefaultRowView() because it will make it more difficult to apply a
> border within CreateDefaultRowView() if we ever need to do it for all clients.

> Would it work to use TriView::SetContainerBorder() for the END container
> instead?
> 
> Also, this seems to be a recurring pattern. i.e. All buttons created with
> TPIU::CreateTrayPopupButton() will need this layout.  Is that true?  If so it
> might make sense to create a new function like
> TPIU::CreateTrayPopupButtonBorder(). We can discuss this offline.

As we have discussed offline, for this code review, i will set up borders on end
container instead.

[bruthig, 2016-11-24 22:36:29]

lgtm with nit.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/network/vpn_list_view.cc:287:
views::CreateEmptyBorder(0, UseMd() ? 0 : kTrayPopupPaddingHorizontal,
On 2016/11/24 21:38:16, yiyix wrote:
> On 2016/11/23 20:15:47, bruthig wrote:
> > Do the horizontal insets need to be updated for rows which are not
> > connected/connecting? i.e. on line 290?
> > 
> > I find it very suspicious that we are setting a border on |this| directly as
> > well as using a TriView for layout. Ideally it would be one or the other.
> 
> I updated the code path such that all border changes are specific to non-md
path
> only. I also investigated/tried to make non-md path to use tri_view as well,
> this change results removing hundred of lines. I think it would make it harder
> to merge to the branch and user probably doesn't see this visual change as MD
is
> turned on. 

Acknowledged.

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://codereview.chromium.org/2512883002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:69:
SetBorder(views::CreateEmptyBorder(0, 0, 0, kTrayPopupButtonEndMargin));
On 2016/11/24 21:38:16, yiyix wrote:
> On 2016/11/23 20:15:47, bruthig wrote:
> > I think it would be better if we didn't set custom borders on TriView's
> returned
> > from CreateDefaultRowView() because it will make it more difficult to apply
a
> > border within CreateDefaultRowView() if we ever need to do it for all
clients.
> 
> > Would it work to use TriView::SetContainerBorder() for the END container
> > instead?
> > 
> > Also, this seems to be a recurring pattern. i.e. All buttons created with
> > TPIU::CreateTrayPopupButton() will need this layout.  Is that true?  If so
it
> > might make sense to create a new function like
> > TPIU::CreateTrayPopupButtonBorder(). We can discuss this offline.
> 
> As we have discussed offline, for this code review, i will set up borders on
end
> container instead. 

Awesome, thx :)

https://codereview.chromium.org/2512883002/diff/140001/ash/common/system/chro...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2512883002/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/network/vpn_list_view.cc:289: DCHECK(tri_view());
nit: I believe the style guide says not to include DCHECKs that will fail due to
a segfault anyway in the following few lines.

[tdanderson, 2016-11-25 00:45:41]

https://codereview.chromium.org/2512883002/diff/140001/ash/common/system/chro...
File ash/common/system/chromeos/screen_security/screen_tray_item.cc (right):

https://codereview.chromium.org/2512883002/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:62:
tri_view->AddView(TriView::Container::END, stop_button_);
Do you also need to call tri_view->SetContainerVisible(TriView::Container::END,
true) ?

https://codereview.chromium.org/2512883002/diff/140001/ash/common/system/chro...
ash/common/system/chromeos/screen_security/screen_tray_item.cc:111:
icon_->SetImage(gfx::CreateVectorIcon(kSystemMenuScreenShareIcon,
As discussed remove this line, and after line 81 in CreateItems() you should
have:

icon_->SetImage(gfx::CreateVectorIcon(kSystemMenuScreenShareIcon,
TrayPopupItemStyle::GetIconColor(TrayPopupItemStyle::ColorStyle::ACTIVE)));

[tdanderson, 2016-11-25 00:58:39]

LGTM

[yiyix, 2016-11-25 00:59:28]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-11-25 00:59:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2512883002/#ps160001
(title: "address comments, thanks @tdanderson")

[commit-bot: I haz the power, 2016-11-25 00:59: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-25 01:28:08]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480035568007620,
"parent_rev": "b28014e31032144ba939da76551ee1d483e683b5", "commit_rev":
"5445d3b0df09391be62ebb6aeb936594aaa1e5af"}

[commit-bot: I haz the power, 2016-11-25 01:28:50]

Description was changed from

==========
Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507
==========

to

==========
Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507
==========

[commit-bot: I haz the power, 2016-11-25 01:28:51]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-11-25 01:30:21]

Description was changed from

==========
Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507
==========

to

==========
Update Cast and VPN to follow MD layout

Implemented the correct layout for both Cast and VPN default rows
by following MD specs of system menu, which is done by making use
of TriView class.

TEST=MANUAL

BUG=666522, 666507

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

[commit-bot: I haz the power, 2016-11-25 01:30:22]

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