Add movement information to PPB_MouseInputEvent.

Add movement information to PPB_MouseInputEvent.


BUG=None
TEST=None

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

[yzshen1, 2011-08-24 00:31:51]

Hi, Darin and Brett.

Is it okay that I directly increase the version of a stable interface and put
the new interface in the c/ folder from the beginning? Or I should put the new
interface in dev/ instead?

Given that we are going to support mouse lock in M15, I assume that both the
change to PPB_MouseInputEvent and PPB/PPP_CursorControl will need to be out of
dev/. - Right?

Thanks!

[brettw, 2011-08-24 20:55:09]

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
ppapi/c/ppb_input_event.h:24: #define PPB_MOUSE_INPUT_EVENT_INTERFACE_1_1
"PPB_MouseInputEvent;1.1"
What are we doing with the IDL for this? I don't see any IDL changes.

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
ppapi/c/ppb_input_event.h:461: struct PPB_MouseInputEvent {
Since we're updating the interface, can you email pepper-dev and point to the
additions?

[yzshen, 2011-08-24 21:11:29]

Hi, Brett.

Thanks! Please see inline comments.

On Wed, Aug 24, 2011 at 1:55 PM, <brettw@chromium.org> wrote:

>
> http://codereview.chromium.**org/7715021/diff/1/ppapi/c/**
>
ppb_input_event.h<http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h>
> File ppapi/c/ppb_input_event.h (right):
>
> http://codereview.chromium.**org/7715021/diff/1/ppapi/c/**
>
ppb_input_event.h#newcode24<http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newcode24>
> ppapi/c/ppb_input_event.h:24: #define
> PPB_MOUSE_INPUT_EVENT_**INTERFACE_1_1 "PPB_MouseInputEvent;1.1"
> What are we doing with the IDL for this? I don't see any IDL changes.
>
[+ Noel]
I asked noelallen about this. And he said the generator hasn't supported
such kind of change yet.
Is it okay to add the IDL change later? Or it is better to modify the IDL
but _not_ using it to generate the ppb_input_event.h file?

>
> http://codereview.chromium.**org/7715021/diff/1/ppapi/c/**
>
ppb_input_event.h#newcode461<http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newcode461>
> ppapi/c/ppb_input_event.h:461: struct PPB_MouseInputEvent {
> Since we're updating the interface, can you email pepper-dev and point
> to the additions?

Do you mean I ask for review before landing it or notify the team after
landing it?


>
>
>
http://codereview.chromium.**org/7715021/<http://codereview.chromium.org/7715...
>



-- 
Best regards,
Yuzhu Shen.

[brettw, 2011-08-24 21:38:47]

On Wed, Aug 24, 2011 at 2:11 PM, Yuzhu Shen <yzshen@google.com> wrote:
> Hi, Brett.
> Thanks! Please see inline comments.
> On Wed, Aug 24, 2011 at 1:55 PM, <brettw@chromium.org> wrote:
>>
>> http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h
>> File ppapi/c/ppb_input_event.h (right):
>>
>>
>>
http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
>> ppapi/c/ppb_input_event.h:24: #define
>> PPB_MOUSE_INPUT_EVENT_INTERFACE_1_1 "PPB_MouseInputEvent;1.1"
>> What are we doing with the IDL for this? I don't see any IDL changes.
>
> [+ Noel]
> I asked noelallen about this. And he said the generator hasn't supported
> such kind of change yet.
> Is it okay to add the IDL change later? Or it is better to modify the IDL
> but _not_ using it to generate the ppb_input_event.h file?

If Noel is happy it's fine.


>>
http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
>> ppapi/c/ppb_input_event.h:461: struct PPB_MouseInputEvent {
>> Since we're updating the interface, can you email pepper-dev and point
>> to the additions?
>
> Do you mean I ask for review before landing it or notify the team after
> landing it?

I'd do it now so people can comment before it goes in.

Brett

[dmichael (off chromium), 2011-08-24 22:32:41]

LGTM (though I think you should wait for brettw or darin to corroborate)

There will be an impact in the NaCl proxy when they roll DEPS, so ideally, you
should contact somebody (maybe the sheriff, polina currently) in NaCl so they're
prepared. If this goes in Monday or later, I can do the NaCl part.

[yzshen, 2011-08-24 22:42:03]

Thanks, Dave!

On Wed, Aug 24, 2011 at 3:32 PM, <dmichael@chromium.org> wrote:

> LGTM (though I think you should wait for brettw or darin to corroborate)
>
> There will be an impact in the NaCl proxy when they roll DEPS, so ideally,
> you
> should contact somebody (maybe the sheriff, polina currently) in NaCl so
> they're
> prepared. If this goes in Monday or later, I can do the NaCl part.

Once this change looks good to everyone, I will notify either you or polina.

>
>
>
http://codereview.chromium.**org/7715021/<http://codereview.chromium.org/7715...
>



-- 
Best regards,
Yuzhu Shen.

[brettw, 2011-08-24 22:47:49]

LGTM. Maybe wait until tomorrow to check this in, in case somebody else has some
comments.

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
ppapi/c/ppb_input_event.h:390: * Create() creates a mouse input event with the
given parameters. Normally
Lets remove the comments from the functions in the 1_0 interface struct so it's
just all the functions smooshed together. I'm worried about somebody updating
the documentation and being confused or updating only one place.

[yzshen1, 2011-08-24 23:22:01]

Thanks Brett!

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/7715021/diff/1/ppapi/c/ppb_input_event.h#newco...
ppapi/c/ppb_input_event.h:390: * Create() creates a mouse input event with the
given parameters. Normally
On 2011/08/24 22:47:49, brettw wrote:
> Lets remove the comments from the functions in the 1_0 interface struct so
it's
> just all the functions smooshed together. I'm worried about somebody updating
> the documentation and being confused or updating only one place.

Done.

[dmichael (off chromium), 2011-08-30 17:07:42]

Now that NaCl has been moved directly in to the Chrome tree, the NaCl proxy will
need to be updated in this same CL. I'll try to list out what needs to change to
make it easier; it should be similar to what you've already done. We're working
on eventually consolidating so that there is only 1 proxy codebase, but we still
have a ways to go. Let me know if there's anything else I can do to help.

In:
ppapi/native_client/src/shared/, you'll need to:
- Edit ppb_input_event.srpc to add movement for creating mouse input events.
- Run 'scons srpcgen' to generate the appropriate RPC code (not actually sure
how to properly run this now; you might need bradnelson or noelallen's help).
- Update browser_ppb_input_event_rpc_server.cc and plugin_ppb_input_event.cc for
the CreateMouseInputEvent call.
- Add your new field to input_event_data.h/.cc
- Retrieve it to populate the InputEventData in browser_ppp_input_event.cc
- Implement GetMovement in plugin_ppb_input_event.cc, and leave compatibility
for the 1.0 version as well (as you did for the Chrome proxy).

[scheib, 2011-08-30 17:44:42]

http://codereview.chromium.org/7715021/diff/9002/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/7715021/diff/9002/ppapi/c/ppb_input_event.h#ne...
ppapi/c/ppb_input_event.h:450: * instance.
spec has been moving, we're deciding to report the last mouse cursor position,
not the center.

[yzshen, 2011-08-30 17:51:45]

Thanks! You answered the question before I asked! :)

On Tue, Aug 30, 2011 at 10:07 AM, <dmichael@chromium.org> wrote:

> Now that NaCl has been moved directly in to the Chrome tree, the NaCl proxy
> will
> need to be updated in this same CL. I'll try to list out what needs to
> change to
> make it easier; it should be similar to what you've already done. We're
> working
> on eventually consolidating so that there is only 1 proxy codebase, but we
> still
> have a ways to go. Let me know if there's anything else I can do to help.
>
> In:
> ppapi/native_client/src/**shared/, you'll need to:
> - Edit ppb_input_event.srpc to add movement for creating mouse input
> events.
> - Run 'scons srpcgen' to generate the appropriate RPC code (not actually
> sure
> how to properly run this now; you might need bradnelson or noelallen's
> help).
> - Update browser_ppb_input_event_rpc_**server.cc and
> plugin_ppb_input_event.cc for
> the CreateMouseInputEvent call.
> - Add your new field to input_event_data.h/.cc
> - Retrieve it to populate the InputEventData in browser_ppp_input_event.cc
> - Implement GetMovement in plugin_ppb_input_event.cc, and leave
> compatibility
> for the 1.0 version as well (as you did for the Chrome proxy).
>
>
>
http://codereview.chromium.**org/7715021/<http://codereview.chromium.org/7715...
>



-- 
Best regards,
Yuzhu Shen.

[yzshen1, 2011-08-30 22:15:55]

Thanks!

David, would you please review the NaCl-related change?

http://codereview.chromium.org/7715021/diff/9002/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/7715021/diff/9002/ppapi/c/ppb_input_event.h#ne...
ppapi/c/ppb_input_event.h:450: * instance.
On 2011/08/30 17:44:42, scheib wrote:
> spec has been moving, we're deciding to report the last mouse cursor position,
> not the center.

Done.

[dmichael (off chromium), 2011-08-31 14:58:49]

NaCl proxy LGTM

[commit-bot: I haz the power, 2011-08-31 15:16:43]

Presubmit check for 7715021-18001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 2281, 106
chars

License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  ppapi/native_client/src/shared/ppapi_proxy/browser_ppp_input_event.cc \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc \
  ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_input_event.h \
  ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_input_event.cc \
  ppapi/native_client/src/shared/ppapi_proxy/untrusted/srpcgen/ppb_rpc.h \
  ppapi/native_client/src/shared/ppapi_proxy/input_event_data.cc \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc \
 
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_input_event_rpc_server.cc
\
  ppapi/native_client/src/shared/ppapi_proxy/trusted/srpcgen/ppb_rpc.h \
  ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc \
  ppapi/native_client/src/shared/ppapi_proxy/input_event_data.h

Presubmit checks took 3.6s to calculate.