Mac: Simulate the OS-level focus handling that windows and linux plugins...

Mac: Simulate the OS-level focus handling that windows and linux plugins
rely on to trigger NPAPI keyboard focus notifications.
BUG=26585
TEST=On pages with multiple Flash text entry fields, only one should have
a blinking caret at any time.  Flash elements that use the ActionScript2
"Key.IsDown()" function should only detect keydowns when they are in a
visible tab and focused.

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

[stuartmorgan, 2009-11-18 22:53:57]

http://codereview.chromium.org/399090/diff/1/5
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/1/5#newcode84
Line 84: // that they no longer do (simulating the OS-level focus notifications
that
s/do/have it/

http://codereview.chromium.org/399090/diff/1/6
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/1/6#newcode54
Line 54: public:
Indent 1

http://codereview.chromium.org/399090/diff/1/6#newcode56
Line 56: : process_id_(process_id), instance_id_(instance_id) { }
Indent 4 more.

http://codereview.chromium.org/399090/diff/1/6#newcode58
Line 58: private:
Indent 1

http://codereview.chromium.org/399090/diff/1/2
File webkit/glue/plugins/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/399090/diff/1/2#newcode417
Line 417: void WebPluginDelegateImpl::FocusNotification(uint32 process_id,
Rather than sending the PID to everyone, the sender can check the PID (using
handle() on the PluginProcessHost), then send 0 as the instance_id to every
non-matching PID.

http://codereview.chromium.org/399090/diff/1/2#newcode423
Line 423: bool have_focus = false;
Why the temp variable, instead of using have_focus_ directly?

[rohitrao (ping after 24h), 2009-11-18 23:01:10]

A reminder that we need to handle get/resign key status, most likely in
browser_window_controller.mm.

[Amanda Walker, 2009-11-19 21:17:14]

http://codereview.chromium.org/399090/diff/1/5
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/1/5#newcode84
Line 84: // that they no longer do (simulating the OS-level focus notifications
that
Fixed.

http://codereview.chromium.org/399090/diff/1/6
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/1/6#newcode54
Line 54: public:
Fixed.

http://codereview.chromium.org/399090/diff/1/6#newcode56
Line 56: : process_id_(process_id), instance_id_(instance_id) { }
Fixed.

http://codereview.chromium.org/399090/diff/1/6#newcode58
Line 58: private:
Fixed.

http://codereview.chromium.org/399090/diff/1/2
File webkit/glue/plugins/webplugin_delegate_impl_mac.mm (right):

http://codereview.chromium.org/399090/diff/1/2#newcode417
Line 417: void WebPluginDelegateImpl::FocusNotification(uint32 process_id,
Fixed.

http://codereview.chromium.org/399090/diff/1/2#newcode423
Line 423: bool have_focus = false;
Fixed.

[Amanda Walker, 2009-11-19 23:35:34]

On 2009/11/18 23:01:10, rohitrao wrote:
> A reminder that we need to handle get/resign key status, most likely in
> browser_window_controller.mm.

Done (in SetActive in render_widget_host_view_mac.mm)

[rohitrao (ping after 24h), 2009-11-19 23:40:15]

RWHV changes LGTM

[stuartmorgan, 2009-11-19 23:53:07]

http://codereview.chromium.org/399090/diff/5001/5005
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/5001/5005#newcode90
Line 90: new PluginProcessMsg_PluginReceivedFocus(process_id, instance_id));
Per discussion, calling handle() on |plugin| here lets you decide whether that
plugin is the one, so you could avoid sending out the process_id in
PluginProcessMsg_PluginReceivedFocus all together.

http://codereview.chromium.org/399090/diff/5001/5006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/5001/5006#newcode64
Line 64: new PluginProcessMsg_PluginReceivedFocus(process_id_, instance_id_));
Same here.

http://codereview.chromium.org/399090/diff/5001/5008
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/399090/diff/5001/5008#newcode55
Line 55: IPC_MESSAGE_CONTROL2(PluginProcessMsg_PluginReceivedFocus,
Also per discussion, it would reduce confusion if this had a different name
(especially once the pid is gone from the arguments).

http://codereview.chromium.org/399090/diff/5001/5003
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/399090/diff/5001/5003#newcode126
Line 126: // call FocusNotify(true) on itself.  This is used in a multiprocess
Since FocusNotify takes a pointer, I'm guessing "true" is from an earlier
revision?

[Amanda Walker, 2009-11-20 00:02:15]

http://codereview.chromium.org/399090/diff/5001/5005
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/5001/5005#newcode90
Line 90: new PluginProcessMsg_PluginReceivedFocus(process_id, instance_id));
Fixed.

http://codereview.chromium.org/399090/diff/5001/5006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/5001/5006#newcode64
Line 64: new PluginProcessMsg_PluginReceivedFocus(process_id_, instance_id_));
Fixed.

http://codereview.chromium.org/399090/diff/5001/5008
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/399090/diff/5001/5008#newcode55
Line 55: IPC_MESSAGE_CONTROL2(PluginProcessMsg_PluginReceivedFocus,
Fixed.

http://codereview.chromium.org/399090/diff/5001/5003
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/399090/diff/5001/5003#newcode126
Line 126: // call FocusNotify(true) on itself.  This is used in a multiprocess
Why, yes it is.  Fixed.

[stuartmorgan, 2009-11-20 00:13:33]

LGTM

http://codereview.chromium.org/399090/diff/1020/4008
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/1020/4008#newcode91
Line 91: new PluginProcessMsg_PluginFocusNotify(instance_id));
Maybe use a local variable or a ?: to avoid the code duplication?

http://codereview.chromium.org/399090/diff/1020/4009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/1020/4009#newcode67
Line 67: plugin->Send(new PluginProcessMsg_PluginFocusNotify(0));
Same

[Amanda Walker, 2009-11-20 00:37:27]

http://codereview.chromium.org/399090/diff/1020/4008
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/399090/diff/1020/4008#newcode91
Line 91: new PluginProcessMsg_PluginFocusNotify(instance_id));
Done.

http://codereview.chromium.org/399090/diff/1020/4009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/399090/diff/1020/4009#newcode67
Line 67: plugin->Send(new PluginProcessMsg_PluginFocusNotify(0));
Done.

[stuartmorgan, 2009-11-20 00:40:29]

LGTM