[win] Ensure the app launcher always comes up on screen.

[win] Ensure the app launcher always comes up on screen.

BUG=178574
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185110

[tapted, 2013-02-27 08:41:59]

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:557: const
gfx::Rect& screen_rect,
bounds_rect, Then,

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:562: if (anchor->x()
- offset.width() < screen_rect.x())
I think these 11 lines can be just

anchor.ClampToMin(bounds_rect.bottom_left());
anchor.ClampToMax(bounds_rect.top_right());

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:587: const
gfx::Rect& screen_rect = display.bounds();
With

gfx::Rect& bounds_rect = display.bounds();
bounds_rect.Inset(float_offset.width(), float_offset.height());

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:587: const
gfx::Rect& screen_rect = display.bounds();
Does it need to take into account the taskbar, to ensure there is no overlap?
(My brain can't really come up with a scenario where that wouldn't be taken care
of by other the things below).

[tapted, 2013-02-27 08:42:30]

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12334115/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:585:
float_offset.Enlarge(kSnapOffset, kSnapOffset);
[ I think with this Enlarge, it also gets a margin around the edge of the
screen. i.e., it won't be flush against the edge of screen. Intentional? (if no,
this could be moved below the "Inset" in the next comment..) ]

[benwells, 2013-02-27 11:00:47]

Great suggestions Trent! I've refactored it quite a bit now, hopefully it is
easier to grok.

[tapted, 2013-02-27 11:58:48]

nice! lgtm

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:544: gfx::Point*
anchor) {
i think |anchor| can be const reference now

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:603:
anchor->SetPoint(ref_point.x(), ref_point.y());
I don't... think default assignment operator has been hidden, so I'd probably go
for 

*anchor = FindReferencePoint(..);

but there might be a different prevailing style/opinion in Chrome.

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:604:
anchor->ClampToMin(bounds_rect.origin());
ah, origin - of course ;). (Cocoa has had me thinking upside-down)

[benwells, 2013-02-27 22:35:41]

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:544: gfx::Point*
anchor) {
On 2013/02/27 11:58:48, tapted wrote:
> i think |anchor| can be const reference now

Yeah, and in the next function. Done.

https://codereview.chromium.org/12334115/diff/2005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:603:
anchor->SetPoint(ref_point.x(), ref_point.y());
On 2013/02/27 11:58:48, tapted wrote:
> I don't... think default assignment operator has been hidden, so I'd probably
go
> for 
> 
> *anchor = FindReferencePoint(..);
> 
> but there might be a different prevailing style/opinion in Chrome.

Yep, good one.

[commit-bot: I haz the power, 2013-02-27 22:38:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/12334115/12001

[commit-bot: I haz the power, 2013-02-28 02:13:13]

Change committed as 185110