Fix SysTray focus rect position

Fix SysTray focus rect position

BUG=672723
TEST=On login, lock, and desktop screens, with and without
--ash-md=disabled flag, move focus to SysTray, and verify that focus
rect is in correct position.

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

[Greg Levin, 2016-12-12 18:37:30]

glevin@chromium.org changed reviewers:
+ estade@chromium.org, tdanderson@chromium.org, xiaoyinh@chromium.org

[Greg Levin, 2016-12-12 18:37:31]

Please have a look!
(And see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=672723#c4)
xiaoyinh@ and estade@, as recent changers of this same code, could you have a
quick look and see if you spot any problems, or let me know if I'm missing
something?

tdanderson@chromium.org - Owner
xiaoyinh@chromium.org - https://codereview.chromium.org/2513823002
estade@chromium.org - https://codereview.chromium.org/2527513002

[xiaoyinh(OOO Sep 11-29), 2016-12-12 23:05:59]

Thanks for the fix, the change looks good to me. Please double check with
tdanderson@ though, because I'm not sure if there will be more changes in the
shelf that will affect the bound/position of the TrayBackgroundView objects. If
so, these inset values might need to be tweaked again.
Thanks!

[Evan Stade, 2016-12-12 23:47:50]

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

https://codereview.chromium.org/2569773002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:588:
paint_bounds.Inset(gfx::Insets(-2, 2, -2, 3));
ick at these magic numbers. Where do they come from? Why is GetFocusBounds not
apparently returning the right thing?

I don't think it's necessary to continue adding code to support the non-material
version (existing code for pre-md will be ripped out soon(tm)).

[Greg Levin, 2016-12-13 17:08:07]

xiaoyinh@ - Thanks!  Will definitely wait for tdanderson@, as he's the owner :-)

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

https://codereview.chromium.org/2569773002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:588:
paint_bounds.Inset(gfx::Insets(-2, 2, -2, 3));
On 2016/12/12 23:47:50, Evan Stade wrote:
> ick at these magic numbers. Where do they come from? Why is
> GetFocusBounds not apparently returning the right thing?

Agree with ick.  That said, I'm not sure about the current merits of a deeper
dive vs my bandaid, given that this code still seems to be in flux (see
xiaoyinh@'s comment, and the fact that this inset has been fixed 3 times in the
last month).  Maybe a TODO for when MD changes are finalized?
 
> I don't think it's necessary to continue adding code to support the
> non-material version (existing code for pre-md will be ripped out
> soon(tm)).

I wasn't sure why we keep coming up with different insets.  I added the non-MD
code on the chance that someone was looking that version, and wanted to avoid
any future change based on the non-MD layout.  That said, I'm not really tuned
in to the MD shelf progress, so will definitely remove it if that makes more
sense.

[tdanderson, 2016-12-13 23:17:10]

LGTM

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

https://codereview.chromium.org/2569773002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:588:
paint_bounds.Inset(gfx::Insets(-2, 2, -2, 3));
On 2016/12/13 17:08:07, Greg Levin wrote:
> On 2016/12/12 23:47:50, Evan Stade wrote:
> > ick at these magic numbers. Where do they come from? Why is
> > GetFocusBounds not apparently returning the right thing?
> 
> Agree with ick.  That said, I'm not sure about the current merits of a deeper
> dive vs my bandaid, given that this code still seems to be in flux (see
> xiaoyinh@'s comment, and the fact that this inset has been fixed 3 times in
the
> last month).  Maybe a TODO for when MD changes are finalized?

Leaving the non-md change in here is fine by me. No need to add a TODO since
this will automatically be addressed when we rip out non-md code.

>  
> > I don't think it's necessary to continue adding code to support the
> > non-material version (existing code for pre-md will be ripped out
> > soon(tm)).
> 
> I wasn't sure why we keep coming up with different insets.  I added the non-MD
> code on the chance that someone was looking that version, and wanted to avoid
> any future change based on the non-MD layout.  That said, I'm not really tuned
> in to the MD shelf progress, so will definitely remove it if that makes more
> sense.

[Evan Stade, 2016-12-14 00:48:16]

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

https://codereview.chromium.org/2569773002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:588:
paint_bounds.Inset(gfx::Insets(-2, 2, -2, 3));
On 2016/12/13 23:17:09, tdanderson wrote:
> On 2016/12/13 17:08:07, Greg Levin wrote:
> > On 2016/12/12 23:47:50, Evan Stade wrote:
> > > ick at these magic numbers. Where do they come from? Why is
> > > GetFocusBounds not apparently returning the right thing?
> > 
> > Agree with ick.  That said, I'm not sure about the current merits of a
deeper
> > dive vs my bandaid, given that this code still seems to be in flux (see
> > xiaoyinh@'s comment, and the fact that this inset has been fixed 3 times in
> the
> > last month).  Maybe a TODO for when MD changes are finalized?
> 
> Leaving the non-md change in here is fine by me. No need to add a TODO since
> this will automatically be addressed when we rip out non-md code.
> 
> >  
> > > I don't think it's necessary to continue adding code to support the
> > > non-material version (existing code for pre-md will be ripped out
> > > soon(tm)).
> > 
> > I wasn't sure why we keep coming up with different insets.  I added the
non-MD
> > code on the chance that someone was looking that version, and wanted to
avoid
> > any future change based on the non-MD layout.  That said, I'm not really
tuned
> > in to the MD shelf progress, so will definitely remove it if that makes more
> > sense.
> 

I'm not really ok with these magic numbers though, and I'm still confused why
GetFocusBounds isn't getting the focus bounds.

[Greg Levin, 2016-12-15 17:30:56]

estade@- As per https://bugs.chromium.org/p/chromium/issues/detail?id=672723#c7,
should I leave the focus rect this color and thickness?  Will that be consistent
with other M-57 shelf focus rects?

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

https://codereview.chromium.org/2569773002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:588:
paint_bounds.Inset(gfx::Insets(-2, 2, -2, 3));
On 2016/12/14 00:48:16, Evan Stade wrote:
> On 2016/12/13 23:17:09, tdanderson wrote:
> > On 2016/12/13 17:08:07, Greg Levin wrote:
> > > On 2016/12/12 23:47:50, Evan Stade wrote:
> > > > ick at these magic numbers. Where do they come from? Why is
> > > > GetFocusBounds not apparently returning the right thing?
> > > 
> > > Agree with ick.  That said, I'm not sure about the current merits of a
> deeper
> > > dive vs my bandaid, given that this code still seems to be in flux (see
> > > xiaoyinh@'s comment, and the fact that this inset has been fixed 3 times
in
> > the
> > > last month).  Maybe a TODO for when MD changes are finalized?
> > 
> > Leaving the non-md change in here is fine by me. No need to add a TODO since
> > this will automatically be addressed when we rip out non-md code.
> > 
> > >  
> > > > I don't think it's necessary to continue adding code to support the
> > > > non-material version (existing code for pre-md will be ripped out
> > > > soon(tm)).
> > > 
> > > I wasn't sure why we keep coming up with different insets.  I added the
> non-MD
> > > code on the chance that someone was looking that version, and wanted to
> avoid
> > > any future change based on the non-MD layout.  That said, I'm not really
> tuned
> > > in to the MD shelf progress, so will definitely remove it if that makes
more
> > > sense.
> > 
> 
> I'm not really ok with these magic numbers though, and I'm still > confused
why GetFocusBounds isn't getting the focus bounds.

I don't know the details of the code, but the way GetFocusBounds() was
overridden seems wrong for MD shelf (but okay for non-MD). 
CreateInkDropHighlight() was using GetBackgroundBounds() to place the ink drop,
and that seems to work better here.
We still have the magic "1" for non-MD, but that'll be going away soon. 
Otherwise, how does this look?

[Evan Stade, 2016-12-15 17:38:01]

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

https://codereview.chromium.org/2569773002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:582: ? -kFocusBorderThickness
this looks much better, but GetFocusBounds is still confusing. It looks like
it's a virtual function on ActionableView but the override in TrayBackgroundView
is never actually used (in the sense that ActionableView::OnPaintFocus, which
would use the specialization, is also overridden). I think you should be able to
just get rid of GetFocusBounds and calculate the proper bounds in this function
(i.e. GetBackgroundsBounds() and the appropriate inset)

[Greg Levin, 2016-12-15 21:06:51]

On 2016/12/15 17:38:01, Evan Stade wrote:
>
https://codereview.chromium.org/2569773002/diff/20001/ash/common/system/tray/...
> File ash/common/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/2569773002/diff/20001/ash/common/system/tray/...
> ash/common/system/tray/tray_background_view.cc:582: ? -kFocusBorderThickness
> this looks much better, but GetFocusBounds is still confusing. It looks like
> it's a virtual function on ActionableView but the override in
TrayBackgroundView
> is never actually used (in the sense that ActionableView::OnPaintFocus, which
> would use the specialization, is also overridden). I think you should be able
to
> just get rid of GetFocusBounds and calculate the proper bounds in this
function
> (i.e. GetBackgroundsBounds() and the appropriate inset)

Could you also let me know if you want border color and width left as they are?

[Greg Levin, 2016-12-15 21:08:58]

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

https://codereview.chromium.org/2569773002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:582: ? -kFocusBorderThickness
On 2016/12/15 17:38:00, Evan Stade wrote:
> this looks much better, but GetFocusBounds is still confusing. It looks
> like it's a virtual function on ActionableView but the override in
> TrayBackgroundView is never actually used (in the sense that
> ActionableView::OnPaintFocus, which would use the specialization, is also
> overridden). I think you should be able to just get rid of GetFocusBounds
> and calculate the proper bounds in this function (i.e.
> GetBackgroundsBounds() and the appropriate inset)

Done.  Also got rid of ActionableView::GetFocusBounds(), since it wasn't being
overridden anywhere.

[Evan Stade, 2016-12-15 21:53:11]

thanks. lgtm

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

https://codereview.chromium.org/2569773002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:575: gfx::RectF
paint_bounds(MaterialDesignController::IsShelfMaterial()
nit: instead of multiple ternaries I'd just use if/else. I think it's easier to
read and it's also slightly easier to delete when we remove non md code.

[Greg Levin, 2016-12-15 23:46:36]

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

https://codereview.chromium.org/2569773002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:575: gfx::RectF
paint_bounds(MaterialDesignController::IsShelfMaterial()
On 2016/12/15 21:53:11, Evan Stade wrote:
> nit: instead of multiple ternaries I'd just use if/else. I think it's easier
> to read and it's also slightly easier to delete when we remove non md code.

Heh... I wrote that version, then switched it back when I realized I'd have to
declare and assign |paint_bounds| on separate lines.  I'll fix it when I get to
work tomorrow.  Thanks for all the help!

[Greg Levin, 2016-12-16 17:30:43]

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

[commit-bot: I haz the power, 2016-12-16 17:31:35]

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

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

[commit-bot: I haz the power, 2016-12-16 18:28:13]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2016-12-22 19:20:12]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-12-22 19:20:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2569773002/#ps60001
(title: "Rearrange MD checks")

[commit-bot: I haz the power, 2016-12-22 19:20:22]

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-22 19:49:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482434412382640,
"parent_rev": "debe7b46ada6844171b3eef3573f93c3bc0de941", "commit_rev":
"39353cac37c09419e67f614d780948cbbd0a90b1"}

[commit-bot: I haz the power, 2016-12-22 19:49:41]

Description was changed from

==========
Fix SysTray focus rect position

BUG=672723
TEST=On login, lock, and desktop screens, with and without
--ash-md=disabled flag, move focus to SysTray, and verify that focus
rect is in correct position.
==========

to

==========
Fix SysTray focus rect position

BUG=672723
TEST=On login, lock, and desktop screens, with and without
--ash-md=disabled flag, move focus to SysTray, and verify that focus
rect is in correct position.

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

[commit-bot: I haz the power, 2016-12-22 19:49:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-22 19:52:34]

Description was changed from

==========
Fix SysTray focus rect position

BUG=672723
TEST=On login, lock, and desktop screens, with and without
--ash-md=disabled flag, move focus to SysTray, and verify that focus
rect is in correct position.

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

to

==========
Fix SysTray focus rect position

BUG=672723
TEST=On login, lock, and desktop screens, with and without
--ash-md=disabled flag, move focus to SysTray, and verify that focus
rect is in correct position.

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

[commit-bot: I haz the power, 2016-12-22 19:52:36]

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