Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to
right-aligned shelf, the spacing between the edge of the screen to tray
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
Committed: https://crrev.com/eeee71c610e8e6548b94d3abb180fd53d16a487d
Cr-Commit-Position: refs/heads/master@{#403215}

[yiyix, 2016-06-29 04:45:16]

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

[commit-bot: I haz the power, 2016-06-29 04:45:31]

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

[yiyix, 2016-06-29 04:45:37]

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

[yiyix, 2016-06-29 04:46:24]

On 2016/06/29 04:45:38, yiyix wrote:

@tdanderson, Could you take a look at this code change?

[yiyix, 2016-06-29 04:56:18]

Description was changed from

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

BUG=623438
==========

to

==========
Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

[commit-bot: I haz the power, 2016-06-29 05:51:03]

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

[commit-bot: I haz the power, 2016-06-29 05:51:04]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-06-29 16:24:50]

yiyix@chromium.org changed reviewers:
+ varkha@chromium.org

[tdanderson, 2016-06-29 17:46:19]

Looks like a reasonable approach. I've left some comments inline but feel free
to send to James as-is to get his thoughts before making any other changes.

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/session/logout_button_tray.h (right):

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/session/logout_button_tray.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: please format the CL description as:

<title>
<blank line>
<brief description of CL>

Please also explicitly call out that this is intended to fix a non-MD regression
and be a noop for MD.

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:72: // Called after the tray has
been added to the widget containing it.
Please add to this comment to explain what |insets| is.

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:93: // Called whenever the shelf
alignment changes.
Please add to this comment to explain |insets|

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:74: CalculateBorderMD(&no_edge_border,
system_tray_->shelf_alignment(), false);
The generally preferred pattern in a case like this is to have
CalculateBorderMD() return a gfx::Insets rather than mutate a gfx::Insets*
argument. Ditto for 86-89.

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:204: // remove all code related to Non Md
Border setup code. See crbug.com/614453.
Thanks for adding this. Though in this specific case I don't think it's strictly
necessary since the dead code will be readily apparent when we do a code search
for uses of MDC::IsShelfMaterial().

nit: "Border setup." instead of "Border setup code."

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:231: CalculateBorderNonMD(&insets_overview,
alignment, true);
Consider only performing this calculation inside the if (overview_button_tray_)

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:312: insets->Set(top_edge, left_edge,
bottom_edge, right_edge);
Some refactoring may be advisable here, but I will defer to James' opinion to
avoid churn (i.e., having you do it the way I suggest only to have James suggest
something different).

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.h:85: // Calculate the border insets for trays
(ex: notification, overview) for both
nit: "non-MD"

nit: "for tray items" instead of "for trays"

[yiyix, 2016-06-29 18:56:39]

yiyix@chromium.org changed reviewers:
+ jamescook@chromium.org

[yiyix, 2016-06-29 18:56:40]

@jamescook, could you please take a look? Thanks

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:72: // Called after the tray has
been added to the widget containing it.
On 2016/06/29 17:46:19, tdanderson wrote:
> Please add to this comment to explain what |insets| is.

Done.

https://codereview.chromium.org/2103113003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:93: // Called whenever the shelf
alignment changes.
On 2016/06/29 17:46:19, tdanderson wrote:
> Please add to this comment to explain |insets|

Done.

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:74: CalculateBorderMD(&no_edge_border,
system_tray_->shelf_alignment(), false);
On 2016/06/29 17:46:19, tdanderson wrote:
> The generally preferred pattern in a case like this is to have
> CalculateBorderMD() return a gfx::Insets rather than mutate a gfx::Insets*
> argument. Ditto for 86-89.

Done.

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:204: // remove all code related to Non Md
Border setup code. See crbug.com/614453.
On 2016/06/29 17:46:19, tdanderson wrote:
> Thanks for adding this. Though in this specific case I don't think it's
strictly
> necessary since the dead code will be readily apparent when we do a code
search
> for uses of MDC::IsShelfMaterial().
> 
> nit: "Border setup." instead of "Border setup code."

I will remove this comment

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:231: CalculateBorderNonMD(&insets_overview,
alignment, true);
On 2016/06/29 17:46:19, tdanderson wrote:
> Consider only performing this calculation inside the if
(overview_button_tray_)

Done.

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.cc:312: insets->Set(top_edge, left_edge,
bottom_edge, right_edge);
On 2016/06/29 17:46:19, tdanderson wrote:
> Some refactoring may be advisable here, but I will defer to James' opinion to
> avoid churn (i.e., having you do it the way I suggest only to have James
suggest
> something different).

Ok, I agree that some trick we used in space calculation is MD can be used here.
I can wait until James's review

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
File ash/system/status_area_widget.h (right):

https://codereview.chromium.org/2103113003/diff/1/ash/system/status_area_widg...
ash/system/status_area_widget.h:85: // Calculate the border insets for trays
(ex: notification, overview) for both
On 2016/06/29 17:46:19, tdanderson wrote:
> nit: "non-MD"
> 
> nit: "for tray items" instead of "for trays"

Done.

[yiyix, 2016-06-29 19:06:11]

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

[commit-bot: I haz the power, 2016-06-29 19:07:03]

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-06-29 19:11:47]

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

[commit-bot: I haz the power, 2016-06-29 19:11:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2016-06-29 19:34:15]

I'm sorry, but I would prefer you find a different way to fix this. In
particular, I would prefer that TrayBackgroundView "pull" the desired insets out
of some shared piece of code whenever the alignment changes, rather than
"pushing" it in via all the various subclasses.

The reasoning for this is that TrayBackgroundView should not own a copy of the
shelf alignment. That information should come out of WmShelf. In the future
TrayBackgroundView should just observe for alignment changes and do whatever
updates it needs to do based on the new alignment found via WmShelf. I'm hoping
to eliminate the TrayBackgroundView::SetShelfAlignment() method.

Hopefully this won't be too bad, since it looks like the methods to compute the
insets can move elsewhere.

Also, given that this is a regression, is it possible to write an automated test
for it?

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

https://codereview.chromium.org/2103113003/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:382: SetTrayBorder(insets);
This should pull the insets from somewhere, rather than having them pushed in.

https://codereview.chromium.org/2103113003/diff/40001/ash/system/status_area_...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/2103113003/diff/40001/ash/system/status_area_...
ash/system/status_area_widget.cc:258: gfx::Insets
StatusAreaWidget::CalculateBorderMD(ShelfAlignment alignment,
Can these methods be static? Then they could move elsewhere.

[yiyix, 2016-06-29 20:59:40]

@jamescook, Could you please review this change? Thanks

[James Cook, 2016-06-29 21:23:37]

https://codereview.chromium.org/2103113003/diff/60001/ash/system/status_area_...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/2103113003/diff/60001/ash/system/status_area_...
ash/system/status_area_widget_delegate.cc:166: View::InvalidateLayout();
It seems a little weird to call View::Layout() then immediately call
View::InvalidateLayout().

If you need to invalidate the layout of all the parent views, it might work to
call it at the start of UpdateLayout(). If that works, it is what I would
prefer.

If you really have to do it after Layout() there is probably only one specific
case where you need it (like after the call to UpdateLayout in
StatusAreaWidget::SetShelfAlignment()).

Either way this needs a comment explaining why you have to do it.

Also, you don't need View::.

[yiyix, 2016-06-29 22:03:39]

Description was changed from

==========
Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

to

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

[yiyix, 2016-06-29 22:08:33]

On 2016/06/29 21:23:37, James Cook wrote:
>
https://codereview.chromium.org/2103113003/diff/60001/ash/system/status_area_...
> File ash/system/status_area_widget_delegate.cc (right):
> 
>
https://codereview.chromium.org/2103113003/diff/60001/ash/system/status_area_...
> ash/system/status_area_widget_delegate.cc:166: View::InvalidateLayout();
> It seems a little weird to call View::Layout() then immediately call
> View::InvalidateLayout().
> 
> If you need to invalidate the layout of all the parent views, it might work to
> call it at the start of UpdateLayout(). If that works, it is what I would
> prefer.
> 
> If you really have to do it after Layout() there is probably only one specific
> case where you need it (like after the call to UpdateLayout in
> StatusAreaWidget::SetShelfAlignment()).
> 
> Either way this needs a comment explaining why you have to do it.
> 
> Also, you don't need View::.

Sorry, I was too excited to see it worked. I was not paying any attention to
coding style. 

It turns out that I had some previous code when I try this patch and this
solution does not work. T_T

All child views are the ones got border changed, so they need a layout update.
This patch works.

[yiyix, 2016-06-29 22:10:50]

Patchset #5 (id:80001) has been deleted

[James Cook, 2016-06-29 22:20:35]

LGTM with nit

https://codereview.chromium.org/2103113003/diff/100001/ash/system/status_area...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/2103113003/diff/100001/ash/system/status_area...
ash/system/status_area_widget_delegate.cc:229: // Force layout to update after
change border around |child|.
nit: I would explain in a little more detail *why* you have to do this. I would
also cite the bug number in the comment.

[yiyix, 2016-06-29 23:46:51]

On 2016/06/29 22:20:35, James Cook wrote:
> LGTM with nit
> 
>
https://codereview.chromium.org/2103113003/diff/100001/ash/system/status_area...
> File ash/system/status_area_widget_delegate.cc (right):
> 
>
https://codereview.chromium.org/2103113003/diff/100001/ash/system/status_area...
> ash/system/status_area_widget_delegate.cc:229: // Force layout to update after
> change border around |child|.
> nit: I would explain in a little more detail *why* you have to do this. I
would
> also cite the bug number in the comment.

After having a better understanding of how layout works in view, I learned that
we also use layout() to update the border. I think using layout() is easier for
reader to follow and understand the logic than using InvalidateLayout().

[yiyix, 2016-06-30 00:05:10]

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

[commit-bot: I haz the power, 2016-06-30 00:06:27]

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-06-30 00:10:16]

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

[commit-bot: I haz the power, 2016-06-30 00:10:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[yiyix, 2016-06-30 15:56:51]

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

[commit-bot: I haz the power, 2016-06-30 15:57:07]

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

[James Cook, 2016-06-30 16:14:35]

LGTM. Nice fix!

[commit-bot: I haz the power, 2016-06-30 16:56:00]

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

[commit-bot: I haz the power, 2016-06-30 16:56:02]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-06-30 17:40:09]

The CQ bit was checked by yiyix@chromium.org

[commit-bot: I haz the power, 2016-06-30 17:40:51]

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-06-30 17:46:58]

Description was changed from

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

to

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

[commit-bot: I haz the power, 2016-06-30 17:46:59]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-30 17:47:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 17:49:48]

Description was changed from

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to 
right-aligned shelf, the spacing between the edge of the screen to tray 
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438
==========

to

==========
Fix shelf layout when switching from left-aligned shelf to right-aligned shelf

Fix the issue that when shelf is switched from left-aligned shelf to
right-aligned shelf, the spacing between the edge of the screen to tray
items are not changed.

TEST=Manuel
- change shelf location left to right or right to left.
- change shelf location bottom to right or bottom to left.
- initialize shelf locating on left or right.

BUG=623438

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

[commit-bot: I haz the power, 2016-06-30 17:49:49]

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