[MacViews] Anchor Bookmarks bubble to top-right corner.

[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209
Committed: https://crrev.com/a9f2053a36b4899fb312bec01a3362f65eafe785
Cr-Commit-Position: refs/heads/master@{#419877}

[Eugene But (OOO till 7-30), 2016-09-19 04:58:21]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 04:58:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 05:38:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 05:38:52]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2016-09-19 05:45:38]

eugenebut@chromium.org changed reviewers:
+ ellyjones@chromium.org, tapted@chromium.org

[Eugene But (OOO till 7-30), 2016-09-19 05:45:38]

I don't know if it's OK to make this change for Windows (and I don't have a PC
to check). Do you think this should be put behind OS_MACOSX flags?

[tapted, 2016-09-19 07:24:35]

wow this is tricky...

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:84:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
ah! So I think the platform difference happens because on MacViews, anchor_view
is null -- we anchor to a point instead (this is because the anchor view would
be an NSView, not a views::View)

But that means we hit 

LocationBarBubbleDelegateView::LocationBarBubbleDelegateView(
    views::View* anchor_view,
    content::WebContents* web_contents)
    : BubbleDialogDelegateView(anchor_view,
                               anchor_view ? views::BubbleBorder::TOP_RIGHT
                                           : views::BubbleBorder::NONE)


I think the original use-case for this was for fullscreen, for cases where there
is no toolbar (and things like Ctrl+d to add a bookmark still need to work, and
should use arrow:NONE). Except... on Mac there's no such thing as toolbar-less
fullscreen: Cmd+d in full screen will slide in a toolbar.

That means adding this set_arrow call might not do the right thing for
fullscreen on other platforms. But! When I go to https://permission.site on
ChromeOS, Fullscreen, Ctrl+D I still get an anchor. Same for the other requests.
It just displays in a randomish place, but not "centered".

I can think of two ways to fix this that feel "right"

1) Just remove the ternary-if-else in the LocationBarBubbleDelegateView
initializer -- always use TOP_RIGHT, regardless of whether there's an
anchor_view. [This will be "wrong" if anything on non-Mac is actually passing a
null anchor_view (and relies on a NONE anchor) but I don't think anything does -
E.g. everything in fullscreen seems to have a non-NONE anchor as far as I can
tell.. but this might need some investigation.

2) Encapsulate the mac-specific logic in LocationBarBubbleDelegateView. E.g.
LocationBarBubbleDelegateView initializer becomes 
: BubbleDialogDelegateView(anchor_view, ArrowForAnchorView(anchor_view))

With (in an anonymous namespace)

views::BubbleBorder::Arrow ArrowForAnchorView(views::View* anchor_view) {
#if defined(OS_MACOSX)
  // On Mac there is no toolbar-less fullscreen: it will slide in when a bubble
is shown. However,
  // |anchor_view| is null when the parent window is a Cocoa browser.
  return views::BubbleBorder::TOP_RIGHT;
#else
  // On other platforms, assume a null anchor_view means fullscreen, and
position under the
  // supplied anchor rectangle.
  return anchor_view ? views::BubbleBorder::TOP_RIGHT :
views::BubbleBorder::NONE;
#endif
}


2) might be the "safer" approach, but (1) is a lot neater if nothing truly
depends on this logic. Looks like it was added in
https://codereview.chromium.org/795053003 (or, in fact, before that for the zoom
bubble) and then forgotten about. Certainly in ChromeOS, ZoomBubble and
Bookmarks Bubble anchor differently in fullscreen, but don't in regular windows,
so this either regressed or never worked properly. So if we can do (1) without
changing the behaviour of the zoom bubble then that's probably the right fix.

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:86:
bookmark_bubble_->set_mirror_arrow_in_rtl(false);
I think we can just omit this for now. The problem is that when building with
`mac_views_browser = true` in `gn args`, then flipping is the correct thing to
do -- this means it can't just be an #ifdef MAC. So anchoring may be off in RTL
- but the fix is a bit orthogonal since it is Cocoa specific -- "fixing" RTL on
Cocoa may mean moving the star to the left side - only if the decision is made
to not do that should we disable the flip, but the tweak needs to be more
isolated. (the hacky fix would be to just move this line to the !anchor_view
block, with an #ifdef, but that would regress if the star moves later on).

[Eugene But (OOO till 7-30), 2016-09-19 08:12:30]

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:84:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
On 2016/09/19 07:24:35, tapted wrote:
> ah! So I think the platform difference happens because on MacViews,
anchor_view
> is null -- we anchor to a point instead (this is because the anchor view would
> be an NSView, not a views::View)
> 
> But that means we hit 
> 
> LocationBarBubbleDelegateView::LocationBarBubbleDelegateView(
>     views::View* anchor_view,
>     content::WebContents* web_contents)
>     : BubbleDialogDelegateView(anchor_view,
>                                anchor_view ? views::BubbleBorder::TOP_RIGHT
>                                            : views::BubbleBorder::NONE)
> 
> 
> I think the original use-case for this was for fullscreen, for cases where
there
> is no toolbar (and things like Ctrl+d to add a bookmark still need to work,
and
> should use arrow:NONE). Except... on Mac there's no such thing as toolbar-less
> fullscreen: Cmd+d in full screen will slide in a toolbar.
> 
> That means adding this set_arrow call might not do the right thing for
> fullscreen on other platforms. But! When I go to https://permission.site on
> ChromeOS, Fullscreen, Ctrl+D I still get an anchor. Same for the other
requests.
> It just displays in a randomish place, but not "centered".
> 
> I can think of two ways to fix this that feel "right"
> 
> 1) Just remove the ternary-if-else in the LocationBarBubbleDelegateView
> initializer -- always use TOP_RIGHT, regardless of whether there's an
> anchor_view. [This will be "wrong" if anything on non-Mac is actually passing
a
> null anchor_view (and relies on a NONE anchor) but I don't think anything does
-
> E.g. everything in fullscreen seems to have a non-NONE anchor as far as I can
> tell.. but this might need some investigation.
> 
> 2) Encapsulate the mac-specific logic in LocationBarBubbleDelegateView. E.g.
> LocationBarBubbleDelegateView initializer becomes 
> : BubbleDialogDelegateView(anchor_view, ArrowForAnchorView(anchor_view))
> 
> With (in an anonymous namespace)
> 
> views::BubbleBorder::Arrow ArrowForAnchorView(views::View* anchor_view) {
> #if defined(OS_MACOSX)
>   // On Mac there is no toolbar-less fullscreen: it will slide in when a
bubble
> is shown. However,
>   // |anchor_view| is null when the parent window is a Cocoa browser.
>   return views::BubbleBorder::TOP_RIGHT;
> #else
>   // On other platforms, assume a null anchor_view means fullscreen, and
> position under the
>   // supplied anchor rectangle.
>   return anchor_view ? views::BubbleBorder::TOP_RIGHT :
> views::BubbleBorder::NONE;
> #endif
> }
> 
> 
> 2) might be the "safer" approach, but (1) is a lot neater if nothing truly
> depends on this logic. Looks like it was added in
> https://codereview.chromium.org/795053003 (or, in fact, before that for the
zoom
> bubble) and then forgotten about. Certainly in ChromeOS, ZoomBubble and
> Bookmarks Bubble anchor differently in fullscreen, but don't in regular
windows,
> so this either regressed or never worked properly. So if we can do (1) without
> changing the behaviour of the zoom bubble then that's probably the right fix.
Why do you think that making a fix in LocationBarBubbleDelegateView is the
"right" way? Why is it ok for LocationBarBubbleDelegateView to make assumptions
that bubble is shown on the right side of the window (consider Omnibox Info
Bubble which is shown on the left side)? bookmark_bubble_view.cc knows that this
bubble should be anchored to top-right corner and can be responsible for
positioning decision.

On mac adding bookmarks for fullscreen looks buggy with and without this change.
The dialog is just shown on top and covers the omnibox. But I would say that is
a separate bug (bookmarks bubble should be shown only after toolbar animation is
finished)

I guess I should fix the way to test this on Windows and Chrome OS.

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:86:
bookmark_bubble_->set_mirror_arrow_in_rtl(false);
On 2016/09/19 07:24:35, tapted wrote:
> I think we can just omit this for now. The problem is that when building with
> `mac_views_browser = true` in `gn args`, then flipping is the correct thing to
> do -- this means it can't just be an #ifdef MAC. So anchoring may be off in
RTL
> - but the fix is a bit orthogonal since it is Cocoa specific -- "fixing" RTL
on
> Cocoa may mean moving the star to the left side - only if the decision is made
> to not do that should we disable the flip, but the tweak needs to be more
> isolated. (the hacky fix would be to just move this line to the !anchor_view
> block, with an #ifdef, but that would regress if the star moves later on).
Omitting this line would make Mac-Views Bookmarks bubble totally unusable in RTL
(it will be anchored to top left and will be rendered offscreen). From your
comments I'm not sure how this line of code make things worse. Could you please
explain the use case where |set_mirror_arrow_in_rtl(false)| would lead to
incorrect layout.

[Eugene But (OOO till 7-30), 2016-09-20 00:01:37]

Description was changed from

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. This change sets arrow to top right and
turns off RTL mirroring (because toolbar does not support RTL yet and
omnibox is not mirrored).

BUG=600209
==========

to

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. This change sets arrow to top right.

This change does not impact windows but makes macviews version
worse in RTL, because omnibox is not mirrored on mac. Since omnibox
will be mirrored eventually, leaving it as it is.

BUG=600209
==========

[Eugene But (OOO till 7-30), 2016-09-20 00:02:51]

Thanks for review. PTAL

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:84:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
On 2016/09/19 08:12:30, Eugene But wrote:
> On 2016/09/19 07:24:35, tapted wrote:
> > ah! So I think the platform difference happens because on MacViews,
> anchor_view
> > is null -- we anchor to a point instead (this is because the anchor view
would
> > be an NSView, not a views::View)
> > 
> > But that means we hit 
> > 
> > LocationBarBubbleDelegateView::LocationBarBubbleDelegateView(
> >     views::View* anchor_view,
> >     content::WebContents* web_contents)
> >     : BubbleDialogDelegateView(anchor_view,
> >                                anchor_view ? views::BubbleBorder::TOP_RIGHT
> >                                            : views::BubbleBorder::NONE)
> > 
> > 
> > I think the original use-case for this was for fullscreen, for cases where
> there
> > is no toolbar (and things like Ctrl+d to add a bookmark still need to work,
> and
> > should use arrow:NONE). Except... on Mac there's no such thing as
toolbar-less
> > fullscreen: Cmd+d in full screen will slide in a toolbar.
> > 
> > That means adding this set_arrow call might not do the right thing for
> > fullscreen on other platforms. But! When I go to https://permission.site on
> > ChromeOS, Fullscreen, Ctrl+D I still get an anchor. Same for the other
> requests.
> > It just displays in a randomish place, but not "centered".
> > 
> > I can think of two ways to fix this that feel "right"
> > 
> > 1) Just remove the ternary-if-else in the LocationBarBubbleDelegateView
> > initializer -- always use TOP_RIGHT, regardless of whether there's an
> > anchor_view. [This will be "wrong" if anything on non-Mac is actually
passing
> a
> > null anchor_view (and relies on a NONE anchor) but I don't think anything
does
> -
> > E.g. everything in fullscreen seems to have a non-NONE anchor as far as I
can
> > tell.. but this might need some investigation.
> > 
> > 2) Encapsulate the mac-specific logic in LocationBarBubbleDelegateView. E.g.
> > LocationBarBubbleDelegateView initializer becomes 
> > : BubbleDialogDelegateView(anchor_view, ArrowForAnchorView(anchor_view))
> > 
> > With (in an anonymous namespace)
> > 
> > views::BubbleBorder::Arrow ArrowForAnchorView(views::View* anchor_view) {
> > #if defined(OS_MACOSX)
> >   // On Mac there is no toolbar-less fullscreen: it will slide in when a
> bubble
> > is shown. However,
> >   // |anchor_view| is null when the parent window is a Cocoa browser.
> >   return views::BubbleBorder::TOP_RIGHT;
> > #else
> >   // On other platforms, assume a null anchor_view means fullscreen, and
> > position under the
> >   // supplied anchor rectangle.
> >   return anchor_view ? views::BubbleBorder::TOP_RIGHT :
> > views::BubbleBorder::NONE;
> > #endif
> > }
> > 
> > 
> > 2) might be the "safer" approach, but (1) is a lot neater if nothing truly
> > depends on this logic. Looks like it was added in
> > https://codereview.chromium.org/795053003 (or, in fact, before that for the
> zoom
> > bubble) and then forgotten about. Certainly in ChromeOS, ZoomBubble and
> > Bookmarks Bubble anchor differently in fullscreen, but don't in regular
> windows,
> > so this either regressed or never worked properly. So if we can do (1)
without
> > changing the behaviour of the zoom bubble then that's probably the right
fix.
> Why do you think that making a fix in LocationBarBubbleDelegateView is the
> "right" way? Why is it ok for LocationBarBubbleDelegateView to make
assumptions
> that bubble is shown on the right side of the window (consider Omnibox Info
> Bubble which is shown on the left side)? bookmark_bubble_view.cc knows that
this
> bubble should be anchored to top-right corner and can be responsible for
> positioning decision.
> 
> On mac adding bookmarks for fullscreen looks buggy with and without this
change.
> The dialog is just shown on top and covers the omnibox. But I would say that
is
> a separate bug (bookmarks bubble should be shown only after toolbar animation
is
> finished)
> 
> I guess I should fix the way to test this on Windows and Chrome OS.
> 
> 
> 
This change does not break fullscreen on Windows.

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:86:
bookmark_bubble_->set_mirror_arrow_in_rtl(false);
On 2016/09/19 08:12:30, Eugene But wrote:
> On 2016/09/19 07:24:35, tapted wrote:
> > I think we can just omit this for now. The problem is that when building
with
> > `mac_views_browser = true` in `gn args`, then flipping is the correct thing
to
> > do -- this means it can't just be an #ifdef MAC. So anchoring may be off in
> RTL
> > - but the fix is a bit orthogonal since it is Cocoa specific -- "fixing" RTL
> on
> > Cocoa may mean moving the star to the left side - only if the decision is
made
> > to not do that should we disable the flip, but the tweak needs to be more
> > isolated. (the hacky fix would be to just move this line to the !anchor_view
> > block, with an #ifdef, but that would regress if the star moves later on).
> Omitting this line would make Mac-Views Bookmarks bubble totally unusable in
RTL
> (it will be anchored to top left and will be rendered offscreen). From your
> comments I'm not sure how this line of code make things worse. Could you
please
> explain the use case where |set_mirror_arrow_in_rtl(false)| would lead to
> incorrect layout.
Ok, this this change indeed breaks Windows in RTL, so removing this line.

[tapted, 2016-09-20 01:37:13]

lgtm with a comment and CL description tweak

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:84:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
On 2016/09/20 00:02:51, Eugene But wrote:
> On 2016/09/19 08:12:30, Eugene But wrote:
> > On 2016/09/19 07:24:35, tapted wrote:
> > > ah! So I think the platform difference happens because on MacViews,
> > anchor_view
> > > is null -- we anchor to a point instead (this is because the anchor view
> would
> > > be an NSView, not a views::View)
> > > 
> > > But that means we hit 
> > > 
> > > LocationBarBubbleDelegateView::LocationBarBubbleDelegateView(
> > >     views::View* anchor_view,
> > >     content::WebContents* web_contents)
> > >     : BubbleDialogDelegateView(anchor_view,
> > >                                anchor_view ?
views::BubbleBorder::TOP_RIGHT
> > >                                            : views::BubbleBorder::NONE)
> > > 
> > > 
> > > I think the original use-case for this was for fullscreen, for cases where
> > there
> > > is no toolbar (and things like Ctrl+d to add a bookmark still need to
work,
> > and
> > > should use arrow:NONE). Except... on Mac there's no such thing as
> toolbar-less
> > > fullscreen: Cmd+d in full screen will slide in a toolbar.
> > > 
> > > That means adding this set_arrow call might not do the right thing for
> > > fullscreen on other platforms. But! When I go to https://permission.site
on
> > > ChromeOS, Fullscreen, Ctrl+D I still get an anchor. Same for the other
> > requests.
> > > It just displays in a randomish place, but not "centered".
> > > 
> > > I can think of two ways to fix this that feel "right"
> > > 
> > > 1) Just remove the ternary-if-else in the LocationBarBubbleDelegateView
> > > initializer -- always use TOP_RIGHT, regardless of whether there's an
> > > anchor_view. [This will be "wrong" if anything on non-Mac is actually
> passing
> > a
> > > null anchor_view (and relies on a NONE anchor) but I don't think anything
> does
> > -
> > > E.g. everything in fullscreen seems to have a non-NONE anchor as far as I
> can
> > > tell.. but this might need some investigation.
> > > 
> > > 2) Encapsulate the mac-specific logic in LocationBarBubbleDelegateView.
E.g.
> > > LocationBarBubbleDelegateView initializer becomes 
> > > : BubbleDialogDelegateView(anchor_view, ArrowForAnchorView(anchor_view))
> > > 
> > > With (in an anonymous namespace)
> > > 
> > > views::BubbleBorder::Arrow ArrowForAnchorView(views::View* anchor_view) {
> > > #if defined(OS_MACOSX)
> > >   // On Mac there is no toolbar-less fullscreen: it will slide in when a
> > bubble
> > > is shown. However,
> > >   // |anchor_view| is null when the parent window is a Cocoa browser.
> > >   return views::BubbleBorder::TOP_RIGHT;
> > > #else
> > >   // On other platforms, assume a null anchor_view means fullscreen, and
> > > position under the
> > >   // supplied anchor rectangle.
> > >   return anchor_view ? views::BubbleBorder::TOP_RIGHT :
> > > views::BubbleBorder::NONE;
> > > #endif
> > > }
> > > 
> > > 
> > > 2) might be the "safer" approach, but (1) is a lot neater if nothing truly
> > > depends on this logic. Looks like it was added in
> > > https://codereview.chromium.org/795053003 (or, in fact, before that for
the
> > zoom
> > > bubble) and then forgotten about. Certainly in ChromeOS, ZoomBubble and
> > > Bookmarks Bubble anchor differently in fullscreen, but don't in regular
> > windows,
> > > so this either regressed or never worked properly. So if we can do (1)
> without
> > > changing the behaviour of the zoom bubble then that's probably the right
> fix.
> > Why do you think that making a fix in LocationBarBubbleDelegateView is the
> > "right" way? Why is it ok for LocationBarBubbleDelegateView to make
> assumptions
> > that bubble is shown on the right side of the window (consider Omnibox Info
> > Bubble which is shown on the left side)? bookmark_bubble_view.cc knows that
> this
> > bubble should be anchored to top-right corner and can be responsible for
> > positioning decision.
> > 
> > On mac adding bookmarks for fullscreen looks buggy with and without this
> change.
> > The dialog is just shown on top and covers the omnibox. But I would say that
> is
> > a separate bug (bookmarks bubble should be shown only after toolbar
animation
> is
> > finished)
> > 
> > I guess I should fix the way to test this on Windows and Chrome OS.
> > 
> > 
> > 
> This change does not break fullscreen on Windows.

OK - I've convinced myself that this is an OK fix, but we may still want a more
generic solution. The rationale is this:

In r380411 this[1] change to bookmark_bubble_view.cc changed an *explicit*
TOP_RIGHT arrow into one that is now inferred by that ternary if-else in
location_bar_bubble_delegate_view.cc. So this line is effectively restoring that
explicit TOP_RIGHT, which got removed in r380411.

And although r380411 converted a few things, bookmark_bubble_view.cc is the only
one that lost its explicit TOP_RIGHT. So other bubbles don't need a similar
update.

Finally, this affects MacViews, but not other platforms, because other platforms
infer a TOP_RIGHT in location_bar_bubble_delegate_view.cc. MacViews does not,
because it's anchored to a point, not a View.

[[this all should be mentioned in the CL description]]


There are still two open questions, but they can be deferred:

 - The arrow inference logic in location_bar_bubble_delegate_view.cc seems to
exist to support fullscreen (on other platforms), but it seems to be broken for
everything except [maybe] the zoom bubble.

 - Is this fix "robust" - i.e. are other bubbles already affected, or is there
the chance that someone adding a new bubble won't test on Mac and will
accidentally get a broken bubble. Having LocationBarBubbleDelegateView handle
fullscreen decisions in a clearer way may be a better approach.

I think that you're correct that bookmark_bubble_view.cc knows how it wants to
be anchored - and "always" anchoring TOP_RIGHT correctly preserves existing
behaviour. However, in fullscreen that existing behaviour might be incorrect
(not just on Mac, but other platforms too).


[1]
https://codereview.chromium.org/1759453002/diff/240001/chrome/browser/ui/view...

https://codereview.chromium.org/2351593002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:83:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
This still needs a comment - e.g. "BookmarkBubble should always try to anchor
TOP_RIGHT, but the LocationBarBubbleDelegateView constructor will infer NONE if
|anchor_view| is null."

[Eugene But (OOO till 7-30), 2016-09-20 01:56:28]

Description was changed from

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. This change sets arrow to top right.

This change does not impact windows but makes macviews version
worse in RTL, because omnibox is not mirrored on mac. Since omnibox
will be mirrored eventually, leaving it as it is.

BUG=600209
==========

to

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209
==========

[Eugene But (OOO till 7-30), 2016-09-20 01:57:45]

Thanks! Updated CL description.

https://codereview.chromium.org/2351593002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:83:
bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);
On 2016/09/20 01:37:13, tapted wrote:
> This still needs a comment - e.g. "BookmarkBubble should always try to anchor
> TOP_RIGHT, but the LocationBarBubbleDelegateView constructor will infer NONE
if
> |anchor_view| is null."
Added a comment. Made it more generic w/o describing implementation details of
LocationBarBubbleDelegateView which may change causing this comment to rot.

[Eugene But (OOO till 7-30), 2016-09-20 01:57:57]

eugenebut@chromium.org changed reviewers:
+ sky@chromium.org

[Eugene But (OOO till 7-30), 2016-09-20 01:57:58]

+sky for owners.

[sky, 2016-09-20 02:50:21]

https://codereview.chromium.org/2351593002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:83: // Bookmark bubble
should always anchor TOP_RIGHT, but the
If the bookmark bubble should always anchor like you say, should this code move
to the BookmarkBubbleView constructor?

[Eugene But (OOO till 7-30), 2016-09-20 03:11:23]

https://codereview.chromium.org/2351593002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2351593002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:83: // Bookmark bubble
should always anchor TOP_RIGHT, but the
On 2016/09/20 02:50:21, sky wrote:
> If the bookmark bubble should always anchor like you say, should this code
move
> to the BookmarkBubbleView constructor?
That would feel inconsistent with the code below (inside !anchor_view block). Do
you think I should move it to constructor anyways?

[sky, 2016-09-20 13:13:08]

Fair enough. LGTM

[Eugene But (OOO till 7-30), 2016-09-20 21:52:23]

Thanks!

[Eugene But (OOO till 7-30), 2016-09-20 21:52:34]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-09-20 21:52:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2351593002/#ps80001
(title: "Addressed review comment")

[commit-bot: I haz the power, 2016-09-20 21:53:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 22:37:28]

Description was changed from

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209
==========

to

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209
==========

[commit-bot: I haz the power, 2016-09-20 22:37:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-20 22:39:32]

Description was changed from

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209
==========

to

==========
[MacViews] Anchor Bookmarks bubble to top-right corner.

Currently Bookmarks bubble is anchored to center-top, so its right part
may be displayed offscreen. Regression happened after this CL:
https://codereview.chromium.org/1759453002/patch/240001/250007
which defer anchoring to LocationBarBubbleDelegateView (which in it's
turn uses NONE arrow, and makes center-top anchoring).

bookmark_bubble_view.cc was the only bubble that lost its explicit TOP_RIGHT.
So other bubbles don't need a similar update. Regression affects MacViews,
but not other platforms, because other platforms infer a TOP_RIGHT in
location_bar_bubble_delegate_view.cc. MacViews does not, because it's
anchored to a point, not a View.

This change restores anchoring to top right and does not impact other platforms
(tested on Windows). The change makes MacViews version worse in RTL, because
omnibox is not mirrored on mac. Since omnibox will be mirrored eventually,
leaving it as it is.

BUG=600209

Committed: https://crrev.com/a9f2053a36b4899fb312bec01a3362f65eafe785
Cr-Commit-Position: refs/heads/master@{#419877}
==========

[commit-bot: I haz the power, 2016-09-20 22:39:33]

Patchset 5 (id:??) landed as
https://crrev.com/a9f2053a36b4899fb312bec01a3362f65eafe785
Cr-Commit-Position: refs/heads/master@{#419877}