Mac App Launcher is positioned on center of dock in certain cases.

Mac App Launcher is positioned on center of dock in certain cases.

The app launcher will be positioned on the center of the dock if the
cursor is not visible (previously would be in the lower-left corner) or
too far away from the dock (previously would be aligned with the cursor).

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

[tapted, 2013-11-29 02:12:51]

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_positioner.cc (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:92: screen_rect.y() +
screen_rect.height() / 2);
Maybe s/screen_rect/work_area? My left dock looks centred between the bottom of
the menu bar and the bottom of the screen. Same for right.
Perhaps |screen_rect| is not needed at all.

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:98: case SCREEN_EDGE_TOP:
Do we need this? I'm not sure if consistency outweighs the badness of adding
dead code [except for tests]

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:80: const int kSnapDistance =
50;
I think this should be at least window width for a left/right dock, and at least
window height for a bottom dock. It really bugs me when I use the Windows app
launcher, and I know where I need to click when it appears, but because it took
too long, the window decided not to pop up there.

[Matt Giuca, 2013-11-29 05:24:07]

Note: I haven't run tests on Mac yet. Will have to do that on Monday, as well as
responding to your last point.

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_positioner.cc (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:92: screen_rect.y() +
screen_rect.height() / 2);
On 2013/11/29 02:12:51, tapted wrote:
> Maybe s/screen_rect/work_area? My left dock looks centred between the bottom
of
> the menu bar and the bottom of the screen. Same for right.
> Perhaps |screen_rect| is not needed at all.

Done.

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:98: case SCREEN_EDGE_TOP:
I think that would be a Bad Idea. This is a general function for all platforms
to use. Just because it's only currently used by Mac should not mean it should
be made to fail in cases that aren't used on Mac.

[tapted, 2013-11-29 06:05:58]

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_positioner.cc (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:98: case SCREEN_EDGE_TOP:
On 2013/11/29 05:24:08, Matt Giuca wrote:
> I think that would be a Bad Idea. This is a general function for all platforms
> to use. Just because it's only currently used by Mac should not mean it should
> be made to fail in cases that aren't used on Mac.

I see the rationale for adding the case statement where the code currently is,
here. But are there plans for using it on other platforms? Usually code that's
only used on a single platform go into files like app_list_positioner_mac.cc .

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:80: const int kSnapDistance =
50;
On 2013/11/29 02:12:51, tapted wrote:
> I think this should be at least window width for a left/right dock, and at
least
> window height for a bottom dock. It really bugs me when I use the Windows app
> launcher, and I know where I need to click when it appears, but because it
took
> too long, the window decided not to pop up there.

Did you see this?

[Matt Giuca, 2013-12-02 05:02:02]

The new boundaries are working. Let me know if you want a demo.

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_positioner.cc (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_positioner.cc:98: case SCREEN_EDGE_TOP:
On 2013/11/29 06:05:59, tapted wrote:
> I see the rationale for adding the case statement where the code currently is,
> here. But are there plans for using it on other platforms? Usually code that's
> only used on a single platform go into files like app_list_positioner_mac.cc .

No, there aren't (although there are probably Linux environments where we'll
want this, like Xfce which centers the dock). But again, that's sort of besides
the point. The idea of centering the launcher on the shelf isn't Mac-specific,
even if it's only used by Mac. I see the role of these *_mac files as being for
code that can't be compiled on non-Mac platforms because either it uses
Mac-specific APIs, or simply doesn't make sense outside the Mac platform. This
is not one of those.

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/93863002/diff/200001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:80: const int kSnapDistance =
50;
Yes, I made a note to get to it on Monday.

Yes, I agree. Done.

[tapted, 2013-12-02 06:28:38]

lgtm unless I just threw a spanner at it. CL description should probably mention
the snap-distance goo too

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:295: dock_location ==
AppListPositioner::SCREEN_EDGE_TOP ?
nit: maybe don't worry about _TOP here? since there's a NOTREACHED() hit via
AdjustWorkAreaForDock on the next statement

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac_unittest.mm (right):

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac_unittest.mm:144:
EXPECT_TRUE(NSEqualPoints(OffsetPoint(target_origin, 0, -kAnimationOffset),
ooh. according to the description in http://crrev.com/220795, when the mouse
cursor is not visible, it should just fade out/in -- there shouldn't be an
animation offset. This is something our Mac UX rep asked for... but maybe it's
different now it's ensured to be toastering out of the Dock centre. Do you have
an opinion? Maybe we ask for a ui-review in Canary if we are ambivalent.

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac_unittest.mm:158:
PlaceDock(DOCK_LOCATION_RIGHT, false);
Does cursor_visible == false need a DOCK_LOCATION_UNKNOWN test?

[Matt Giuca, 2013-12-03 02:21:53]

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:295: dock_location ==
AppListPositioner::SCREEN_EDGE_TOP ?
I would prefer for this code to not implicitly map TOP to window_size.width(),
even if it is technically unreachable.

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac_unittest.mm (right):

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac_unittest.mm:144:
EXPECT_TRUE(NSEqualPoints(OffsetPoint(target_origin, 0, -kAnimationOffset),
I explicitly decided (although I didn't communicate this in comments or review
-- sorry!) that now that we are anchoring it to the dock, it is appropriate for
it to pop out of the dock instead of just fading in.

I'll add a comment. I'm happy to get a confirmation from UI once it's in Canary,
but I don't want it to hold up this review.

https://codereview.chromium.org/93863002/diff/310001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac_unittest.mm:158:
PlaceDock(DOCK_LOCATION_RIGHT, false);
In the code, it's the same case as cursor_visible == true with
DOCK_LOCATION_UNKNOWN, but from a strict black-box perspective, this should have
a case. Added the new case to FindAnchorPointNoDock.

[commit-bot: I haz the power, 2013-12-03 02:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/93863002/350001

[commit-bot: I haz the power, 2013-12-03 04:17:52]

Retried try job too often on mac_rel for step(s) browser_tests,
content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-04 00:14:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/93863002/350001

[commit-bot: I haz the power, 2013-12-04 02:11:05]

Change committed as 238528