MacViews: Implement Tooltips

MacViews: Implement Tooltips

After some experimentation, I couldn't improve on the techniques
currently used in render_widget_host_view_mac.mm to provide tooltips.
They also work well in MacViews, so encapsulate them in a shared base
class, `ToolTipBaseView' and put it in ui/base/cocoa.

As a result, BridgedNativeWidget is now also a `BaseView` instead of
just an NSView. This actually allows some boilerplate to go since some
useful stuff gets inherited from BaseView.

Note: Cocoa capitalizes as "ToolTip", and toolkit-views as "Tooltip". In
this CL, aim for "Tooltip" unless the code is used completely outside of
toolkit-views.

As a bonus, MenuButtonTest.MenuButtonPressedLock now passes. This is
because functionality was added to the Mac EventGenerator to simulate
cursor position, which ScreenMac::GetCursorScreenPoint() is able to take
advantage of in tests.

BUG=441543, 378134
TEST=views_unittests (now just 6 failures, 1 crash).
NOPRESUBMIT=true

Committed: https://crrev.com/f3dbf91f48e6e97abe51d562c806c8c847ebb2fb
Cr-Commit-Position: refs/heads/master@{#330483}

[tapted, 2015-05-13 01:13:10]

tapted@chromium.org changed reviewers:
+ asvitkine@chromium.org

[tapted, 2015-05-13 01:14:09]

tapted@chromium.org changed reviewers:
+ andresantoso@chromium.org

[tapted, 2015-05-13 01:14:42]

tapted@chromium.org changed reviewers:
+ avi@chromium.org
- asvitkine@chromium.org

[tapted, 2015-05-13 01:51:33]

andre: please take a look for general review in ui/views/cocoa/* and
*native_widget_mac*

avi: please take a look for review/OWNERS in content/browser, ui/base, and
ui/views/{test,views.gyp}

note I've copied the stuff in tool_tip_base_view.mm verbatim, so there's a
presubmit warning:
    ui/base/cocoa/tool_tip_base_view.mm:59:
      The use of -[NSView addTrackingRect:owner:userData:assumeInside:]
isprohibited. Please use CrTrackingArea instead.

So, I think I'll need to submit with NOPRESUBMIT=true [after adding a
android_webview OWNER] -- I'll see what the CQ thinks.

Thanks!

[Avi (use Gerrit), 2015-05-13 02:57:01]

If this is just a move (which it looks like it is), LGTM.

[tapted, 2015-05-13 03:09:39]

On 2015/05/13 02:57:01, Avi wrote:
> If this is just a move (which it looks like it is), LGTM.

pretty much, although there's a new class layer, and I added some comments to
tool_tip_base_view.h

I'm technically relying on you for OWNERS in views.gyp and
event_generator_delegate_mac.mm (although I wrote all of that file so I really
should put up an OWNERS CL, since it's come up in
http://crrev.com/1105613002/#msg10 as well)

(also realised the @class ToolTip forward declare I moved doesn't actually exist
- see below)

https://codereview.chromium.org/1137653005/diff/80001/ui/base/cocoa/tool_tip_...
File ui/base/cocoa/tool_tip_base_view.h (right):

https://codereview.chromium.org/1137653005/diff/80001/ui/base/cocoa/tool_tip_...
ui/base/cocoa/tool_tip_base_view.h:12: @class ToolTip;
this came from render_widget_host_view_mac.h, but I just realised it's not
actually a thing. (removed)

[Andre, 2015-05-13 19:33:13]

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:12: #import
"ui/base/cocoa/tool_tip_base_view.h"
Sorts before tracking_area.h.

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:59: -
(void)updateTooltipIfRequiredAt:(const gfx::Point&)locationInWindow;
If I understand it correctly, the point is in this view's coordinates, not the
window's.
If so, locationInWindow is not a good name.

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
File ui/views/cocoa/tooltip_manager_mac.h (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.h:17: TooltipManagerMac(BridgedNativeWidget*
widget);
explicit

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
File ui/views/cocoa/tooltip_manager_mac.mm (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.mm:13: const int kTooltipMaxWidthPixels =
250;
Should be inside unnamed namespace.

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.mm:39: NSPoint nspoint = [window
convertScreenToBase:[NSEvent mouseLocation]];
Why not [window mouseLocationOutsideOfEventStream]?

[tapted, 2015-05-15 07:54:21]

Patchset #7 (id:120001) has been deleted

[tapted, 2015-05-15 07:56:48]

Patchset #8 (id:160001) has been deleted

[tapted, 2015-05-15 07:59:55]

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:12: #import
"ui/base/cocoa/tool_tip_base_view.h"
On 2015/05/13 19:33:13, Andre wrote:
> Sorts before tracking_area.h.

Done. I don't know why we don't just put .mm files in the PRESUBMIT -- since
Obj-C .h files are already checked, so the scripts are already robust. Let's try
landing a fix for this in http://crrev.com/1133713009 :) (I'll see how many
conflicts there are after branch point as a guide to whether I should split up
the CL)

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:59: -
(void)updateTooltipIfRequiredAt:(const gfx::Point&)locationInWindow;
On 2015/05/13 19:33:13, Andre wrote:
> If I understand it correctly, the point is in this view's coordinates, not the
> window's.
> If so, locationInWindow is not a good name.

Very true - ui::LocatedEvent is a bit vague about what its location actually is
(and that's what I wanted this to be). The content rect of the window is what
works for toolkit-view, so I'll go with locationInContent, and note this in the
comment. (Done)

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
File ui/views/cocoa/tooltip_manager_mac.h (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.h:17: TooltipManagerMac(BridgedNativeWidget*
widget);
On 2015/05/13 19:33:13, Andre wrote:
> explicit

Done (there's already a presubmit CL for this one too...
http://crrev.com/973993002 ).

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
File ui/views/cocoa/tooltip_manager_mac.mm (right):

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.mm:13: const int kTooltipMaxWidthPixels =
250;
On 2015/05/13 19:33:13, Andre wrote:
> Should be inside unnamed namespace.

Done (`const` also imposes internal linkage, but I've always struggled to form
an opinion on this - more often an anonymous namespace comes later anyway when a
function is needed).

https://codereview.chromium.org/1137653005/diff/100001/ui/views/cocoa/tooltip...
ui/views/cocoa/tooltip_manager_mac.mm:39: NSPoint nspoint = [window
convertScreenToBase:[NSEvent mouseLocation]];
On 2015/05/13 19:33:13, Andre wrote:
> Why not [window mouseLocationOutsideOfEventStream]?

Hm, it wasn't a conscious decision. I tried it out. Unfortunately
mouseLocationOutsideOfEventStream doesn't get the mouse coordinate from [NSEvent
mouseLocation] so it's a bit harder to test for (swizzling instance methods is
more fiddly). But it still uses convertScreenToBase:. The trace is:

- NativeWidgetMacNSWindow NSWindow mouseLocationOutsideOfEventStream
- NativeWidgetMacNSWindow NSWindow windowNumber
- NSApplication NSApplication contextID
- NativeWidgetMacNSWindow NSWindow convertScreenToBase:

For [NSEvent mouseLocation], it is

+ NSEvent NSEvent mouseLocation
+ NSScreen NSScreen _zeroScreenHeight
(then our own) - NativeWidgetMacNSWindow NSWindow convertScreenToBase:


I could go either way, but I think I'm slightly in favour of mouseLocation since
it's a cross-platform concept (i.e. gfx::Screen::GetCursorScreenPoint(), whereas
there's no Widget::LastMouseLocation), and it's slightly easier to test for.

mouseLocationOutsideOfEventStream could work here with more changes in
event_generator_delegate_mac.mm

[Andre, 2015-05-15 16:42:50]

LGTM

[tapted, 2015-05-18 23:03:49]

tapted@chromium.org changed reviewers:
+ sgurun@chromium.org

[tapted, 2015-05-18 23:03:50]

+sgurun for OWNERS in android_webview/tools/third_party_files_whitelist.txt
(just a move). Thanks!

[sgurun-gerrit only, 2015-05-19 03:00:40]

On 2015/05/18 23:03:50, tapted wrote:
> +sgurun for OWNERS in android_webview/tools/third_party_files_whitelist.txt
> (just a move). Thanks!

lgtm

[tapted, 2015-05-19 03:34:37]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-05-19 03:34:38]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1137653005/#ps180001
(title: "respond to comments")

[commit-bot: I haz the power, 2015-05-19 03:34:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137653005/180001

[commit-bot: I haz the power, 2015-05-19 03:42:02]

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

[commit-bot: I haz the power, 2015-05-19 03:42:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2015-05-19 03:46:16]

On 2015/05/19 03:42:04, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Yep that's just the stuff I foreshadowed in the initial publish message (The use
of -[NSView addTrackingRect..] in tool_tip_base_view.mm). Keeping it for now
since it's just a move, but it might be something to address in a follow-up.

(adding NOPRESUBMIT=true -- no other warnings/errors show up). Full text FYI:

DEBUG:root:Running CheckChangeOnCommit done.

** Presubmit Messages **
/b/build/slave/linux/build/src/android_webview/tools/copyright_scanner_unittest.py
(0.18s)

** Presubmit Warnings **
Banned functions were used.
    ui/base/cocoa/tool_tip_base_view.mm:59:
      The use of -[NSView addTrackingRect:owner:userData:assumeInside:]
isprohibited. Please use CrTrackingArea instead.
      http://dev.chromium.org/developers/coding-style/cocoa-dos-and-donts
    ui/base/cocoa/tool_tip_base_view.mm:70:
      The use of -[NSView addTrackingRect:owner:userData:assumeInside:]
isprohibited. Please use CrTrackingArea instead.
      http://dev.chromium.org/developers/coding-style/cocoa-dos-and-donts

Presubmit checks took 2.6s to calculate.


@@@STEP_CURSOR presubmit@@@

[tapted, 2015-05-19 03:47:13]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-19 03:47:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137653005/180001

[commit-bot: I haz the power, 2015-05-19 04:53:53]

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

[commit-bot: I haz the power, 2015-05-19 04:53:55]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2015-05-19 05:37:06]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-19 05:37:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137653005/180001

[commit-bot: I haz the power, 2015-05-19 06:14:02]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-05-19 06:15:09]

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