[Mac] Show correct cursor after context menu is closed.

[Mac] Show correct cursor after context menu is closed.

The fix has two parts to it.

First keep the RenderViewContextMenu in a scoped_ptr on the TabContentsViewMac instead of creating it on the stack in |ShowContextMenu()|. (This is how it's done with views and gtk already.) This ensures |MenuClosed()| gets called on the RenderViewContextMenu properly - previously it would get destroyed before that could happen.

Second, send mouse leave and mouse enter events to the renderer when the context menu is opened and closed, so that the renderer will update the cursor.

The CL also renames |is_showing_popup_menu_| to |is_showing_context_menu_| in the views code to be consistent with the name of the callback and the equivalent variable in the GTK and now Mac code.

BUG=64436
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92370

[brettw, 2011-07-06 17:04:31]

http://codereview.chromium.org/7227007/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7227007/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:4266: Send(new
ViewHostMsg_SetCursor(routing_id_, current_cursor_));
How do you know the old cursor is the right one to use for the current position
of the cursor when the menu is closed? It seems like we should have Mac
duplicate what Windows does by sending a mouse move message to the renderer so
it hit tests the page properly.

[Alexei Svitkine (slow), 2011-07-06 17:56:47]

> How do you know the old cursor is the right one to use for the current
position
> of the cursor when the menu is closed? It seems like we should have Mac
> duplicate what Windows does by sending a mouse move message to the renderer so
> it hit tests the page properly.

I did some testing of this with this page that has a bunch of different
mouseover cursor examples:

http://www.echoecho.com/csscursors.htm

and with this change, I get the correct result - always the cursor underneath
the mouse when I dismiss the menu (e.g. even if I dismiss it by clicking
somewhere else on the page than where I started).

Doing it this way also saves an extra round-trip and avoids slightly more-messy
code in creating a fake mouse move event. I can change it to send a fake mouse
move event, though, if you prefer.

[brettw, 2011-07-06 18:03:47]

We should understand why your way works if it's indeed the right thing.

[Alexei Svitkine (slow), 2011-07-06 21:10:48]

On 2011/07/06 18:03:47, brettw wrote:
> We should understand why your way works if it's indeed the right thing.

My bad, turns out I was wrong - it didn't work correctly (the cursor was out of
date).

I've updated the CL to send fake mouse exit and mouse enter events from the
renderer view host, which works fine (after I also made it ignore cursor events
when the context menu is up).

I also changed a variable name in the equivalent views code from
|is_showing_popup_menu_| to |is_showing_context_menu_| to have a consistent name
between the GTK and now Mac code.

[brettw, 2011-07-07 17:04:37]

This looks fine to me from a high level, but Avi should check the mac view
changes.

[Alexei Svitkine (slow), 2011-07-12 13:34:37]

ping Avi

On Thu, Jul 7, 2011 at 1:04 PM,  <brettw@chromium.org> wrote:
> This looks fine to me from a high level, but Avi should check the mac view
> changes.
>
> http://codereview.chromium.org/7227007/
>

[Alexei Svitkine (slow), 2011-07-12 13:37:34]

Hey Mark,

Could you review the Mac-side of this, since Avi is on vacation?

[Mark Mentovai, 2011-07-12 14:06:57]

LGTM. I’m concerned about the event stream thing, but otherwise this is fine.

http://codereview.chromium.org/7227007/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7227007/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:658: NSPoint
location = [window mouseLocationOutsideOfEventStream];
“outside of event stream”—is this a good idea? What if the current event
([NSEvent currentEvent]) lags the “physical” world because processing was
delayed?

There’s another TODO about this in this file, and we use -[NSWindow
mouseLocationOutsideOfEventStream] elsewhere in Chrome. It may not be the best
choice in those locations either.

[Alexei Svitkine (slow), 2011-07-12 14:26:09]

> “outside of event stream”—is this a good idea? What if the current event
> ([NSEvent currentEvent]) lags the “physical” world because processing
> was delayed?

I think if location outside of the event stream doesn't correspond to
the current event, then there should be mouse move events in the pipe
that will update the cursor appropriately when they'll be processed.
So the end cursor will be correct but there may be a brief flicker of
the cursor if that scenario actually happens on a slow enough machine.

Do you want me to add a TODO comment about it?

[commit-bot: I haz the power, 2011-07-13 00:39:04]

Can't apply patch for file chrome/browser/tab_contents/tab_contents_view_mac.h.
While running patch -p0 --forward --force;
patching file chrome/browser/tab_contents/tab_contents_view_mac.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 20 (offset 1 line).
Hunk #3 succeeded at 138 (offset 12 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/tab_contents/tab_contents_view_mac.h.rej

[commit-bot: I haz the power, 2011-07-13 15:50:46]

Change committed as 92370