Change Android activity title from JNI call

Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1978843002

Committed: https://skia.googlesource.com/skia/+/36632f46a6d87d9d9bd5e0ab65350638e8cfb65c

[liyuqian, 2016-05-13 14:26:59]

Description was changed from

==========
Change Android activity title from JNI call

BUG=skia:
==========

to

==========
Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[liyuqian, 2016-05-13 14:28:45]

Description was changed from

==========
Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[liyuqian, 2016-05-13 14:28:45]

liyuqian@google.com changed reviewers:
+ djsollen@google.com

[djsollen, 2016-05-13 14:56:27]

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
File
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java
(right):

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:11:
import android.util.Log;
unused import

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:45:
public ViewerActivity getViewerActivity() {
remove unused getter

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:59:
mViewerActivity.setTitle(mTitle);
I would have thought this would be something like...

ActionBar ab = mViewerActivity.getActionBar();
 ab.setTitle("My Title");

also is this not thread safe?  Do you need to post a message to the UI thread to
make that change?

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.cpp:141:
skiaAndroidApp->fJavaVM->AttachCurrentThread(&(skiaAndroidApp->fPThreadEnv),
nullptr);
can you add some comments on why we need to attach the vm to the current thread?

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.h (right):

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:46: // This must be called in
SkiaAndroidApp's own pthread because the JNIEnv is thread sensitive
do we have some way to enforce this behavior?

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:54: void
setAndroidTitle(const char* title) const;
shorten to setTitle

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:57: static void*
pthread_main(void*);
I think you should be able to make these functions private.

[liyuqian, 2016-05-13 15:18:44]

Revised and commented.

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
File
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java
(right):

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:11:
import android.util.Log;
On 2016/05/13 14:56:27, djsollen wrote:
> unused import

Done.

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:45:
public ViewerActivity getViewerActivity() {
On 2016/05/13 14:56:26, djsollen wrote:
> remove unused getter

Done.

https://codereview.chromium.org/1978843002/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:59:
mViewerActivity.setTitle(mTitle);
On 2016/05/13 14:56:27, djsollen wrote:
> I would have thought this would be something like...
> 
> ActionBar ab = mViewerActivity.getActionBar();
>  ab.setTitle("My Title");
> 
> also is this not thread safe?  Do you need to post a message to the UI thread
to
> make that change?

I think mViewerActivity.setTitle is equivalent to
mViewerActivity.getActionBar().setTitle, except that the former is a little
shorter?

The setTitle needs to be run on UI thread because setting ActionBar title from
my native pthread is forbidden (and it will cause exception to crash the Android
app).

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.cpp:141:
skiaAndroidApp->fJavaVM->AttachCurrentThread(&(skiaAndroidApp->fPThreadEnv),
nullptr);
On 2016/05/13 14:56:27, djsollen wrote:
> can you add some comments on why we need to attach the vm to the current
thread?

Done.

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.h (right):

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:46: // This must be called in
SkiaAndroidApp's own pthread because the JNIEnv is thread sensitive
On 2016/05/13 14:56:27, djsollen wrote:
> do we have some way to enforce this behavior?

One way that I can think of is to check the current thread, and throw an
exception if the current thread is not our pthread. This is currently done by
JNI: if you call these methods from another thread, JNI throws an exception
saying that you should only use the JNIEnv in the correct thread.

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:54: void
setAndroidTitle(const char* title) const;
On 2016/05/13 14:56:27, djsollen wrote:
> shorten to setTitle

Done.

https://codereview.chromium.org/1978843002/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.h:57: static void*
pthread_main(void*);
On 2016/05/13 14:56:27, djsollen wrote:
> I think you should be able to make these functions private.

Done.

[liyuqian, 2016-05-13 15:24:25]

Make destructor private

The constructor must be public to allow JNI native call to create a
SkiaAndroidApp.

[djsollen, 2016-05-13 15:35:58]

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/surface_glue_android.cpp:57: jmethodID methodID =
fPThreadEnv->GetMethodID(cls, "setTitle", "(Ljava/lang/String;)V");
these JNI getters can be expensive so lookup the methodID once in the
constructor and stash it is a private variable (e.g. fSetStringMethod)

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/surface_glue_android.cpp:58:
fPThreadEnv->CallVoidMethod(fAndroidApp, methodID,
fPThreadEnv->NewStringUTF(title));
this will leak the string that you just created.

jstring str = fPThreadEnv->NewStringUTF(title);
...
env->DeleteLocalRef(str);

[liyuqian, 2016-05-13 16:01:00]

Thank you for the suggestions :)

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/surface_glue_android.cpp:57: jmethodID methodID =
fPThreadEnv->GetMethodID(cls, "setTitle", "(Ljava/lang/String;)V");
On 2016/05/13 15:35:58, djsollen wrote:
> these JNI getters can be expensive so lookup the methodID once in the
> constructor and stash it is a private variable (e.g. fSetStringMethod)

Done.

https://codereview.chromium.org/1978843002/diff/20001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/surface_glue_android.cpp:58:
fPThreadEnv->CallVoidMethod(fAndroidApp, methodID,
fPThreadEnv->NewStringUTF(title));
On 2016/05/13 15:35:57, djsollen wrote:
> this will leak the string that you just created.
> 
> jstring str = fPThreadEnv->NewStringUTF(title);
> ...
> env->DeleteLocalRef(str);

Done.

[djsollen, 2016-05-13 16:19:12]

lgtm with a few nits.

https://codereview.chromium.org/1978843002/diff/60001/platform_tools/android/...
File
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java
(right):

https://codereview.chromium.org/1978843002/diff/60001/platform_tools/android/...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerApplication.java:49:
mTitle = title;
we don't need to store the title as it we can always retrieve it from the
actionbar.

https://codereview.chromium.org/1978843002/diff/60001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/surface_glue_android.h (right):

https://codereview.chromium.org/1978843002/diff/60001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/surface_glue_android.h:58: jclass fAppClass;
We don't need to hold onto fAppClass

[liyuqian, 2016-05-13 16:25:49]

The CQ bit was checked by liyuqian@google.com

[liyuqian, 2016-05-13 16:25:49]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1978843002/#ps80001
(title: "Nits")

[commit-bot: I haz the power, 2016-05-13 16:25:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978843002/80001

[commit-bot: I haz the power, 2016-05-13 16:57:48]

Description was changed from

==========
Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Change Android activity title from JNI call

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/36632f46a6d87d9d9bd5e0ab65350638e8cfb65c
==========

[commit-bot: I haz the power, 2016-05-13 16:57:48]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/36632f46a6d87d9d9bd5e0ab65350638e8cfb65c