Fix app list position in windows

Fix app list position in windows

BubbleDelegate::GetAnchorRect() should not inset an anchor point. This was
causing the app list to appear behind the taskbar.


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

[calamity, 2013-03-28 05:29:10]

+msw for OWNERS

[benwells, 2013-03-28 05:43:18]

lgtm with a nit

https://codereview.chromium.org/13142003/diff/1/ui/views/bubble/bubble_delega...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/13142003/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:239: return gfx::Rect(anchor_point_,
gfx::Size());
nit: no need for the else here, as the if() has a return.

That is, do it like this:

if (anchor_view()) {
  ...
  return ...;
}

return ....;

[msw, 2013-03-28 05:48:03]

+varunjain, the original author of this code, for review.
His http://crrev.com/188285 directly contradicts your claim.
Could you instead set the anchor_insets to none (gfx::Insets()) in your case?

[benwells, 2013-03-28 06:07:51]

On 2013/03/28 05:48:03, msw wrote:
> +varunjain, the original author of this code, for review.
> His http://crrev.com/188285 directly contradicts your claim.
> Could you instead set the anchor_insets to none (gfx::Insets()) in your case?

Ah, that's why it broke! Sorry, we should have checked the history.

We could do that. But I'm wondering about the original change to "Apply insets
to |anchor_point|. This give the user the ability to specify a
generic "anchor_rect" (as a combination of center point and insets) instead of
either a |anchor_point| or an |anchor_view|". Are any views using this? It seems
unintuitive, as you would need to use negative insets to get a real rectangle.

[msw, 2013-03-28 06:20:26]

On 2013/03/28 06:07:51, benwells wrote:
> On 2013/03/28 05:48:03, msw wrote:
> > +varunjain, the original author of this code, for review.
> > His http://crrev.com/188285 directly contradicts your claim.
> > Could you instead set the anchor_insets to none (gfx::Insets()) in your
case?
> 
> Ah, that's why it broke! Sorry, we should have checked the history.
> 
> We could do that. But I'm wondering about the original change to "Apply insets
> to |anchor_point|. This give the user the ability to specify a
> generic "anchor_rect" (as a combination of center point and insets) instead of
> either a |anchor_point| or an |anchor_view|". Are any views using this? It
seems
> unintuitive, as you would need to use negative insets to get a real rectangle.

It looks like there are about 16 uses of set_anchor_insets:
https://code.google.com/p/chromium/codesearch#search/&q=set_anchor_insets
One of which is AppListView:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
You can also override GetAnchorRect itself if the |anchor_point|/|anchor_view| +
|anchor_insets| is insufficient for your needs.

[benwells, 2013-03-28 06:42:31]

On 2013/03/28 06:20:26, msw wrote:
> On 2013/03/28 06:07:51, benwells wrote:
> > On 2013/03/28 05:48:03, msw wrote:
> > > +varunjain, the original author of this code, for review.
> > > His http://crrev.com/188285 directly contradicts your claim.
> > > Could you instead set the anchor_insets to none (gfx::Insets()) in your
> case?
> > 
> > Ah, that's why it broke! Sorry, we should have checked the history.
> > 
> > We could do that. But I'm wondering about the original change to "Apply
insets
> > to |anchor_point|. This give the user the ability to specify a
> > generic "anchor_rect" (as a combination of center point and insets) instead
of
> > either a |anchor_point| or an |anchor_view|". Are any views using this? It
> seems
> > unintuitive, as you would need to use negative insets to get a real
rectangle.
> 
> It looks like there are about 16 uses of set_anchor_insets:
> https://code.google.com/p/chromium/codesearch#search/&q=set_anchor_insets
> One of which is AppListView:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
> You can also override GetAnchorRect itself if the |anchor_point|/|anchor_view|
+
> |anchor_insets| is insufficient for your needs.

There are only three users of set_anchor_point, the app list,
touch_editing_menu.cc and zoom_bubble_view.cc.

It looks like varunjain's change was made to support touch_editing_menu.cc,
which uses set_anchor_point and negative insets (which are generated from an
anchor_rect).

This change changed the behavior of app_list_view, which we can fix next week by
removing the insets (it is a holiday in Australia on Friday and is Thursday
night now).

The change will have also changed the behavior of zoom_bubble_view which has an
anchor point and insets when it goes into full screen mode. I don't know if that
is a problem or not.

[benwells, 2013-03-28 06:51:48]

On 2013/03/28 06:42:31, benwells wrote:
> On 2013/03/28 06:20:26, msw wrote:
> > On 2013/03/28 06:07:51, benwells wrote:
> > > On 2013/03/28 05:48:03, msw wrote:
> > > > +varunjain, the original author of this code, for review.
> > > > His http://crrev.com/188285 directly contradicts your claim.
> > > > Could you instead set the anchor_insets to none (gfx::Insets()) in your
> > case?
> > > 
> > > Ah, that's why it broke! Sorry, we should have checked the history.
> > > 
> > > We could do that. But I'm wondering about the original change to "Apply
> insets
> > > to |anchor_point|. This give the user the ability to specify a
> > > generic "anchor_rect" (as a combination of center point and insets)
instead
> of
> > > either a |anchor_point| or an |anchor_view|". Are any views using this? It
> > seems
> > > unintuitive, as you would need to use negative insets to get a real
> rectangle.
> > 
> > It looks like there are about 16 uses of set_anchor_insets:
> > https://code.google.com/p/chromium/codesearch#search/&q=set_anchor_insets
> > One of which is AppListView:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
> > You can also override GetAnchorRect itself if the
|anchor_point|/|anchor_view|
> +
> > |anchor_insets| is insufficient for your needs.
> 
> There are only three users of set_anchor_point, the app list,
> touch_editing_menu.cc and zoom_bubble_view.cc.
> 
> It looks like varunjain's change was made to support touch_editing_menu.cc,
> which uses set_anchor_point and negative insets (which are generated from an
> anchor_rect).
> 
> This change changed the behavior of app_list_view, which we can fix next week
by
> removing the insets (it is a holiday in Australia on Friday and is Thursday
> night now).
> 
> The change will have also changed the behavior of zoom_bubble_view which has
an
> anchor point and insets when it goes into full screen mode. I don't know if
that
> is a problem or not.

Maybe instead of using a point and insets to specify a rect, we could remove
set_anchor_point and just have set_anchor_rect? That could specify a point or a
rect easily, and feels more intuitive.

[msw, 2013-03-28 06:52:47]

On 2013/03/28 06:42:31, benwells wrote:
> On 2013/03/28 06:20:26, msw wrote:
> > On 2013/03/28 06:07:51, benwells wrote:
> > > On 2013/03/28 05:48:03, msw wrote:
> > > > +varunjain, the original author of this code, for review.
> > > > His http://crrev.com/188285 directly contradicts your claim.
> > > > Could you instead set the anchor_insets to none (gfx::Insets()) in your
> > case?
> > > 
> > > Ah, that's why it broke! Sorry, we should have checked the history.
> > > 
> > > We could do that. But I'm wondering about the original change to "Apply
> insets
> > > to |anchor_point|. This give the user the ability to specify a
> > > generic "anchor_rect" (as a combination of center point and insets)
instead
> of
> > > either a |anchor_point| or an |anchor_view|". Are any views using this? It
> > seems
> > > unintuitive, as you would need to use negative insets to get a real
> rectangle.
> > 
> > It looks like there are about 16 uses of set_anchor_insets:
> > https://code.google.com/p/chromium/codesearch#search/&q=set_anchor_insets
> > One of which is AppListView:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
> > You can also override GetAnchorRect itself if the
|anchor_point|/|anchor_view|
> +
> > |anchor_insets| is insufficient for your needs.
> 
> There are only three users of set_anchor_point, the app list,
> touch_editing_menu.cc and zoom_bubble_view.cc.
> 
> It looks like varunjain's change was made to support touch_editing_menu.cc,
> which uses set_anchor_point and negative insets (which are generated from an
> anchor_rect).
> 
> This change changed the behavior of app_list_view, which we can fix next week
by
> removing the insets (it is a holiday in Australia on Friday and is Thursday
> night now).
> 
> The change will have also changed the behavior of zoom_bubble_view which has
an
> anchor point and insets when it goes into full screen mode. I don't know if
that
> is a problem or not.

Gah, you're right, I counted uses with |anchor_view| instead of just
|anchor_point|, sorry.
Adjusting (negating) the AppListView insets (and point?) is likely the right way
to go, but feel free to explore better solutions that support the other two
bubbles' use cases.

[msw, 2013-03-28 06:55:15]

> Maybe instead of using a point and insets to specify a rect, we could remove
> set_anchor_point and just have set_anchor_rect? That could specify a point or
a
> rect easily, and feels more intuitive.

We'll still need insets for anchor_view, but switching point to rect sgtm.

[varunjain, 2013-03-28 13:02:11]

On 2013/03/28 06:55:15, msw wrote:
> > Maybe instead of using a point and insets to specify a rect, we could remove
> > set_anchor_point and just have set_anchor_rect? That could specify a point
or
> a
> > rect easily, and feels more intuitive.
> 
> We'll still need insets for anchor_view, but switching point to rect sgtm.

sorry for causing the regression. I was under the impression that
set_anchor_inset is never used with set_anchor_point (except for the usage I
added in touch_editing_menu). I remember checking both app_list_view and
zoom_bubble_view while looking for set_anchor_point. Somehow I missed the insets
call at both places :(
Changing point to rect sgtm too. It seems like a big change though. Another fix
could be to check in GetAnchorRect that if anchor_view is NULL, apply the insets
only when they are negative (the intent of having an anchor_point with insets
was to specify a rectangle, which is only possible with negative insets).

[benwells, 2013-04-02 03:52:33]

lgtm

[calamity, 2013-04-02 04:33:20]

Changed from anchor_point to anchor_rect and reverted varunjain's change to
GetAnchorRect().

+sky for OWNERS
Could you please review chrome/browser/ui/views, ash/launcher and
ui/views/touchui (all just function name changes). Thanks.

[sky, 2013-04-02 14:57:45]

LGTM

[msw, 2013-04-02 19:49:01]

LGTM with nits (please address).

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226:
kLocationIconVerticalMargin, 0));
nit: indent to match.

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295:
kLocationIconVerticalMargin, 0));
nit: indent to match.

https://codereview.chromium.org/13142003/diff/12002/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/13142003/diff/12002/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:144: // The anchor rect or anchor view must be
set before calling CreateBubble or
nit, just remove this comment and the blank line between set_anchor_view and
set_anchor_rect; the comment above is sufficient.

[calamity, 2013-04-02 23:37:00]

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226:
kLocationIconVerticalMargin, 0));
On 2013/04/02 19:49:01, msw wrote:
> nit: indent to match.

Done.

https://codereview.chromium.org/13142003/diff/12002/chrome/browser/ui/views/w...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:295:
kLocationIconVerticalMargin, 0));
On 2013/04/02 19:49:01, msw wrote:
> nit: indent to match.

Done.

https://codereview.chromium.org/13142003/diff/12002/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate.h (right):

https://codereview.chromium.org/13142003/diff/12002/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate.h:144: // The anchor rect or anchor view must be
set before calling CreateBubble or
On 2013/04/02 19:49:01, msw wrote:
> nit, just remove this comment and the blank line between set_anchor_view and
> set_anchor_rect; the comment above is sufficient.

Done.

[commit-bot: I haz the power, 2013-04-02 23:37:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/13142003/20001

[commit-bot: I haz the power, 2013-04-03 05:03:54]

Change committed as 191990