[Remoting Android] Make JniClient own JniDisplayHandler

[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

BUG=385924
Committed: https://crrev.com/6cee11d75bb202e4dab39246837b069af2691404
Cr-Commit-Position: refs/heads/master@{#403272}

[Yuwei, 2016-06-27 23:06:59]

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

[Yuwei, 2016-06-27 23:06:59]

ptal

[Lambros, 2016-06-27 23:51:44]

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
File remoting/client/jni/jni_display_handler.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
remoting/client/jni/jni_display_handler.cc:71: return
base::android::ScopedJavaLocalRef<jobject>(java_display_);
I'm not sure if this is correct? ScopedJavaLocalRef(foo) takes ownership of foo,
if I'm not mistaken? Looking at the implementation of the ctor
ScopedJavaLocalRef(const U& other), it looks like it takes ownership of
other.obj().

[Sergey Ulanov, 2016-06-27 23:59:17]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-06-27 23:59:18]

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
File remoting/client/jni/jni_display_handler.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
remoting/client/jni/jni_display_handler.cc:71: return
base::android::ScopedJavaLocalRef<jobject>(java_display_);
On 2016/06/27 23:51:44, Lambros wrote:
> I'm not sure if this is correct? ScopedJavaLocalRef(foo) takes ownership of
foo,
> if I'm not mistaken? Looking at the implementation of the ctor
> ScopedJavaLocalRef(const U& other), it looks like it takes ownership of
> other.obj().

It's a java object, so it cannot be owned. This line just creates a new
reference to the same object.

I think it can be replaced with 
  return java display_;

The constructor should be called implicitly.

[Yuwei, 2016-06-28 00:03:26]

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
File remoting/client/jni/jni_display_handler.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
remoting/client/jni/jni_display_handler.cc:71: return
base::android::ScopedJavaLocalRef<jobject>(java_display_);
On 2016/06/27 23:51:44, Lambros wrote:
> I'm not sure if this is correct? ScopedJavaLocalRef(foo) takes ownership of
foo,
> if I'm not mistaken? Looking at the implementation of the ctor
> ScopedJavaLocalRef(const U& other), it looks like it takes ownership of
> other.obj().

It will eventually call SetNewLocalRef(env, other.obj()) but that should be
creating a new local reference to other.obj(). I don't think you can take
ownership of a jobject?

What JniDisplayHandler does is just to call Display.invalidate() inside its
destructor. The Java Display will be GC'ed when no reference remains...

[Yuwei, 2016-06-28 00:06:57]

On 2016/06/27 23:59:18, Sergey Ulanov wrote:
>
https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
> File remoting/client/jni/jni_display_handler.cc (right):
> 
>
https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_dis...
> remoting/client/jni/jni_display_handler.cc:71: return
> base::android::ScopedJavaLocalRef<jobject>(java_display_);
> On 2016/06/27 23:51:44, Lambros wrote:
> > I'm not sure if this is correct? ScopedJavaLocalRef(foo) takes ownership of
> foo,
> > if I'm not mistaken? Looking at the implementation of the ctor
> > ScopedJavaLocalRef(const U& other), it looks like it takes ownership of
> > other.obj().
> 
> It's a java object, so it cannot be owned. This line just creates a new
> reference to the same object.
> 
> I think it can be replaced with 
>   return java display_;
> 
> The constructor should be called implicitly.

ScopedJavaLocalRef has quite weird constructors:

  ScopedJavaLocalRef(const ScopedJavaLocalRef<T>& other)
      : env_(other.env_) {
    this->SetNewLocalRef(env_, other.obj());
  }

  template<typename U>
  explicit ScopedJavaLocalRef(const U& other)
      : env_(NULL) {
    this->Reset(other);
  }

There is no implicit constructor to convert a global ref to
a (new) local ref...

[Yuwei, 2016-06-28 05:04:36]

Description was changed from

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler very not straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler own the
Java display object instead of the other way around.

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

to

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control the
lifetime of the
Java display object instead of the other way around.

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

[Yuwei, 2016-06-28 05:04:51]

Description was changed from

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control the
lifetime of the
Java display object instead of the other way around.

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

to

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

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

[Lambros, 2016-06-29 00:18:39]

https://codereview.chromium.org/2100943004/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:47: private
void setDisplay(Object display) {
I think you could use Display instead of Object here. Looking elsewhere in
Chromium, I see examples of methods that are @CalledByNative, but they still
specify the proper type of their parameters.
For example:
https://cs.chromium.org/chromium/src/base/android/jni_generator/jni_generator...
I think you'll get a ClassCastException if you try to pass the wrong type of
object from native code.

https://codereview.chromium.org/2100943004/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:56: public
Object getDisplay() {
Even if you needed the instanceof checking, you could still return Display and
put the instanceof check inside here instead of the calling code.

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
remoting/client/jni/jni_client.cc:164: NOTIMPLEMENTED();
This won't compile - raw_display_handler will be undefined.

Maybe just remove the temporary variable, and call
display_handler_.reset(...) here?

[Yuwei, 2016-06-29 00:40:10]

Hi sergeyu@, do you have any comments or concerns about this CL?

https://codereview.chromium.org/2100943004/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:47: private
void setDisplay(Object display) {
On 2016/06/29 00:18:39, Lambros wrote:
> I think you could use Display instead of Object here. Looking elsewhere in
> Chromium, I see examples of methods that are @CalledByNative, but they still
> specify the proper type of their parameters.
> For example:
>
https://cs.chromium.org/chromium/src/base/android/jni_generator/jni_generator...
> I think you'll get a ClassCastException if you try to pass the wrong type of
> object from native code.

As discussed offline,

* The point of passing and storing Object is that different implementations of
DesktopView will expect different type of Display object.
* The Java client will no longer need to access the display object so it doesn't
(and shouldn't) need to know the type information.

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
remoting/client/jni/jni_client.cc:164: NOTIMPLEMENTED();
On 2016/06/29 00:18:39, Lambros wrote:
> This won't compile - raw_display_handler will be undefined.
> 
> Maybe just remove the temporary variable, and call
> display_handler_.reset(...) here?

That's right but I don't think it matters? I would expect
REMOTING_ANDROID_ENABLE_OPENGL_RENDERER to be set after JniGlDisplayHandler is
done...

[Yuwei, 2016-06-30 18:37:00]

Any updates for this CL? O_o

[Sergey Ulanov, 2016-06-30 18:52:51]

lgtm

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
remoting/client/jni/jni_client.cc:164: NOTIMPLEMENTED();
It doesn't matter much, but this code will fail to compile with OpenGL enabled
because raw_display_handler is not defined, so this could be a replaced with
#error.

[Yuwei, 2016-06-30 19:52:02]

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2100943004/diff/1/remoting/client/jni/jni_cli...
remoting/client/jni/jni_client.cc:164: NOTIMPLEMENTED();
On 2016/06/30 18:52:51, Sergey Ulanov wrote:
> It doesn't matter much, but this code will fail to compile with OpenGL enabled
> because raw_display_handler is not defined, so this could be a replaced with
> #error.

Done.

[Yuwei, 2016-06-30 19:52:06]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-06-30 19:52:07]

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/2100943004/#ps20001
(title: "Reviewer's Feedback")

[commit-bot: I haz the power, 2016-06-30 19:52:43]

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

[commit-bot: I haz the power, 2016-06-30 20:37:12]

Description was changed from

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

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

to

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

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

[commit-bot: I haz the power, 2016-06-30 20:37:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-30 20:39:10]

Description was changed from

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

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

to

==========
[Remoting Android] Make JniClient own JniDisplayHandler

Refactorization CL preparing for OpenGL rendering on Android. Currently
JniDisplayHandler is owned and managed by the Java display object, which
requires passing pointer of the display handler back and forth and makes the
lifetime management of the display handler not very straightforward. This CL
makes JniClient own JniDisplayHandler and makes JniDisplayHandler control
the lifetime of the Java display object instead of the other way around.

BUG=385924

Committed: https://crrev.com/6cee11d75bb202e4dab39246837b069af2691404
Cr-Commit-Position: refs/heads/master@{#403272}
==========

[commit-bot: I haz the power, 2016-06-30 20:39:11]

Patchset 2 (id:??) landed as
https://crrev.com/6cee11d75bb202e4dab39246837b069af2691404
Cr-Commit-Position: refs/heads/master@{#403272}