Implement OpenGL backend in Android viewer app

Implement OpenGL backend in Android viewer app

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002

Committed: https://skia.googlesource.com/skia/+/6cb70251829a467b146da9bc8925064f33608e48

[liyuqian, 2016-06-01 18:39:13]

Description was changed from

==========
Temp commit for upload

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

to

==========
Temp commit for upload

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002
==========

[liyuqian, 2016-06-02 15:29:23]

Description was changed from

==========
Temp commit for upload

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002
==========

to

==========
Implement OpenGL backend in Android viewer app

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002
==========

[liyuqian, 2016-06-02 15:39:16]

liyuqian@google.com changed reviewers:
+ jvanverth@google.com, scroggo@google.com

[liyuqian, 2016-06-02 15:39:41]

Leon: Since Derek is on vacation, can you please review my change on the Android
side?

Jim: here's my implementation of GLWindowContext_android. Please check if I
missed something.

Thanks,
Yuqian

[liyuqian, 2016-06-02 15:40:27]

Sorry, I forget to change the title...

[jvanverth1, 2016-06-02 17:17:36]

lgtm + comments

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/GLWindowContext_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/GLWindowContext_android.cpp:52: // The following
initialization is mainly copied from CreatePlatformGLTestContext_egl.cpp
I don't think you need to note this. It's all a very straightforward
initialization path.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/GLWindowContext_android.cpp:89: // Initialization
copied from CreatePlatformGLTestContext_egl.cpp ends.
Same here, I don't think this comment is needed.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:49: if (kVulkan_BackendType !=
attachType && kNativeGL_BackendType != attachType) {
You can probably remove this block -- the conditional always evaluates to false.

[scroggo, 2016-06-02 18:25:17]

On 2016/06/02 15:39:41, liyuqian wrote:
> Leon: Since Derek is on vacation, can you please review my change on the
Android
> side?

I've taken a look, although I'm not familiar with this code. Are there specific
things you want me to look at?

Jim, is there anything that looked Android specific that you don't know and are
counting on me to review?

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:439: // After the change, please call updateUIState no
notify the frontend (e.g., Android app).
to* notify?

Also, who is this comment for? The caller of this method? This seems like the
wrong place for it, if so.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:456: for(int i=0; i<sk_app::Window::kBackendTypeCount;
i++) {
nit: Typically we put spaces around operators:

for (int i = 0; i < ...; i++) {

(also space after "for")

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:57: initDisplay(fNativeWindow);
nit: Skia code calls member functions using "this", e.g.

this->initDisplay(fNativeWindow);

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:60: // we delay the creation of
fWindowContext until Android informs us that
When/where does that happen? Should we return false here?

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:76: default:
Why does default create a vulkan backend?

[scroggo, 2016-06-02 18:25:17]

On 2016/06/02 15:39:41, liyuqian wrote:
> Leon: Since Derek is on vacation, can you please review my change on the
Android
> side?

I've taken a look, although I'm not familiar with this code. Are there specific
things you want me to look at?

Jim, is there anything that looked Android specific that you don't know and are
counting on me to review?

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:439: // After the change, please call updateUIState no
notify the frontend (e.g., Android app).
to* notify?

Also, who is this comment for? The caller of this method? This seems like the
wrong place for it, if so.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:456: for(int i=0; i<sk_app::Window::kBackendTypeCount;
i++) {
nit: Typically we put spaces around operators:

for (int i = 0; i < ...; i++) {

(also space after "for")

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:57: initDisplay(fNativeWindow);
nit: Skia code calls member functions using "this", e.g.

this->initDisplay(fNativeWindow);

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:60: // we delay the creation of
fWindowContext until Android informs us that
When/where does that happen? Should we return false here?

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:76: default:
Why does default create a vulkan backend?

[liyuqian, 2016-06-02 18:47:14]

Thank you for the comments! Please see my answers below.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:439: // After the change, please call updateUIState no
notify the frontend (e.g., Android app).
On 2016/06/02 18:25:16, scroggo wrote:
> to* notify?
> 
> Also, who is this comment for? The caller of this method? This seems like the
> wrong place for it, if so.

This comment is for those who will add more features to onUIStateChanged. For
example, when I add the feature of handling backend state change, I should call
updateUIState (line 464) to tell the frontend to update its UI to reflect the
backend change. I'll update the comment to clarify the audience of this comment.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:456: for(int i=0; i<sk_app::Window::kBackendTypeCount;
i++) {
On 2016/06/02 18:25:16, scroggo wrote:
> nit: Typically we put spaces around operators:
> 
> for (int i = 0; i < ...; i++) {
> 
> (also space after "for")

Done.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/GLWindowContext_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/GLWindowContext_android.cpp:52: // The following
initialization is mainly copied from CreatePlatformGLTestContext_egl.cpp
On 2016/06/02 17:17:36, jvanverth1 wrote:
> I don't think you need to note this. It's all a very straightforward
> initialization path.

Done.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/GLWindowContext_android.cpp:89: // Initialization
copied from CreatePlatformGLTestContext_egl.cpp ends.
On 2016/06/02 17:17:36, jvanverth1 wrote:
> Same here, I don't think this comment is needed.

Done.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:49: if (kVulkan_BackendType !=
attachType && kNativeGL_BackendType != attachType) {
On 2016/06/02 17:17:36, jvanverth1 wrote:
> You can probably remove this block -- the conditional always evaluates to
false.

Done.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:57: initDisplay(fNativeWindow);
On 2016/06/02 18:25:17, scroggo wrote:
> nit: Skia code calls member functions using "this", e.g.
> 
> this->initDisplay(fNativeWindow);

Done.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:60: // we delay the creation of
fWindowContext until Android informs us that
On 2016/06/02 18:25:16, scroggo wrote:
> When/where does that happen? Should we return false here?

It happens in initDisplay, which is initiated by kSurfaceCreated event. Derek
originally wrote this function by returning true so I just followed his design.
I'll update the comment to make it more clear.

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:76: default:
On 2016/06/02 18:25:16, scroggo wrote:
> Why does default create a vulkan backend?

Because that's the first backend in Android viewer app?

[scroggo, 2016-06-02 18:58:37]

lgtm

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
File tools/viewer/sk_app/android/Window_android.cpp (right):

https://codereview.chromium.org/2032623002/diff/40001/tools/viewer/sk_app/and...
tools/viewer/sk_app/android/Window_android.cpp:76: default:
On 2016/06/02 18:47:13, liyuqian wrote:
> On 2016/06/02 18:25:16, scroggo wrote:
> > Why does default create a vulkan backend?
> 
> Because that's the first backend in Android viewer app?

It looks like there are only two enum values. Can you just remove "default"
entirely? That way, if we add another enum, the compiler will catch us if we
forget to add another case.

Another option would be to default to SkASSERT(false); I think you only want to
create a vulkan backend for kVulkan_BackendType

https://codereview.chromium.org/2032623002/diff/80001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2032623002/diff/80001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:465: updateTitle();
nit:
this->updateTitle();
this->updateUIState();

[liyuqian, 2016-06-02 19:01:19]

The CQ bit was checked by liyuqian@google.com

[liyuqian, 2016-06-02 19:01:20]

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

[commit-bot: I haz the power, 2016-06-02 19:01:31]

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

[commit-bot: I haz the power, 2016-06-02 19:16:28]

Description was changed from

==========
Implement OpenGL backend in Android viewer app

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002
==========

to

==========
Implement OpenGL backend in Android viewer app

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2032623002

Committed:
https://skia.googlesource.com/skia/+/6cb70251829a467b146da9bc8925064f33608e48
==========

[commit-bot: I haz the power, 2016-06-02 19:16:33]

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