[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924
Committed: https://crrev.com/688299aff8d5fff9f9c238ebe5acccaca3110470
Cr-Commit-Position: refs/heads/master@{#408243}

[Yuwei, 2016-07-27 00:12:17]

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org, sergeyu@chromium.org

[Yuwei, 2016-07-27 00:12:17]

ptal

If you are interested in the actual use of this class, you can check out
JniGlDisplayHandler.cc in the WIP branch:

https://codereview.chromium.org/2032963002

Thanks!

[Sergey Ulanov, 2016-07-27 18:26:09]

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.cc (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.cc:22: if (task_runner_) {
Do you really need to support case when task_runner_ isn't set?

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.h (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:16: // CursorShapeStub. The
callback will be run when SetCursorShape is called.
The comment doesn't mention that the callback is called on another thread.

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:17: class CallbackCursorShapeStub :
public protocol::CursorShapeStub {
In other places we call classes like this that move interface calls from on
thread to another a "proxy". So for consistency I think this should really be
called CursorShapeStubProxy and instead of taking a callback it should take a
CursorShapeStub (maybe WeakPtr<CursorShapeStub>). This also means that
Jni*DesktopHandler will need to implement CursorShapeStub.
This will also be useful if we needed to add other methods in that interface.

[Yuwei, 2016-07-27 18:37:20]

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.cc (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.cc:22: if (task_runner_) {
On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> Do you really need to support case when task_runner_ isn't set?

It's not really being used... I'll drop it...

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.h (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:17: class CallbackCursorShapeStub :
public protocol::CursorShapeStub {
On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> In other places we call classes like this that move interface calls from on
> thread to another a "proxy". So for consistency I think this should really be
> called CursorShapeStubProxy and instead of taking a callback it should take a
> CursorShapeStub (maybe WeakPtr<CursorShapeStub>). This also means that
> Jni*DesktopHandler will need to implement CursorShapeStub.
> This will also be useful if we needed to add other methods in that interface.

The class being proxied will need to implement the CursorShapeStub on display
thread, but It sounds like a CursorShapeStub should be run on the network
thread... This should work but may be slightly confusing?

[Sergey Ulanov, 2016-07-27 18:47:51]

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.h (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:17: class CallbackCursorShapeStub :
public protocol::CursorShapeStub {
On 2016/07/27 18:37:20, Yuwei wrote:
> On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> > In other places we call classes like this that move interface calls from on
> > thread to another a "proxy". So for consistency I think this should really
be
> > called CursorShapeStubProxy and instead of taking a callback it should take
a
> > CursorShapeStub (maybe WeakPtr<CursorShapeStub>). This also means that
> > Jni*DesktopHandler will need to implement CursorShapeStub.
> > This will also be useful if we needed to add other methods in that
interface.
> 
> The class being proxied will need to implement the CursorShapeStub on display
> thread, but It sounds like a CursorShapeStub should be run on the network
> thread... This should work but may be slightly confusing?

There is no requirement that CursorShapeStub _interface_ is used on network
thread. But each implementation of that interface may require to be called on a
specific thread. JniDesktopHandler will implement CursorShapeStub and that
implementation will be called on desktop thread.

[Yuwei, 2016-07-27 19:22:28]

ptal

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.cc (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.cc:22: if (task_runner_) {
On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> Do you really need to support case when task_runner_ isn't set?

Done. Removed.

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
File remoting/client/callback_cursor_shape_stub.h (right):

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:16: // CursorShapeStub. The
callback will be run when SetCursorShape is called.
On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> The comment doesn't mention that the callback is called on another thread.

Added comment.

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:17: class CallbackCursorShapeStub :
public protocol::CursorShapeStub {
On 2016/07/27 18:47:51, Sergey Ulanov wrote:
> On 2016/07/27 18:37:20, Yuwei wrote:
> > On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> > > In other places we call classes like this that move interface calls from
on
> > > thread to another a "proxy". So for consistency I think this should really
> be
> > > called CursorShapeStubProxy and instead of taking a callback it should
take
> a
> > > CursorShapeStub (maybe WeakPtr<CursorShapeStub>). This also means that
> > > Jni*DesktopHandler will need to implement CursorShapeStub.
> > > This will also be useful if we needed to add other methods in that
> interface.
> > 
> > The class being proxied will need to implement the CursorShapeStub on
display
> > thread, but It sounds like a CursorShapeStub should be run on the network
> > thread... This should work but may be slightly confusing?
> 
> There is no requirement that CursorShapeStub _interface_ is used on network
> thread. But each implementation of that interface may require to be called on
a
> specific thread. JniDesktopHandler will implement CursorShapeStub and that
> implementation will be called on desktop thread.

Acknowledged.

https://codereview.chromium.org/2187723002/diff/20001/remoting/client/callbac...
remoting/client/callback_cursor_shape_stub.h:17: class CallbackCursorShapeStub :
public protocol::CursorShapeStub {
On 2016/07/27 18:26:09, Sergey Ulanov wrote:
> In other places we call classes like this that move interface calls from on
> thread to another a "proxy". So for consistency I think this should really be
> called CursorShapeStubProxy and instead of taking a callback it should take a
> CursorShapeStub (maybe WeakPtr<CursorShapeStub>). This also means that
> Jni*DesktopHandler will need to implement CursorShapeStub.
> This will also be useful if we needed to add other methods in that interface.

Done.

[Sergey Ulanov, 2016-07-27 20:38:01]

lgtm

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
File remoting/client/cursor_shape_stub_proxy.cc (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.cc:21: DCHECK(task_runner_);
Don't need this DCHECK.
operator-> in scoped_refptr<> already asserts the pointer is not null:
https://codesearch.chromium.org/chromium/src/base/memory/ref_counted.h?rcl=0&...

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
File remoting/client/cursor_shape_stub_proxy.h (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.h:8: #include "base/callback.h"
don't need this anymore

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.h:17: // the thread that the underlying
CursorShapeStub lives.
Suggest rewording
s/the thread that the underlying CursorShapeStub lives/another/

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_display_handler.h (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_display_handler.h:38: // CursorShapeStub override.
nit: s/override/interface/

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_display_handler.h:38: // CursorShapeStub override.
nit: move this to private: section to make it clear this method is not called
directly.

[Yuwei, 2016-07-27 21:00:26]

Patchset #5 (id:80001) has been deleted

[Yuwei, 2016-07-27 21:01:36]

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
File remoting/client/cursor_shape_stub_proxy.cc (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.cc:21: DCHECK(task_runner_);
On 2016/07/27 20:38:01, Sergey Ulanov wrote:
> Don't need this DCHECK.
> operator-> in scoped_refptr<> already asserts the pointer is not null:
>
https://codesearch.chromium.org/chromium/src/base/memory/ref_counted.h?rcl=0&...

Done.

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
File remoting/client/cursor_shape_stub_proxy.h (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.h:8: #include "base/callback.h"
On 2016/07/27 20:38:01, Sergey Ulanov wrote:
> don't need this anymore

Done.

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/cursor_...
remoting/client/cursor_shape_stub_proxy.h:17: // the thread that the underlying
CursorShapeStub lives.
On 2016/07/27 20:38:01, Sergey Ulanov wrote:
> Suggest rewording
> s/the thread that the underlying CursorShapeStub lives/another/

Done.

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_display_handler.h (right):

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_display_handler.h:38: // CursorShapeStub override.
On 2016/07/27 20:38:01, Sergey Ulanov wrote:
> nit: s/override/interface/

Done.

https://codereview.chromium.org/2187723002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_display_handler.h:38: // CursorShapeStub override.
On 2016/07/27 20:38:01, Sergey Ulanov wrote:
> nit: move this to private: section to make it clear this method is not called
> directly.

Done.

That's interesting C++ logic... By overriding a public method with private
implementation, is it like you can access the method of the subclass but can
access after you cast it to its base class?

[Yuwei, 2016-07-27 21:02:21]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-07-27 21:02:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2187723002/#ps60001
(title: "Reviewer's Feedback")

[commit-bot: I haz the power, 2016-07-27 21:02:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2016-07-27 21:20:22]

Description was changed from

==========
[Remoting Android] Refactor CallbackCursorShapeStub from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that wraps a cursor shape updating callback into a CursorShapeStub
object. It can be used by JniGlDisplayHandler to update the cursor shape for
the remoting Android OpenGL host renderer implementation.

BUG=385924
==========

to

==========
[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924
==========

[commit-bot: I haz the power, 2016-07-27 21:32:46]

Description was changed from

==========
[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924
==========

to

==========
[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924
==========

[commit-bot: I haz the power, 2016-07-27 21:32:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 21:34:36]

Description was changed from

==========
[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924
==========

to

==========
[Remoting Android] Refactor CursorShapeStubProxy from JniDisplayHandler

This CL refactors the CursorShapeStub being used by JniDisplayHandler into a
helper class that forwards cursor shape info from one
thread to another. It can be used by JniGlDisplayHandler
to update the cursor shape for the remoting Android
OpenGL host renderer implementation.

BUG=385924

Committed: https://crrev.com/688299aff8d5fff9f9c238ebe5acccaca3110470
Cr-Commit-Position: refs/heads/master@{#408243}
==========

[commit-bot: I haz the power, 2016-07-27 21:34:39]

Patchset 4 (id:??) landed as
https://crrev.com/688299aff8d5fff9f9c238ebe5acccaca3110470
Cr-Commit-Position: refs/heads/master@{#408243}