Fixing the dynamic positioning (move with anchor) for the app launcher

Fixing the dynamic positioning (move with anchor) for the app launcher

The problem was that the positioning was done before it was shown and the move_with_anchor didn't work since there was no anchor. Changed it now to use an anchor offset.

BUG=284780
TEST=unittest (partial), visually [(RTL and LTR) x (left, right, bottom aligned) x (always visible, sliding into view, hidden)]

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

[Mr4D (OOO till 08-26), 2013-09-05 18:40:20]

Please have a look!

[James Cook, 2013-09-05 20:05:17]

https://codereview.chromium.org/23622020/diff/2001/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (left):

https://codereview.chromium.org/23622020/diff/2001/ash/wm/app_list_controller...
ash/wm/app_list_controller.cc:45: const int kArrowTipHeight = 10;
Nice that you found this thing to clean up.

https://codereview.chromium.org/23622020/diff/2001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/23622020/diff/2001/ui/app_list/views/app_list...
ui/app_list/views/app_list_view.h:48: const gfx::Point& anchor_point,
I don't like having a parameter that can be in two different coordinate systems.
Either add an explicit extra parameter for anchor_offset or break into two
different Init methods.

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:39: void test_set_anchor_view(View*
anchor_view) {
nit: set_anchor_view_for_test?

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:183: // If an anchor view is
provided, it's coordinates should get returned.
nit: it's -> its

[Mr4D (OOO till 08-26), 2013-09-05 20:52:00]

Please have another look!

https://codereview.chromium.org/23622020/diff/2001/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (left):

https://codereview.chromium.org/23622020/diff/2001/ash/wm/app_list_controller...
ash/wm/app_list_controller.cc:45: const int kArrowTipHeight = 10;
Sure thing!

https://codereview.chromium.org/23622020/diff/2001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/23622020/diff/2001/ui/app_list/views/app_list...
ui/app_list/views/app_list_view.h:48: const gfx::Point& anchor_point,
I did like none of the three possibilities. Okay - added another function. That
way the use case of the anchor gets at least much clearer!

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:39: void test_set_anchor_view(View*
anchor_view) {
On 2013/09/05 20:05:17, James Cook wrote:
> nit: set_anchor_view_for_test?

Done.

https://codereview.chromium.org/23622020/diff/2001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:183: // If an anchor view is
provided, it's coordinates should get returned.
On 2013/09/05 20:05:17, James Cook wrote:
> nit: it's -> its

Done.

[James Cook, 2013-09-05 21:58:55]

lgtm

[commit-bot: I haz the power, 2013-09-05 22:59:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/23622020/9001

[commit-bot: I haz the power, 2013-09-05 23:14:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mr4D (OOO till 08-26), 2013-09-05 23:18:18]

@msw, @xiyuan: Could you please do an owners review?

Many thanks in advance!

[xiyuan, 2013-09-06 00:57:40]

Is the anchor offset used to adjust the app launcher bubble to be inside the
screen? If so, maybe we should fix BubbleFrameView::GetUpdatedWindowBounds to
make it do something for bubble that has no arrow.

[xiyuan, 2013-09-06 04:34:53]

Talked with Mr4D offline. This is needed because the UI design has specific
requirements for the bubble position. Simple offscreen adjustment does not
suffice the needs. 

app_list part LGTM

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:152: void set_anchor_offset(const gfx::Point&
offset) {anchor_offset_ = offset; }
nit: the new fashion is to use Vector2d for offset.

[msw, 2013-09-06 16:31:59]

I suspect that you can accomplish the same thing with the existing
|anchor_view_insets_| and it would be even more correct. |anchor_view_insets_|
can be used for shrinking or expanding the anchor view's rect to offset the
bubble position, and is already used by app_list_view.cc. Please try to fix that
existing client usage instead of adding new highly-similar infrastructure.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:260: anchor_bounds.set_x(anchor_bounds.x() +
anchor_offset_.x());
Use Rect::Offset after you make anchor_offset_ a vector.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:37: // Accessors for the protected
anchor functionality - avoiding making them
nit: make the test a friend instead of wrapping these accessors.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:177:
EXPECT_EQ(static_anchor_rect.ToString(),
Doesn't Rect have an equality operator? Why do you need ToString? Ditto below.

[Mr4D (OOO till 08-26), 2013-09-06 17:57:31]

Addressed: Added the Vector2d, removed BubbleDelegate changes as requested.

Sidenote: I am not really a fan of "misusing" one parameter to avoid introducing
another clearer one, but since it duplicates functionality and it simplifies
things for this patch it was possibly a good thing to do it that way.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:260: anchor_bounds.set_x(anchor_bounds.x() +
anchor_offset_.x());
Not required anymore - removed

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.h:152: void set_anchor_offset(const gfx::Point&
offset) {anchor_offset_ = offset; }
Done.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:37: // Accessors for the protected
anchor functionality - avoiding making them
I was trying to not touch the class and instead add what is needed here. It
feels much cleaner to let tests change what they need in their realm then
changing class declarations for production code. But due to your other change
request this isn't necessary anyways. Removed.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:177:
EXPECT_EQ(static_anchor_rect.ToString(),
Yes, it does - but when you get a failure it also tells you what is and what is
expected which is much more helpful then only "it failed".

[Mr4D (OOO till 08-26), 2013-09-06 18:00:29]

-msw (since not needed anymore)
+benwells since I need an owners review

[msw, 2013-09-06 18:18:41]

On 2013/09/06 17:57:31, Mr4D wrote:
> Addressed: Added the Vector2d, removed BubbleDelegate changes as requested.
> 
> Sidenote: I am not really a fan of "misusing" one parameter to avoid
introducing
> another clearer one, but since it duplicates functionality and it simplifies
> things for this patch it was possibly a good thing to do it that way.

Thanks for addressing my overarching comment; I think the end result is better.
I don't think this is really misusing the insets, since they effectively create
a padding space around the button, which seems more or less like what's needed.
afaik, the effective difference between applying insets vs. offsets would be the
off-screen placement of the bubble, which shouldn't be reached in any case.

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:37: // Accessors for the protected
anchor functionality - avoiding making them
On 2013/09/06 17:57:31, Mr4D wrote:
> I was trying to not touch the class and instead add what is needed here. It
> feels much cleaner to let tests change what they need in their realm then
> changing class declarations for production code. But due to your other change
> request this isn't necessary anyways. Removed.

Moot, but I hoped you could add that friend test decl here (test code).

https://codereview.chromium.org/23622020/diff/9001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate_unittest.cc:177:
EXPECT_EQ(static_anchor_rect.ToString(),
On 2013/09/06 17:57:31, Mr4D wrote:
> Yes, it does - but when you get a failure it also tells you what is and what
is
> expected which is much more helpful then only "it failed".

Moot, but EXPECT_EQ does actually output mismatching rect values, albeit it's
not printed very pretty. I just ran a quick experiment:

  const gfx::Rect expected_rect(1, 2, 3, 4);
  const gfx::Rect actual_rect(5, 6, 7, 8);
  EXPECT_EQ(expected_rect, actual_rect);

That outputs: ..._unittest.cc(160): error: Value of: actual_rect
  Actual: 16-byte object <05-00 00-00 06-00 00-00 07-00 00-00 08-00 00-00>
Expected: expected_rect
Which is: 16-byte object <01-00 00-00 02-00 00-00 03-00 00-00 04-00 00-00>

[Mr4D (OOO till 08-26), 2013-09-08 16:01:05]

Since benwells is travelling, maybe koz can have a quick look at the owners
review? Only one line has changed. Many thanks in advance!

[benwells, 2013-09-09 15:11:03]

lgtm

[commit-bot: I haz the power, 2013-09-09 15:30:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/23622020/47001

[commit-bot: I haz the power, 2013-09-09 15:38:42]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-09 16:29:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/23622020/47001

[commit-bot: I haz the power, 2013-09-10 02:43:08]

Change committed as 222191