refactoring mouse lock to support pepper and WebKit

Mouse Lock is currently supported in Pepper, but not yet supported from WebKit.

Move the render thread logic for managing the mouse lock state out of the pepper_plugin_delegate_impl, and into a higher level dispatcher for render_view_impl.

Handle mouse lock / pointer lock requests from both pepper and webkit (WebKit API not yet landed, small TODOs left in this code to enable once that lands).

BUG=109957
TEST=Pepper examples/mouse_lock and NaCl mouse lock examples still work.

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

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

[yzshen1, 2012-01-04 00:50:00]

I apologize for late reply.

The change mostly looks good.

http://codereview.chromium.org/8970016/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:4847: if (pending_unlock_request_)
nit: It seems better to put this in the if-block of line 4854.
(I agree that the current logic is correct, but the other way is easier to
understand.)

http://codereview.chromium.org/8970016/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8970016/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.h:1238: // TEMP -- REFACTOR TO A DISPATCHER
We probably cannot check in the code without a nice dispatcher definition, even
if it is temporarily. :)

http://codereview.chromium.org/8970016/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.h:1261: //???????? need to deal with it being
deleted?
As far as PPAPI is concerned, we will get a notification when the instance gets
deleted. We will clean up the owner pointer if necessary.

You need to do similar things for WebKit::WebWidget.

http://codereview.chromium.org/8970016/diff/1/webkit/plugins/ppapi/plugin_del...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8970016/diff/1/webkit/plugins/ppapi/plugin_del...
webkit/plugins/ppapi/plugin_delegate.h:495: // be called when LockMouse() is
still on the stack).
Please remember to change the comment about when/whether OnLockMouseACK() will
be called.

http://codereview.chromium.org/8970016/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1901: if
(delegate()->LockMouse(this)) {
It seems better to *not* have |mouse_locked_to_this_instance_|: we have the same
information stored in render_view. it is error-prone to keep track of the same
state in two places.

We could consider:
- let LockMouse return an enum value (pending/your are already the
owner!/failed)
- have a delegate()->IsCurrentMouseLockOwner(instance).

(Sorry, I know we discussed about this issue before. But reading the code makes
me think that it is not a good solution.)

[scheib, 2012-01-12 19:36:47]

Thanks for previous review, I've addressed concerns and this should be in a
state for final review. (however, landing this is predicated on landing WebKit:
https://bugs.webkit.org/show_bug.cgi?id=75762)

[yzshen1, 2012-01-12 19:49:25]

The first part of comments.

Thanks!

http://codereview.chromium.org/8970016/diff/13001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/13001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:435: mouse_lock_dispatcher_ = new
MouseLockDispatcher(this);
- This is not 'lazily initialized' as the comment in the .h file says, right?
- Currently the object is leaked. You probably want to use scoped_ptr.
- It seems you haven't included the dispatcher implementation in the CL.

[scheib, 2012-01-12 21:25:19]

Added the missing files... sorry.

http://codereview.chromium.org/8970016/diff/13001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/13001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:435: mouse_lock_dispatcher_ = new
MouseLockDispatcher(this);
On 2012/01/12 19:49:25, yzshen1 wrote:
> - This is not 'lazily initialized' as the comment in the .h file says, right?
> - Currently the object is leaked. You probably want to use scoped_ptr.
> - It seems you haven't included the dispatcher implementation in the CL.

Fixed comment; cut paste error.

The object is a RenderViewObserver, which will self delete when the RenderView
is destroyed.

Added missing files.

[yzshen1, 2012-01-16 07:50:34]

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012. :)

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:10: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
Alphabetically, please.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:26: webkit::ppapi::PluginInstance*
pinstance) {
Wrong indent.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
pinstance) {
Wrong indent.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:67: if (mouse_lock_pinstance_owner_)
you don't need this check, right?

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:77: }
Empty line after it, please.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:110: WebKit::WebWidget*
last_webwidget_owner = mouse_lock_webwidget_owner_;
[Just want to double check.]
Is it possible that mouse_lock_webwidget_owner_ dies before MouseLockDispatcher?
If yes, have you notified this object?

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:144: // Reset mouse lock owner to NULL
before calling OnLockMouseACK(), so
OnLockMouseACK -> OnMouseLockLost. (And the same for the next line.)

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012. (Yes, apocalypse is very close now. :))

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:5: #ifndef MOUSE_LOCK_DISPATCHER_H_
You should list the full path.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:12: class WebMouseEvent;
It is in WebKit namespace.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:24: // MouseLockDispatcherDispatcher is
owned by RenderViewImpl.
One redundant Dispatcher.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:30: bool LockMouse(WebKit::WebWidget*
webwidget,
It is better to have the same interface for webwidget/pinstance to either
inherit or compose.
So we don't need to handle the two explicitly.

Is there any difficulty to do that?

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:46: 
One empty line is enough.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:70:
DISALLOW_COPY_AND_ASSIGN(MouseLockDispatcher);
You should include .h for DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/render_vie...
content/renderer/render_view_impl.h:291: bool
PpapiLockMouse(webkit::ppapi::PluginInstance* pinstance);
nit: pinstance -> instance.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:507: // Returns true iif |instance|
currently owns the mouse lock.
iif -> if, please.
(or you mean iff?)

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1666: void
PluginInstance::OnLockMouseACK(bool succeeded) {
Please use the same parameter name as in the .h file.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1972: if
(lock_mouse_callback_.func) // a lock is pending
- Please use a complete sentence. (With capital letter at the beginning and
trailing dot.)
- 2 spaces before '//'.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:306: virtual void
OnLockMouseACK(bool success);
Why the three methods need to be virtual?

the success -> the result.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:307: // A mouse lock was in place,
but has been lost
Please add a period, please.

[scheib, 2012-01-17 22:05:36]

Thanks for review. Items addressed; 2 responses to questions:

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/01/16 07:50:35, yzshen1 wrote:
> 2012. :)

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:10: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
On 2012/01/16 07:50:35, yzshen1 wrote:
> Alphabetically, please.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:26: webkit::ppapi::PluginInstance*
pinstance) {
On 2012/01/16 07:50:35, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
pinstance) {
On 2012/01/16 07:50:35, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:67: if (mouse_lock_pinstance_owner_)
On 2012/01/16 07:50:35, yzshen1 wrote:
> you don't need this check, right?

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:77: }
On 2012/01/16 07:50:35, yzshen1 wrote:
> Empty line after it, please.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:110: WebKit::WebWidget*
last_webwidget_owner = mouse_lock_webwidget_owner_;
On 2012/01/16 07:50:35, yzshen1 wrote:
> [Just want to double check.]
> Is it possible that mouse_lock_webwidget_owner_ dies before
MouseLockDispatcher?
> If yes, have you notified this object?

It is not possible, the render view impl will call close() on the widget, which
will unlock. I discussed this with Darin for increased confidence.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:144: // Reset mouse lock owner to NULL
before calling OnLockMouseACK(), so
On 2012/01/16 07:50:35, yzshen1 wrote:
> OnLockMouseACK -> OnMouseLockLost. (And the same for the next line.)

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/01/16 07:50:35, yzshen1 wrote:
> 2012. (Yes, apocalypse is very close now. :))

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:5: #ifndef MOUSE_LOCK_DISPATCHER_H_
On 2012/01/16 07:50:35, yzshen1 wrote:
> You should list the full path.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:12: class WebMouseEvent;
On 2012/01/16 07:50:35, yzshen1 wrote:
> It is in WebKit namespace.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:24: // MouseLockDispatcherDispatcher is
owned by RenderViewImpl.
On 2012/01/16 07:50:35, yzshen1 wrote:
> One redundant Dispatcher.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:30: bool LockMouse(WebKit::WebWidget*
webwidget,
On 2012/01/16 07:50:35, yzshen1 wrote:
> It is better to have the same interface for webwidget/pinstance to either
> inherit or compose.
> So we don't need to handle the two explicitly.
> 
> Is there any difficulty to do that?


I started that way, and showed darin. He did not like the idea of defining that
interface in the WebKit repository and then also using it for pepper. Because
the interface is so minor, and there are only two users of it, we agreed
together that a 'raw access' special casing the two was preferable.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:46: 
On 2012/01/16 07:50:35, yzshen1 wrote:
> One empty line is enough.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:70:
DISALLOW_COPY_AND_ASSIGN(MouseLockDispatcher);
On 2012/01/16 07:50:35, yzshen1 wrote:
> You should include .h for DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/8970016/diff/15001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/render_vie...
content/renderer/render_view_impl.h:291: bool
PpapiLockMouse(webkit::ppapi::PluginInstance* pinstance);
On 2012/01/16 07:50:35, yzshen1 wrote:
> nit: pinstance -> instance.

Done. -- changed pinstance to plugin everywhere.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:507: // Returns true iif |instance|
currently owns the mouse lock.
On 2012/01/16 07:50:35, yzshen1 wrote:
> iif -> if, please.
> (or you mean iff?)

Done.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1666: void
PluginInstance::OnLockMouseACK(bool succeeded) {
On 2012/01/16 07:50:35, yzshen1 wrote:
> Please use the same parameter name as in the .h file.

Done.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1972: if
(lock_mouse_callback_.func) // a lock is pending
On 2012/01/16 07:50:35, yzshen1 wrote:
> - Please use a complete sentence. (With capital letter at the beginning and
> trailing dot.)
> - 2 spaces before '//'.

Done.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:306: virtual void
OnLockMouseACK(bool success);
On 2012/01/16 07:50:35, yzshen1 wrote:
> Why the three methods need to be virtual?
> 
> the success -> the result.

Done.

http://codereview.chromium.org/8970016/diff/15001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:307: // A mouse lock was in place,
but has been lost
On 2012/01/16 07:50:35, yzshen1 wrote:
> Please add a period, please.

Done.

[scheib, 2012-01-17 23:14:48]

Brett, could you OWNERS review?

I've removed the WebKit API dependency to allow this to land before that lands
and rolls, with a minimal patch to re-couple to it later.

[yzshen1, 2012-01-17 23:39:25]

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:30: bool LockMouse(WebKit::WebWidget*
webwidget,
On 2012/01/17 22:05:36, scheib wrote:
> On 2012/01/16 07:50:35, yzshen1 wrote:
> > It is better to have the same interface for webwidget/pinstance to either
> > inherit or compose.
> > So we don't need to handle the two explicitly.
> > 
> > Is there any difficulty to do that?
> 
> 
> I started that way, and showed darin. He did not like the idea of defining
that
> interface in the WebKit repository and then also using it for pepper. Because
> the interface is so minor, and there are only two users of it, we agreed
> together that a 'raw access' special casing the two was preferable.

Okay, if that has been carefully considered.

Please note that even if you don't define that interface in WebKit, you still
can use an adapter to wrap the webwidget, and expose an interface that is shared
by PluginInstance.
That seems better to me. But I will let you decide. :)

If you decide to leave it unchanged, please comment somewhere why you do it this
way. So that others can understand the decision easily.

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:26: webkit::ppapi::PluginInstance*
plugin) {
According to our style guide, it should align with WebKit::WebWidget* webwidget

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
plugin) {
Ditto.

http://codereview.chromium.org/8970016/diff/20009/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8970016/diff/20009/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:309: // A mouse lock request was
pending, and the success is being returned.
the success -> the result?

[scheib, 2012-01-18 17:54:58]

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:30: bool LockMouse(WebKit::WebWidget*
webwidget,
On 2012/01/17 23:39:25, yzshen1 wrote:
> On 2012/01/17 22:05:36, scheib wrote:
> > On 2012/01/16 07:50:35, yzshen1 wrote:
> > > It is better to have the same interface for webwidget/pinstance to either
> > > inherit or compose.
> > > So we don't need to handle the two explicitly.
> > > 
> > > Is there any difficulty to do that?
> > 
> > 
> > I started that way, and showed darin. He did not like the idea of defining
> that
> > interface in the WebKit repository and then also using it for pepper.
Because
> > the interface is so minor, and there are only two users of it, we agreed
> > together that a 'raw access' special casing the two was preferable.
> 
> Okay, if that has been carefully considered.
> 
> Please note that even if you don't define that interface in WebKit, you still
> can use an adapter to wrap the webwidget, and expose an interface that is
shared
> by PluginInstance.
> That seems better to me. But I will let you decide. :)
> 
> If you decide to leave it unchanged, please comment somewhere why you do it
this
> way. So that others can understand the decision easily.

We did consider an adapter as well, but that would require adding a pure
interface, an adapter, wrapping in the adapter, vs a few lines where we deal
with two pointers directly. We sided on the direct implementation, pros: clarity
of operation, still encapsulated in the dispatcher, not more lines of code and
files and indirection.

I've added comments describing the decision.

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:26: webkit::ppapi::PluginInstance*
plugin) {
On 2012/01/17 23:39:25, yzshen1 wrote:
> According to our style guide, it should align with WebKit::WebWidget*
webwidget

Done.

http://codereview.chromium.org/8970016/diff/20009/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
plugin) {
On 2012/01/17 23:39:25, yzshen1 wrote:
> Ditto.

Done.

http://codereview.chromium.org/8970016/diff/20009/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8970016/diff/20009/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:309: // A mouse lock request was
pending, and the success is being returned.
On 2012/01/17 23:39:25, yzshen1 wrote:
> the success -> the result?

Done.

[yzshen1, 2012-01-18 18:13:29]

lgtm

[scheib, 2012-01-18 18:18:28]

piman, can you do Owner review. (brettw not available)

[piman, 2012-01-18 19:00:59]

The ownership logic is a bit subtle, especially in the context of asynchronous
messages. Could we add a test for the mouse_lock_dispatcher that tackles at
least this in particular?

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:27: DCHECK(!(webwidget && plugin)); //
Only one mouse lock target at a time.
Is it valid to lock with no owner at all?

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
plugin) {
This gets called when the plugin instance gets deleted. From my reading of the
logic, we're still hanging on to the PluginInstance pointer so that we can send
a callback when the unlock actually happened. But at that point the pointer will
be stale.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:48: plugin ==
mouse_lock_plugin_owner_) {
This test should be stricter. E.g. if I do LockMouse(NULL, plugin_instance1);
UnlockMouse(NULL, plugin_instance2); then mouse_lock_webwidget_owner_==NULL, and
webwidget==NULL here, so it will succeed.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:96:
render_view_impl_->webwidget()->mouseCaptureLost();
could this callback re-enter and cause, say, the content to try to release the
lock? Are we doing the right thing in that case?

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:109: if (mouse_lock_webwidget_owner_
|| mouse_lock_plugin_owner_) {
(similar question as above) can this be false?

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:36: bool
IsMouseLockedTo(webkit::ppapi::PluginInstance* plugin);
On these 2 functions, they seem identical except for the type of owner.
Shouldn't they have the same name (tagged with a type for the overload)?
e.g. IsMouseLockedToWebWidget/IsMouseLockedToPluginInstance ?

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:74: WebKit::WebWidget*
mouse_lock_webwidget_owner_;
Looking at the code, it seems that the only possible webwidget  owner would be
render_view_impl_'s webwidget. It seems that we could simplify the logic a
little bit by always going through the RenderViewImpl to get to the webwidget
(and that may help with lifetime issues), making this a bool
"render_view_is_owner_" or something.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:443: mouse_lock_dispatcher_ = new
MouseLockDispatcher(this);
Where is this deleted? Should mouse_lock_dispatcher_ be a scoped_ptr?

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1997: return PP_ERROR_FAILED;
This function doesn't respect the "always asynchronous unless
PP_COMPLETIONCALLBACK_FLAG_OPTIONAL is specified" semantics on the completion
callback. Can we fix that?
We'd really want the same exact semantics as TrackedCallback, but associated
with the instance (instead of a resource).

[yzshen1, 2012-01-18 19:21:42]

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1997: return PP_ERROR_FAILED;
On 2012/01/18 19:00:59, piman wrote:
> This function doesn't respect the "always asynchronous unless
> PP_COMPLETIONCALLBACK_FLAG_OPTIONAL is specified" semantics on the completion
> callback. Can we fix that?
> We'd really want the same exact semantics as TrackedCallback, but associated
> with the instance (instead of a resource).

I think that semantics is guaranteed by the thunk layer. In
ppb_mouse_lock_thunk.cc, we have MayForceCallback():
http://code.google.com/codesearch#OAMlx_jo-ck/src/ppapi/thunk/ppb_mouse_lock_...

[piman, 2012-01-18 19:26:00]

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8970016/diff/37001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1997: return PP_ERROR_FAILED;
On 2012/01/18 19:21:42, yzshen1 wrote:
> On 2012/01/18 19:00:59, piman wrote:
> > This function doesn't respect the "always asynchronous unless
> > PP_COMPLETIONCALLBACK_FLAG_OPTIONAL is specified" semantics on the
completion
> > callback. Can we fix that?
> > We'd really want the same exact semantics as TrackedCallback, but associated
> > with the instance (instead of a resource).
> 
> I think that semantics is guaranteed by the thunk layer. In
> ppb_mouse_lock_thunk.cc, we have MayForceCallback():
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/ppapi/thunk/ppb_mouse_lock_...
> 

Ah, ok, that works too. Thanks!

[scheib, 2012-01-24 01:51:42]

I've introduced an interface LockTarget that eases testing and removed the dual
pointers from MouseLockDispatcher.

I've created unit tests around multiple targets and handling the asynchronous
responses.

Other issues also addressed.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:27: DCHECK(!(webwidget && plugin)); //
Only one mouse lock target at a time.
On 2012/01/18 19:00:59, piman wrote:
> Is it valid to lock with no owner at all?

Refactoring has removed this issue, there is only one target now.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:42: webkit::ppapi::PluginInstance*
plugin) {
On 2012/01/18 19:00:59, piman wrote:
> This gets called when the plugin instance gets deleted. From my reading of the
> logic, we're still hanging on to the PluginInstance pointer so that we can
send
> a callback when the unlock actually happened. But at that point the pointer
will
> be stale.

Addressed by creating a new method UnlockMouseAndClearPointer(). Also, verified
the old error by upgrading the mouse_lock sample and that the new code works.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:48: plugin ==
mouse_lock_plugin_owner_) {
On 2012/01/18 19:00:59, piman wrote:
> This test should be stricter. E.g. if I do LockMouse(NULL, plugin_instance1);
> UnlockMouse(NULL, plugin_instance2); then mouse_lock_webwidget_owner_==NULL,
and
> webwidget==NULL here, so it will succeed.

Done.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:96:
render_view_impl_->webwidget()->mouseCaptureLost();
On 2012/01/18 19:00:59, piman wrote:
> could this callback re-enter and cause, say, the content to try to release the
> lock? Are we doing the right thing in that case?

Moved to after all state changes to prevent reentrant issues.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:109: if (mouse_lock_webwidget_owner_
|| mouse_lock_plugin_owner_) {
On 2012/01/18 19:00:59, piman wrote:
> (similar question as above) can this be false?

Now with UnlockAndClearPointer it is possible for no owner to be on record when
a response comes in. However, I've removed the if() bracket because the code is
safe to false/null owners regardless.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:36: bool
IsMouseLockedTo(webkit::ppapi::PluginInstance* plugin);
On 2012/01/18 19:00:59, piman wrote:
> On these 2 functions, they seem identical except for the type of owner.
> Shouldn't they have the same name (tagged with a type for the overload)?
> e.g. IsMouseLockedToWebWidget/IsMouseLockedToPluginInstance ?

Refactoring has resulted in only one set of functions, with the two differing
interfaces (pepper, webwidget) encapsulated in LockTarget subclasses.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:74: WebKit::WebWidget*
mouse_lock_webwidget_owner_;
On 2012/01/18 19:00:59, piman wrote:
> Looking at the code, it seems that the only possible webwidget  owner would be
> render_view_impl_'s webwidget. It seems that we could simplify the logic a
> little bit by always going through the RenderViewImpl to get to the webwidget
> (and that may help with lifetime issues), making this a bool
> "render_view_is_owner_" or something.

Refactoring has removed this issue, there is only one target now.

http://codereview.chromium.org/8970016/diff/37001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/37001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:443: mouse_lock_dispatcher_ = new
MouseLockDispatcher(this);
On 2012/01/18 19:00:59, piman wrote:
> Where is this deleted? Should mouse_lock_dispatcher_ be a scoped_ptr?
I agree it's confusing, I've added the existing comment in the header file to
the .cc as well:
  // The next group of objects all implement RenderViewObserver, so are deleted
  // along with the RenderView automatically.  This is why we just store
  // weak references.

[piman, 2012-01-24 02:27:37]

Very nice test!
I think there's a missing delete, the rest is nits.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:29: : plugin_(plugin) { }
nit: indentation before ':' should be +4 (6 total)

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:51: : webwidget_(webwidget) { }
nit: indentation before ':' should be +4 (6 total)

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:73: };
High level comment: do these 2 classes need to be defined here, and only
accessible through the factory method below?
I'm thinking, RenderViewImpl could implement MouseLockDispatcher::LockTarget
(effectively these), and pass 'this' as the lock target.
I guess it doesn't save you much for the Pepper instance case, except maybe a
little bit of locality (in terms of new/delete for example). IMO It makes it
easier to convince oneself that PluginInstanceLockTarget won't outlive the
PluginInstance.

I don't feel strongly about this, so feel free to ignore if you think this is
better.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:101: if (target == target_ &&
!pending_unlock_request_) {
When comparing pointers to compare identity (here and below), I'm always asking
myself - is there any way that we get into a situation where the instance is
destroyed, and another one gets allocated and gets the same address?
I think in this case we rely on the fact that target_ is always set to NULL
before the instance is destroyed, but it's worth a comment I think.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher_unittest.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:46: int route_id_;
Typically we make the members protected, they should be accessible from the test
themselves, I think.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2091:
mouse_lock_instances_.erase(instance);
I think you still need to delete the LockTarget here.

[yzshen1, 2012-01-24 18:56:07]

http://codereview.chromium.org/8970016/diff/15002/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8970016/diff/15002/content/content_tests.gypi#...
content/content_tests.gypi:286: 'renderer/mouse_lock_dispatcher_unittest.cc',
Alphabetically, please.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:24: namespace {
It is better to put it on top of the file (and below those includes).

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:29: : plugin_(plugin) { }
nit: I think we don't put a space between {}.
(and also line 51)

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:101: if (target == target_ &&
!pending_unlock_request_) {
Please also check !NULL. Otherwise, an unlock request will be sent if currently
no one owns the lock and |target|==NULL.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:107: void
MouseLockDispatcher::UnlockMouseAndClearTarget(LockTarget* target) {
It is a little bit confusing: UnlockMouse() will also clear target. The only
difference is that it clears the target later.
Maybe call it something like OnLockTargetDestroyed()?

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:31: class LockTarget {
Please define a virtual destructor.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:42: static LockTarget*
CreateLockTarget(webkit::ppapi::PluginInstance* plugin);
Please comment about the ownership of the returned object.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:45: // Lock the mouse to the |target|.
If true is returned, an asynchronous
Lock -> Locks. (And some other places.)

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:48: // Request to unlock the mouse. An
asynchronous
Fit as many words in one line as possible. (And some other places.)

There may be no OnMouseLockLost() after this is called, right?

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:52: // longer valid. The pointer will
not be accessed again..
.. -> .

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:56: // Allow lock target to consumed a
mouse event, if it does returns true.
returns -> return.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher_unittest.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:10: #include
"mouse_lock_dispatcher.h"
Please add path.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:85: EXPECT_CALL(target,
OnLockMouseACK(true));
Important note: gMock requires expectations to be set before the mock functions
are called, otherwise the behavior is undefined. In particular, you mustn't
interleave EXPECT_CALL()s and calls to the mock functions.

from: https://wiki.corp.google.com/twiki/bin/view/Main/GMockForDummies

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:196: MockLockTarget& target =
*(target_.get());
Now that you define a ref explicitly at the top of every method. Why not create
an object directly here?

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2083:
MouseLockDispatcher::CreateLockTarget(instance);
4 space indent, please.

http://codereview.chromium.org/8970016/diff/15002/ppapi/examples/mouse_lock/m...
File ppapi/examples/mouse_lock/mouse_lock.html (right):

http://codereview.chromium.org/8970016/diff/15002/ppapi/examples/mouse_lock/m...
ppapi/examples/mouse_lock/mouse_lock.html:83: '<object
type="application/x-ppapi-example-mouse-lock" ' +
4 space indent? (But I am not familiar with javascript style.)

[scheib, 2012-01-25 00:27:10]

Thanks again.
Bonus: the WebKit API has landed and rolled in DEPs. I have removed the TODOs
associated with that and this patch will not require a followup.

http://codereview.chromium.org/8970016/diff/15002/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8970016/diff/15002/content/content_tests.gypi#...
content/content_tests.gypi:286: 'renderer/mouse_lock_dispatcher_unittest.cc',
On 2012/01/24 18:56:08, yzshen1 wrote:
> Alphabetically, please.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:24: namespace {
On 2012/01/24 18:56:08, yzshen1 wrote:
> It is better to put it on top of the file (and below those includes).

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:29: : plugin_(plugin) { }
On 2012/01/24 18:56:08, yzshen1 wrote:
> nit: I think we don't put a space between {}.
> (and also line 51)

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:51: : webwidget_(webwidget) { }
On 2012/01/24 02:27:37, piman wrote:
> nit: indentation before ':' should be +4 (6 total)

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:73: };
On 2012/01/24 02:27:37, piman wrote:
> High level comment: do these 2 classes need to be defined here, and only
> accessible through the factory method below?
> I'm thinking, RenderViewImpl could implement MouseLockDispatcher::LockTarget
> (effectively these), and pass 'this' as the lock target.
> I guess it doesn't save you much for the Pepper instance case, except maybe a
> little bit of locality (in terms of new/delete for example). IMO It makes it
> easier to convince oneself that PluginInstanceLockTarget won't outlive the
> PluginInstance.
> 
> I don't feel strongly about this, so feel free to ignore if you think this is
> better.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:101: if (target == target_ &&
!pending_unlock_request_) {
On 2012/01/24 02:27:37, piman wrote:
> When comparing pointers to compare identity (here and below), I'm always
asking
> myself - is there any way that we get into a situation where the instance is
> destroyed, and another one gets allocated and gets the same address?
> I think in this case we rely on the fact that target_ is always set to NULL
> before the instance is destroyed, but it's worth a comment I think.

It's a good consideration. But to add comments everywhere pointers are compared
would really bloat out the code. There's a lot incorrect behavior possible if
stale pointers are left and I don't see this concern being any more unusual or
prominent than the rest. I can add comments if you like but I think it will harm
readability.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:101: if (target == target_ &&
!pending_unlock_request_) {
On 2012/01/24 18:56:08, yzshen1 wrote:
> Please also check !NULL. Otherwise, an unlock request will be sent if
currently
> no one owns the lock and |target|==NULL.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.cc:107: void
MouseLockDispatcher::UnlockMouseAndClearTarget(LockTarget* target) {
On 2012/01/24 18:56:08, yzshen1 wrote:
> It is a little bit confusing: UnlockMouse() will also clear target. The only
> difference is that it clears the target later.
> Maybe call it something like OnLockTargetDestroyed()?

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:31: class LockTarget {
On 2012/01/24 18:56:08, yzshen1 wrote:
> Please define a virtual destructor.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:42: static LockTarget*
CreateLockTarget(webkit::ppapi::PluginInstance* plugin);
On 2012/01/24 18:56:08, yzshen1 wrote:
> Please comment about the ownership of the returned object.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:45: // Lock the mouse to the |target|.
If true is returned, an asynchronous
On 2012/01/24 18:56:08, yzshen1 wrote:
> Lock -> Locks. (And some other places.)

Done. er.. but I'm not sure if we have a preferred tense/conjugation for
comments? I think e.g. "Load the file" is ok vs "Loads the file".

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:48: // Request to unlock the mouse. An
asynchronous
On 2012/01/24 18:56:08, yzshen1 wrote:
> Fit as many words in one line as possible. (And some other places.)
> 
> There may be no OnMouseLockLost() after this is called, right?

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:52: // longer valid. The pointer will
not be accessed again..
On 2012/01/24 18:56:08, yzshen1 wrote:
> .. -> .

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:56: // Allow lock target to consumed a
mouse event, if it does returns true.
On 2012/01/24 18:56:08, yzshen1 wrote:
> returns -> return.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher_unittest.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:10: #include
"mouse_lock_dispatcher.h"
On 2012/01/24 18:56:08, yzshen1 wrote:
> Please add path.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:46: int route_id_;
On 2012/01/24 02:27:37, piman wrote:
> Typically we make the members protected, they should be accessible from the
test
> themselves, I think.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:85: EXPECT_CALL(target,
OnLockMouseACK(true));
On 2012/01/24 18:56:08, yzshen1 wrote:
> Important note: gMock requires expectations to be set before the mock
functions
> are called, otherwise the behavior is undefined. In particular, you mustn't
> interleave EXPECT_CALL()s and calls to the mock functions.
> 
> from: https://wiki.corp.google.com/twiki/bin/view/Main/GMockForDummies

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher_unittest.cc:196: MockLockTarget& target =
*(target_.get());
On 2012/01/24 18:56:08, yzshen1 wrote:
> Now that you define a ref explicitly at the top of every method. Why not
create
> an object directly here?
Done: I've removed the per TEST reference line, and placed a bare pointer in the
fixture.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2083:
MouseLockDispatcher::CreateLockTarget(instance);
On 2012/01/24 18:56:08, yzshen1 wrote:
> 4 space indent, please.

Done.

http://codereview.chromium.org/8970016/diff/15002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2091:
mouse_lock_instances_.erase(instance);
On 2012/01/24 02:27:37, piman wrote:
> I think you still need to delete the LockTarget here.

Done.

[yzshen1, 2012-01-25 18:10:00]

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/15002/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:45: // Lock the mouse to the |target|.
If true is returned, an asynchronous
According to our code style, the second one is preferred.

On 2012/01/25 00:27:11, scheib wrote:
> On 2012/01/24 18:56:08, yzshen1 wrote:
> > Lock -> Locks. (And some other places.)
> 
> Done. er.. but I'm not sure if we have a preferred tense/conjugation for
> comments? I think e.g. "Load the file" is ok vs "Loads the file".

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:666:
webkit::ppapi::PluginInstance* plugin_;
Better to place it in a private section.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:880:
PepperPluginDelegateImpl::~PepperPluginDelegateImpl() {
Can you double check here to make sure the map of lock targets is empty?

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2110:
MouseLockDispatcher::LockTarget* target = mouse_lock_instances_[instance];
If |instance| is not in the map. This line will insert it.
And it won't get erased below because |target| will be NULL.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:363: WebKit::WebWidget* webwidget_;
private, please.

[piman, 2012-01-26 01:31:26]

http://codereview.chromium.org/8970016/diff/56001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:80: // owning reference here that must
be cleared by |UnlockMouseAndClearTarget|
nit: s/UnlockMouseAndClearTarget/OnLockTargetDestroyed

[scheib, 2012-01-26 01:37:59]

http://codereview.chromium.org/8970016/diff/56001/content/renderer/mouse_lock...
File content/renderer/mouse_lock_dispatcher.h (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/mouse_lock...
content/renderer/mouse_lock_dispatcher.h:80: // owning reference here that must
be cleared by |UnlockMouseAndClearTarget|
On 2012/01/26 01:31:27, piman wrote:
> nit: s/UnlockMouseAndClearTarget/OnLockTargetDestroyed

Done.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:666:
webkit::ppapi::PluginInstance* plugin_;
On 2012/01/25 18:10:01, yzshen1 wrote:
> Better to place it in a private section.

Done.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:880:
PepperPluginDelegateImpl::~PepperPluginDelegateImpl() {
On 2012/01/25 18:10:01, yzshen1 wrote:
> Can you double check here to make sure the map of lock targets is empty?

Done.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:2110:
MouseLockDispatcher::LockTarget* target = mouse_lock_instances_[instance];
On 2012/01/25 18:10:01, yzshen1 wrote:
> If |instance| is not in the map. This line will insert it.
> And it won't get erased below because |target| will be NULL.

Done.

http://codereview.chromium.org/8970016/diff/56001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8970016/diff/56001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:363: WebKit::WebWidget* webwidget_;
On 2012/01/25 18:10:01, yzshen1 wrote:
> private, please.

Done.

[piman, 2012-01-26 01:59:33]

LGTM+nit

http://codereview.chromium.org/8970016/diff/56004/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8970016/diff/56004/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:880:
DCHECK(!mouse_lock_instances_.size());
nit: DCHECK(mouse_lock_instances_.empty());

[yzshen1, 2012-01-26 02:15:12]

lgtm

[commit-bot: I haz the power, 2012-01-26 03:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/8970016/55027

[commit-bot: I haz the power, 2012-01-26 07:13:22]

Change committed as 119206

[commit-bot: I haz the power, 2012-01-27 05:41:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/8970016/60002

[commit-bot: I haz the power, 2012-01-27 07:33:13]

Change committed as 119406