740898 - Merge LayerView and FlexibleGLSurfaceView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Merge LayerView and GLSurfaceView

Once, GLThread is gone we don't have any need to support both styles of GL rendering. Merging LayerView and FlexibleGLSurfaceView makes it clearer what's going on, and I expect we'll also be able to take out some dead code from the resulting LayerView.

This patch is mostly uninteresting. The interesting bits are that implementations of requestRender and the constructors are merged. Everything else is mostly copying over methods and renaming FlexibleGLSurfaceView's mController to mGLController.

Comment 1

[Patrick Walton (:pcwalton)]

Comment on attachment 610949 [details] [diff] [review]
Merge LayerView and GLSurfaceView

Review of attachment 610949 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits addressed.

::: widget/android/AndroidLayerViewWrapper.cpp
@@ +18,5 @@
> +* Portions created by the Initial Developer are Copyright (C) 2011-2012
> +* the Initial Developer. All Rights Reserved.
> +*
> +* Contributor(s):
> +* Patrick Walton <[e-mail]>

I didn't write this file, put your name in :)

@@ +68,5 @@
> +void
> +AndroidGLController::Acquire(JNIEnv* aJEnv, jobject aJObj)
> +{
> +    mJEnv = aJEnv;
> +    mThread = (void*)pthread_self();

I'd use pthread_t and get rid of the void*. (It should be reinterpret_cast<void *> anyway to match the style of the rest of this file.)

::: widget/android/AndroidLayerViewWrapper.h
@@ +18,5 @@
> +* Portions created by the Initial Developer are Copyright (C) 2011-2012
> +* the Initial Developer. All Rights Reserved.
> +*
> +* Contributor(s):
> +* Patrick Walton <[e-mail]>

Ditto.

@@ +41,5 @@
> +#include <jni.h>
> +#include <cassert>
> +#include <cstdlib>
> +#include <pthread.h>
> +#include <android/log.h>

Nit: cassert, cstdlib, and android/log.h aren't needed.

@@ +48,5 @@
> +public:
> +    static void Init(JNIEnv* aJEnv);
> +};
> +
> +typedef void *EGLSurface;

nit: void* EGLSurface, not void *EGLSurface

@@ +54,5 @@
> +class AndroidGLController {
> +public:
> +    static void Init(JNIEnv* aJEnv);
> +
> +    void Acquire(JNIEnv *aJEnv, jobject aJObj);

nit: JNIEnv*, not JNIEnv *

@@ +65,5 @@
> +    static jmethodID jWaitForValidSurfaceMethod;
> +    static jmethodID jProvideEGLSurfaceMethod;
> +
> +    // the JNIEnv for the compositor thread
> +    JNIEnv *mJEnv;

nit: see above

@@ +66,5 @@
> +    static jmethodID jProvideEGLSurfaceMethod;
> +
> +    // the JNIEnv for the compositor thread
> +    JNIEnv *mJEnv;
> +    void *mThread;

Why not pthread_t?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I put this in with the wrong bug number first:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8439284834

Then fixed it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab75de4ad2e

Comment 3

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/fab75de4ad2e