Add new MouseCursor interface for setting the mouse cursor.

Add new MouseCursor interface for setting the mouse cursor.

Remove most of the old cursor control interface. This keeps backwards compat for CursorControl.SetCursor (which just redirects to the new function) to kepe existing users running. None of the other functions on cursor control were implemented, so I removed all the proxying and stuff for them.

BUG=
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131314

[dmichael (off chromium), 2012-03-26 17:43:16]

http://codereview.chromium.org/9814015/diff/8061/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9814015/diff/8061/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:1029: TEST_PPAPI_IN_PROCESS(MouseCursor)
you could separate your 3 tests out, though I'm not pushing that until we have
the tests running faster (i.e., without having to restart the browser)

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.idl
File ppapi/api/ppb_mouse_cursor.idl (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.i...
ppapi/api/ppb_mouse_cursor.idl:100: * was invalid, or if the image too large.
nit: image "is" too large

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.i...
ppapi/api/ppb_mouse_cursor.idl:102: PP_Bool SetCursor([in] PP_Instance instance,
This could have been 2 functions, maybe
SetCursor(type)
SetCustomCursor(image, hot_spot)
That seems clearer from an end-developer perspective. What's the rationale for
doing it in 1?

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_mouse_cursor.h
(right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_mouse_cursor.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/tests/pp...
File ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/tests/pp...
ppapi/native_client/tests/ppapi_test_lib/get_browser_interface.cc:161: // Change
to GetBrowserInterfaceSafe when moving out of dev.
So it should use GetBrowserInterfaceSafe, and move to the proper place above the
"Dev interfaces" line.

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/tests/pp...
File ppapi/native_client/tests/ppapi_tests/nacl.scons (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/native_client/tests/pp...
ppapi/native_client/tests/ppapi_tests/nacl.scons:40: 'test_mouse_cursor.cc',
nit: alphabetical

http://codereview.chromium.org/9814015/diff/8061/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (left):

http://codereview.chromium.org/9814015/diff/8061/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:678:
IPC_SYNC_MESSAGE_ROUTED4_1(PpapiHostMsg_PPBCursorControl_SetCursor,
yeesh... more sync messages down, yay!

But it does beg the question...  is there any use case where an App needs to
know definitively that the cursor is now set? I think we can go with this and
see if anybody ever complains. It's probably perfectly fine in practice, but we
could add a completion callback later if we wanted.

http://codereview.chromium.org/9814015/diff/8061/ppapi/proxy/ppb_instance_pro...
File ppapi/proxy/ppb_instance_proxy.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/proxy/ppb_instance_pro...
ppapi/proxy/ppb_instance_proxy.cc:415: // parameters will be validated again in
the rendeer.
rendeer->renderer

http://codereview.chromium.org/9814015/diff/8061/ppapi/proxy/ppb_instance_pro...
ppapi/proxy/ppb_instance_proxy.cc:423: if (!image || cursor_image->pp_instance()
!= instance)
!image->!cursor_image?

http://codereview.chromium.org/9814015/diff/8061/ppapi/shared_impl/ppb_instan...
File ppapi/shared_impl/ppb_instance_shared.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/shared_impl/ppb_instan...
ppapi/shared_impl/ppb_instance_shared.cc:62: static_cast<int>(type) >
static_cast<int>(PP_MOUSECURSOR_TYPE_GRABBING))
optional: You could stick a "PP_MOUSECURSOR_TYPE_MAX" (or _END or whatever) on
the end of the list of enum values if you want this to be less arbitrary.

http://codereview.chromium.org/9814015/diff/8061/ppapi/shared_impl/ppb_instan...
ppapi/shared_impl/ppb_instance_shared.cc:80: // cursor to be a reasonably-sized
image.
Is there any way for the image to change size to be bigger in the renderer while
we still think it's small enough in the plugin process?

I'm thinking not (size is immutable for a given ImageData, right?), but since
it's security-relevant, I figured it was worth asking.

http://codereview.chromium.org/9814015/diff/8061/ppapi/tests/test_mouse_curso...
File ppapi/tests/test_mouse_cursor.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/tests/test_mouse_curso...
ppapi/tests/test_mouse_cursor.cc:29: std::string
TestMouseCursor::TestInvalidType() {
name nit: you are testing a valid type also <shrug/>

TestCursorType?

http://codereview.chromium.org/9814015/diff/8061/ppapi/tests/test_mouse_curso...
ppapi/tests/test_mouse_cursor.cc:38: std::string
TestMouseCursor::TestInvalidImage() {
ditto, could be TestCustom (or similar)

http://codereview.chromium.org/9814015/diff/8061/ppapi/tests/test_mouse_cursor.h
File ppapi/tests/test_mouse_cursor.h (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/tests/test_mouse_curso...
ppapi/tests/test_mouse_cursor.h:13: TestMouseCursor(TestingInstance* instance);
explicit

http://codereview.chromium.org/9814015/diff/8061/ppapi/thunk/interfaces_ppb_p...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/thunk/interfaces_ppb_p...
ppapi/thunk/interfaces_ppb_public_dev.h:35: PROXIED_IFACE(NoAPIName,
PPB_CURSOR_CONTROL_DEV_INTERFACE_0_4,
UNPROXIED?

http://codereview.chromium.org/9814015/diff/8061/ppapi/thunk/ppb_cursor_contr...
File ppapi/thunk/ppb_cursor_control_thunk.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/thunk/ppb_cursor_contr...
ppapi/thunk/ppb_cursor_control_thunk.cc:10: // This interface is only for
temporary backswards compat and currently just
bascswards->backwards

http://codereview.chromium.org/9814015/diff/8061/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/9814015/diff/8061/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1980: DoSetCursor(new
WebCursorInfo(static_cast<WebCursorInfo::Type>(type)));
Would it be possible to have a test or something to make sure your values line
up with the WebKit ones? I'm sure they're very unlikely to change, but if they
do, you have no way to detect it automatically. Comparing enum values can even
be done with compile asserts.

[brettw, 2012-03-26 17:59:30]

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.idl
File ppapi/api/ppb_mouse_cursor.idl (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.i...
ppapi/api/ppb_mouse_cursor.idl:102: PP_Bool SetCursor([in] PP_Instance instance,
On 2012/03/26 17:43:17, dmichael wrote:
> This could have been 2 functions, maybe
> SetCursor(type)
> SetCustomCursor(image, hot_spot)
> That seems clearer from an end-developer perspective. What's the rationale for
> doing it in 1?

A previous version of this patch actually did what you're suggesting. The
initial reason I gave up was that we were going to try to get this into 19 and
changing the signature would require me to write new proxies in both IPC and
NaCl, which was more work than I wanted. And we have to keep a function around
with this signature anyway since Flash needs it.

I think this is OK even though this is no longer M19 because it doesn't feel
like a function signature or behavior that's out-of-place. It seems pretty clear
and easy enough to use, and saves us the overhead of having to have thunks and
APIs and proxies for two different messages.

[brettw, 2012-03-27 05:17:19]

Other comments should be addressed. PTAL

http://codereview.chromium.org/9814015/diff/8061/ppapi/shared_impl/ppb_instan...
File ppapi/shared_impl/ppb_instance_shared.cc (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/shared_impl/ppb_instan...
ppapi/shared_impl/ppb_instance_shared.cc:80: // cursor to be a reasonably-sized
image.
No, image datas can't change sizes so we should be OK.

http://codereview.chromium.org/9814015/diff/8061/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/9814015/diff/8061/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1980: DoSetCursor(new
WebCursorInfo(static_cast<WebCursorInfo::Type>(type)));
There were already compiler asserts for this in ppapi_plugin_instance.cc, but I
realized it's still testing the old enum. I fixed this.

[dmichael (off chromium), 2012-03-27 15:48:44]

lgtm

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.idl
File ppapi/api/ppb_mouse_cursor.idl (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.i...
ppapi/api/ppb_mouse_cursor.idl:102: PP_Bool SetCursor([in] PP_Instance instance,
On 2012/03/26 17:59:30, brettw wrote:
> On 2012/03/26 17:43:17, dmichael wrote:
> > This could have been 2 functions, maybe
> > SetCursor(type)
> > SetCustomCursor(image, hot_spot)
> > That seems clearer from an end-developer perspective. What's the rationale
for
> > doing it in 1?
> 
> A previous version of this patch actually did what you're suggesting. The
> initial reason I gave up was that we were going to try to get this into 19 and
> changing the signature would require me to write new proxies in both IPC and
> NaCl, which was more work than I wanted. And we have to keep a function around
> with this signature anyway since Flash needs it.
> 
> I think this is OK even though this is no longer M19 because it doesn't feel
> like a function signature or behavior that's out-of-place. It seems pretty
clear
> and easy enough to use, and saves us the overhead of having to have thunks and
> APIs and proxies for two different messages.
I can live with it either way. The two behaviors seem different enough (enough
so that their paramaters are effectively disjoint) that it might be easier to
understand if separated out. I still prefer that, just for the clarity of the
API. And you could still do all the backing code (except the thunking) the same
way.

But I think this way's fine too if you prefer it.

http://codereview.chromium.org/9814015/diff/13001/ppapi/tests/test_mouse_curs...
File ppapi/tests/test_mouse_cursor.h (right):

http://codereview.chromium.org/9814015/diff/13001/ppapi/tests/test_mouse_curs...
ppapi/tests/test_mouse_cursor.h:13: TestMouseCursor(TestingInstance* instance);
nit: explicit

http://codereview.chromium.org/9814015/diff/13001/ppapi/tests/test_mouse_curs...
ppapi/tests/test_mouse_cursor.h:21: std::string TestCursom();
Cursom->Custom?

http://codereview.chromium.org/9814015/diff/13001/ppapi/thunk/ppb_cursor_cont...
File ppapi/thunk/ppb_cursor_control_thunk.cc (right):

http://codereview.chromium.org/9814015/diff/13001/ppapi/thunk/ppb_cursor_cont...
ppapi/thunk/ppb_cursor_control_thunk.cc:10: // This interface is only for
temporary backswards compat and currently just
backswards->backwards

[brettw, 2012-03-27 16:10:46]

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.idl
File ppapi/api/ppb_mouse_cursor.idl (right):

http://codereview.chromium.org/9814015/diff/8061/ppapi/api/ppb_mouse_cursor.i...
ppapi/api/ppb_mouse_cursor.idl:102: PP_Bool SetCursor([in] PP_Instance instance,
It might be nice, but I don't think it's worth the effort to change for such a
minor improvement, the API is easy enough to use.