Change Chromoting client to use Pepper's new Resource-base InputEvents.

Change Chromoting client to use Pepper's new Resource-base InputEvents.

Remove gfx::Point and gfx::Rect from the Chromoting client plugin code and consistently use pp::Point and pp::Rect.

Push ConvertScreenToHost down into PepperView so it can take and return a pp::Point.

BUG=none
TEST=none

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

[dmac, 2011-07-20 00:34:31]

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.cc:205:
(event.GetType()==PP_INPUTEVENT_TYPE_KEYDOWN ? "DOWN" : "UP"),
space between ) and ==

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_input_handler.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton() ==
PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
Would a switch on event.GetMouseButton() be more readable and give us a compile
time check to make sure we hit all the cases if more are added later?

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.cc:133: pp::Rect r_scaled =
UpdateScaledBackingStore(
is there really no gfx::Rect to pp:Rect conversion routine?

[Wez, 2011-07-20 00:58:35]

http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
File remoting/client/chromoting_view.h (left):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
remoting/client/chromoting_view.h:78: virtual gfx::Point
ConvertScreenToHost(const gfx::Point& p) const = 0;
Isn't this view-to-host coordinate conversion, rather than screen-to-host?

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.cc:207:
pih->HandleKeyEvent(event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN, key);
nit: Why use event.GetType() == foo here, but not for MOUSEDOWN/MOUSEUP?

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.h:32: class Module;
Need to forward-define InputEvent here, too.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_input_handler.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:43: pp::Point p =
pepper_view->ConvertScreenToHost(event.GetMousePosition());
ConvertScreenToHost accepts & returns a gfx::Point, not a pp::Point, doesn't it?

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton() ==
PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
On 2011/07/20 00:34:31, dmac wrote:
> Would a switch on event.GetMouseButton() be more readable and give us a
compile
> time check to make sure we hit all the cases if more are added later?

Although we still need to cope with cases we don't recognize, so we don't break
if Pepper gets extended.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.h:59: // screen.
"and clips to the host screen" doesn't sound right.  The Host needs to cope with
mouse coordinates outside its dimensions, since it may be resized asynchronously
to client mouse movement.  What is the purpose of clipping here?

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.h:81: pp::Point ConvertHostToScreen(const
pp::Point& p) const;
nit: Naming.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view_proxy.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view_proxy.h:61: // called only on the pepper
thread.
nit: How does this differ from the other methods on this class?

[garykac, 2011-07-20 18:55:27]

http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
File remoting/client/chromoting_view.h (left):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
remoting/client/chromoting_view.h:78: virtual gfx::Point
ConvertScreenToHost(const gfx::Point& p) const = 0;
On 2011/07/20 00:58:35, Wez wrote:
> Isn't this view-to-host coordinate conversion, rather than screen-to-host?

I think so, but the term screen is used through the view code: ScreenToHost,
HostToScreen, screen_x_to_host_x_, ...

Any renaming should be a separate cl.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.cc:205:
(event.GetType()==PP_INPUTEVENT_TYPE_KEYDOWN ? "DOWN" : "UP"),
On 2011/07/20 00:34:31, dmac wrote:
> space between ) and ==

Done.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.cc:207:
pih->HandleKeyEvent(event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN, key);
On 2011/07/20 00:58:35, Wez wrote:
> nit: Why use event.GetType() == foo here, but not for MOUSEDOWN/MOUSEUP?

made consistent

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_instance.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_instance.h:32: class Module;
On 2011/07/20 00:58:35, Wez wrote:
> Need to forward-define InputEvent here, too.

Done.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_input_handler.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:43: pp::Point p =
pepper_view->ConvertScreenToHost(event.GetMousePosition());
On 2011/07/20 00:58:35, Wez wrote:
> ConvertScreenToHost accepts & returns a gfx::Point, not a pp::Point, doesn't
it?

err.. not any more.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton() ==
PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
On 2011/07/20 00:34:31, dmac wrote:
> Would a switch on event.GetMouseButton() be more readable and give us a
compile
> time check to make sure we hit all the cases if more are added later?

Done.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton() ==
PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
On 2011/07/20 00:58:35, Wez wrote:
> On 2011/07/20 00:34:31, dmac wrote:
> > Would a switch on event.GetMouseButton() be more readable and give us a
> compile
> > time check to make sure we hit all the cases if more are added later?
> 
> Although we still need to cope with cases we don't recognize, so we don't
break
> if Pepper gets extended.

word

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.cc:133: pp::Rect r_scaled =
UpdateScaledBackingStore(
On 2011/07/20 00:34:31, dmac wrote:
> is there really no gfx::Rect to pp:Rect conversion routine?

Nope. The old code did a conversion like this as well.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.h:59: // screen.
On 2011/07/20 00:58:35, Wez wrote:
> "and clips to the host screen" doesn't sound right.  The Host needs to cope
with
> mouse coordinates outside its dimensions, since it may be resized
asynchronously
> to client mouse movement.  What is the purpose of clipping here?

!ExpandingScopeAlert!

We should follow up with Simon re: this because the comment (copied from
chromoting_view.h) is correct - it does clip to the host width/height.

And I agree that it doesn't seem like it should need to be clipping to the host.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.h:81: pp::Point ConvertHostToScreen(const
pp::Point& p) const;
On 2011/07/20 00:58:35, Wez wrote:
> nit: Naming.

see previous comment re: screen.

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view_proxy.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view_proxy.h:61: // called only on the pepper
thread.
On 2011/07/20 00:58:35, Wez wrote:
> nit: How does this differ from the other methods on this class?

It returns a value, whereas all the other methods return void.

I think I'm misunderstanding your question here.

[simonmorris, 2011-07-20 19:12:32]

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.h (right):

http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.h:59: // screen.
On 2011/07/20 18:55:28, garykac wrote:
> On 2011/07/20 00:58:35, Wez wrote:
> > "and clips to the host screen" doesn't sound right.  The Host needs to cope
> with
> > mouse coordinates outside its dimensions, since it may be resized
> asynchronously
> > to client mouse movement.  What is the purpose of clipping here?
> 
> !ExpandingScopeAlert!
> 
> We should follow up with Simon re: this because the comment (copied from
> chromoting_view.h) is correct - it does clip to the host width/height.
> 
> And I agree that it doesn't seem like it should need to be clipping to the
host.

I think I remember adding the "clip to host" for
scale-to-fit. Without it, the host (a Mac, I think)
could lose the cursor entirely if the client cursor
moved outside the view of the host screen.

Maybe that's been fixed now, but I agree it ought to be.
Probably in another CL.

[Wez, 2011-07-20 22:36:29]

On 2011/07/20 18:55:27, garykac wrote:
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
> File remoting/client/chromoting_view.h (left):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/chromoting_v...
> remoting/client/chromoting_view.h:78: virtual gfx::Point
> ConvertScreenToHost(const gfx::Point& p) const = 0;
> On 2011/07/20 00:58:35, Wez wrote:
> > Isn't this view-to-host coordinate conversion, rather than screen-to-host?
> 
> I think so, but the term screen is used through the view code: ScreenToHost,
> HostToScreen, screen_x_to_host_x_, ...
> 
> Any renaming should be a separate cl.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
> File remoting/client/plugin/chromoting_instance.cc (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
> remoting/client/plugin/chromoting_instance.cc:205:
> (event.GetType()==PP_INPUTEVENT_TYPE_KEYDOWN ? "DOWN" : "UP"),
> On 2011/07/20 00:34:31, dmac wrote:
> > space between ) and ==
> 
> Done.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
> remoting/client/plugin/chromoting_instance.cc:207:
> pih->HandleKeyEvent(event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN, key);
> On 2011/07/20 00:58:35, Wez wrote:
> > nit: Why use event.GetType() == foo here, but not for MOUSEDOWN/MOUSEUP?
> 
> made consistent
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
> File remoting/client/plugin/chromoting_instance.h (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/chrom...
> remoting/client/plugin/chromoting_instance.h:32: class Module;
> On 2011/07/20 00:58:35, Wez wrote:
> > Need to forward-define InputEvent here, too.
> 
> Done.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> File remoting/client/plugin/pepper_input_handler.cc (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_input_handler.cc:43: pp::Point p =
> pepper_view->ConvertScreenToHost(event.GetMousePosition());
> On 2011/07/20 00:58:35, Wez wrote:
> > ConvertScreenToHost accepts & returns a gfx::Point, not a pp::Point, doesn't
> it?
> 
> err.. not any more.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton()
==
> PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
> On 2011/07/20 00:34:31, dmac wrote:
> > Would a switch on event.GetMouseButton() be more readable and give us a
> compile
> > time check to make sure we hit all the cases if more are added later?
> 
> Done.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_input_handler.cc:51: if (event.GetMouseButton()
==
> PP_INPUTEVENT_MOUSEBUTTON_LEFT) {
> On 2011/07/20 00:58:35, Wez wrote:
> > On 2011/07/20 00:34:31, dmac wrote:
> > > Would a switch on event.GetMouseButton() be more readable and give us a
> > compile
> > > time check to make sure we hit all the cases if more are added later?
> > 
> > Although we still need to cope with cases we don't recognize, so we don't
> break
> > if Pepper gets extended.
> 
> word
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> File remoting/client/plugin/pepper_view.cc (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_view.cc:133: pp::Rect r_scaled =
> UpdateScaledBackingStore(
> On 2011/07/20 00:34:31, dmac wrote:
> > is there really no gfx::Rect to pp:Rect conversion routine?
> 
> Nope. The old code did a conversion like this as well.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> File remoting/client/plugin/pepper_view.h (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_view.h:59: // screen.
> On 2011/07/20 00:58:35, Wez wrote:
> > "and clips to the host screen" doesn't sound right.  The Host needs to cope
> with
> > mouse coordinates outside its dimensions, since it may be resized
> asynchronously
> > to client mouse movement.  What is the purpose of clipping here?
> 
> !ExpandingScopeAlert!
> 
> We should follow up with Simon re: this because the comment (copied from
> chromoting_view.h) is correct - it does clip to the host width/height.
> 
> And I agree that it doesn't seem like it should need to be clipping to the
host.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_view.h:81: pp::Point ConvertHostToScreen(const
> pp::Point& p) const;
> On 2011/07/20 00:58:35, Wez wrote:
> > nit: Naming.
> 
> see previous comment re: screen.
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> File remoting/client/plugin/pepper_view_proxy.h (right):
> 
>
http://codereview.chromium.org/7453003/diff/4001/remoting/client/plugin/peppe...
> remoting/client/plugin/pepper_view_proxy.h:61: // called only on the pepper
> thread.
> On 2011/07/20 00:58:35, Wez wrote:
> > nit: How does this differ from the other methods on this class?
> 
> It returns a value, whereas all the other methods return void.
> 
> I think I'm misunderstanding your question here.

My point was really just that that method doesn't do anything that requires a
reply from another thread; it just processes an input value, so does it really
need to be tagged as "blocking"?

LGTM other than that, with these changes.