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

chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
index e3e5e0aabaf991eaf66d8ec428addf06c11f37f2..2cb847721527d8ef9115400c3fac48b80da22fe0 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
@@ -80,6 +80,10 @@ views::Widget* BookmarkBubbleView::ShowBubble(
   bookmark_bubble_ =
       new BookmarkBubbleView(anchor_view, observer, std::move(delegate),
                              profile, url, !already_bookmarked);
+
+  bookmark_bubble_->set_arrow(views::BubbleBorder::TOP_RIGHT);

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

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.

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

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.

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

This change does not break fullscreen on Windows.

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

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...

+  // TODO(crbug.com/642732): Mirror arrow once toolbar supports RTL.
+  bookmark_bubble_->set_mirror_arrow_in_rtl(false);

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

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]

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:02:51]

Ok, this this change indeed breaks Windows in RTL, so removing this line. 

   if (!anchor_view) {
     bookmark_bubble_->SetAnchorRect(anchor_rect);
     bookmark_bubble_->set_parent_window(parent_window);