Position apps list on Windows near the cursor.

Position apps list on Windows near the cursor.

This change makes the app list on Windows appear either above or below the cursor. Above the cursor is preferred but the list will be moved below the cursor if there is not enough room.

BUG=138632
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158375

[sky, 2012-09-18 18:03:13]

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:108: static const
int min_space_y = 600;
Where does this come from?

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:113: gfx::Rect
display_rect = display.work_area();
const gfx::Rect&

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:117: if (cursor.y()
- display_rect.y() < min_space_y &&
Why does this check both above and below?

[benwells, 2012-09-19 01:02:25]

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:108: static const
int min_space_y = 600;
On 2012/09/18 18:03:13, sky wrote:
> Where does this come from?

I measured the height of the view and added a bit. I'd like to calculate this
instead but the code that works out the height is quite deep in the view - the
content view works out its ideal size by first asking it's grid view, etc.

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:113: gfx::Rect
display_rect = display.work_area();
On 2012/09/18 18:03:13, sky wrote:
> const gfx::Rect&

Done.

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:117: if (cursor.y()
- display_rect.y() < min_space_y &&
On 2012/09/18 18:03:13, sky wrote:
> Why does this check both above and below?

It moves the view below the anchor point if there is not enough room above, and
if there is enough room below.

I've made this smarter by also using vertical arrows if there is not enough room
above or below, and hopefully made the logic nicer.

[sky, 2012-09-19 13:44:53]

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10937008/diff/1/chrome/browser/ui/views/app_li...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:108: static const
int min_space_y = 600;
On 2012/09/19 01:02:25, benwells wrote:
> On 2012/09/18 18:03:13, sky wrote:
> > Where does this come from?
> 
> I measured the height of the view and added a bit. I'd like to calculate this
> instead but the code that works out the height is quite deep in the view - the
> content view works out its ideal size by first asking it's grid view, etc.

WHy can't you ask for the preferred size?

[benwells, 2012-09-21 23:26:51]

Oops forgot to publish this.

Using preferred size now, I didn't realize I could do that. It needed a bit of
rejigging to use it.

[sky, 2012-09-21 23:36:01]

LGTM

http://codereview.chromium.org/10937008/diff/11001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10937008/diff/11001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:134: static const
int arrow_size = 10;
If you're going with static const, these should be kArrowSize.

[benwells, 2012-09-21 23:37:42]

On 2012/09/21 23:36:01, sky wrote:
> LGTM
> 
>
http://codereview.chromium.org/10937008/diff/11001/chrome/browser/ui/views/ap...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
http://codereview.chromium.org/10937008/diff/11001/chrome/browser/ui/views/ap...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:134: static const
> int arrow_size = 10;
> If you're going with static const, these should be kArrowSize.

Ah, I'd been asked to change to linux hacker style in a previous review, good to
know which way it should be.

[commit-bot: I haz the power, 2012-09-24 16:37:22]

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

[commit-bot: I haz the power, 2012-09-24 16:37:34]

Presubmit check for 10937008-17001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ppapi/native_client

Presubmit checks took 4.1s to calculate.

[commit-bot: I haz the power, 2012-09-24 16:42:05]

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

[commit-bot: I haz the power, 2012-09-24 20:42:29]

Change committed as 158375