725095 - (land-maple) [Meta] OMTC: Land Android compositor

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Ali Juma [:ajuma]]

This tracks the work that needs to be completed before we land OMTC for Android.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Maple/Central diff (Not ready to land)

Here's an idea of what merging is going to look like. We're not ready but I'm putting this up so that people can start looking and suggesting changes now.

To get this in parts you'll need to look at the Maple changelog.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Benoit, do you think we can drop the adreno dependency on this bug now that most of the major crashes should be fixed?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

we don't track meta bugs

Comment 5

[Joe Drew (not getting mail)]

Let's not block this bug on meta bugs.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Compositor (GFX)

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Compositor (Layout)

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Compositor (Mobile)

Comment 9

[Benoit Girard (:BenWa)]

Attachment: Compositor (Widget)

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 602990 [details] [diff] [review]
Compositor (Mobile)

Chris, feel free to delegate (parts of this review?) if you want.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 602989 [details] [diff] [review]
Compositor (Layout)

> layout/base/FrameLayerBuilder.cpp
> +   // FIXME: Temporary workaround for bug 681192 and bug 724786. Uncomment this code before review!

Remove the second sentence?

> layout/generic/nsGfxScrollFrame.cpp

  mShouldBuildLayer =
     ((XRE_GetProcessType() == GeckoProcessType_Content || usingDisplayport) &&
     (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN ||
      styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) &&
     (scrollRange.width > 0 ||
      scrollRange.height > 0 || usingDisplayport) &&
     (usingDisplayport || !mIsRoot ||
      !mOuter->PresContext()->IsRootContentDocument()));

Adding 'usingDisplayport' in three places in this expression makes it
hard to read.  Can we write it like this instead:

  mShouldBuildLayer =
    (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN ||
     styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) &&
    (usingDisplayport ||
     (XRE_GetProcessType() == GeckoProcessType_Content &&
      (scrollRange.width > 0 || scrollRange.height > 0) &&
      (!mIsRoot || !mOuter->PresContext()->IsRootContentDocument())));

r=mats with those fixes

Also, I think bug 728983 should block this part from landing.
I don't think we should enable scroll layers for displayport until it
handles CSS z-index correctly.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 602990 [details] [diff] [review]
Compositor (Mobile)

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

r- because I have too many questions. I also think bug 723036 and bug 732564 should block this bug.

::: mobile/android/app/mobile.js
@@ +597,5 @@
>  pref("layers.acceleration.disabled", true);
>  #endif
>  
> +pref("layers.offmainthreadcomposition.enabled", true);
> +pref("layers.acceleration.draw-fps", true);

Do we want this pref to default to true when we merge?

::: mobile/android/base/GeckoApp.java
@@ -569,5 @@
>                  if (!Tabs.getInstance().isSelectedTab(mThumbnailTab))
>                      return;
>  
> -                if (getLayerController().getLayerClient() != mSoftwareLayerClient)
> -                    return;

Why was this removed?

@@ -1745,5 @@
>               * run experience, perhaps?
>               */
>              mLayerController = new LayerController(this);
> -            mPlaceholderLayerClient = PlaceholderLayerClient.createInstance(this);
> -            mLayerController.setLayerClient(mPlaceholderLayerClient);

Why was this removed?

::: mobile/android/base/GeckoEvent.java
@@ +422,5 @@
>          event.mMetaState = tabId;
>          return event;
>      }
> +
> +    public void doCallback(String jsonData) {

A comment as to what this is would be nice.

::: mobile/android/base/gfx/CheckerboardImage.java
@@ +82,5 @@
> +        // XXX We don't handle setting the color to white (-1),
> +        //     there a bug somewhere but I'm not sure where,
> +        //     let's hardcode the color for now to black and come back and fix it.
> +        //mMainColor = color;
> +        mMainColor = 0;

This should be fixed before landing, I can't imagine it'd be that hard.

::: mobile/android/base/gfx/FlexibleGLSurfaceView.java
@@ +46,5 @@
> +import android.view.SurfaceHolder;
> +import android.view.SurfaceView;
> +
> +public class FlexibleGLSurfaceView extends SurfaceView implements SurfaceHolder.Callback {
> +    private static final String LOGTAG = "GeckoFlexibleGLSurfaceView";

There is very little documentation in this class. I would like to see some comments explaining what is going on and why dotted about the place. There is too much assumed knowledge here and the interaction between native/Java isn't obvious.

@@ +185,5 @@
> +            Log.e(LOGTAG, "### registerCxxCompositor point A");
> +            System.out.println("register layer comp");
> +            Log.e(LOGTAG, "### registerCxxCompositor point B");
> +            FlexibleGLSurfaceView flexView = (FlexibleGLSurfaceView)GeckoApp.mAppContext.getLayerController().getView();
> +            Log.e(LOGTAG, "### registerCxxCompositor point C: " + flexView);

Is all the logging here actually about errors? This should be Log.d.

::: mobile/android/base/gfx/GLController.java
@@ +253,5 @@
> +        }
> +    }
> +
> +    // Provides an EGLSurface without assuming ownership of this surface.
> +    private EGLSurface provideEGLSurface() {

I'm not sure if the name of this function really implies what it does. I wouldn't expect it to set mEGLSurface, given the 'provide' text. Maybe this should be changed into a parameter on createEGLSurface?

::: mobile/android/base/gfx/GLThread.java
@@ +135,5 @@
> +            }
> +
> +            mController.swapBuffers();
> +            //if (!mController.swapBuffers() && mController.checkForLostContext()) {
> +            //    doRecreateSurface();

If this isn't necessary, let's remove this and roll doRecreateSurface into RecreateSurfaceMessage.run. Otherwise, add some comments explaining why this is here, why it's commented out, etc.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +195,5 @@
> +            mDrawListener.drawFinished();
> +        }
> +    }
> +
> +    RectF getDisplayPort() {

This isn't the same displayport as is in browser.js. This ought to be dealt with by bug 732564.

@@ +258,5 @@
> +        Log.e(LOGTAG, "### getBufferSize " + size);
> +        return size;
> +    }
> +
> +    public Bitmap getBitmap() {

If it's unused, can we refactor things so that getBufferSize and getBitmap don't exist/are named in a more obvious way?

@@ +328,5 @@
> +            return new ViewportMetrics(mGeckoViewport);
> +        return null;
> +    }
> +
> +    /** This function is invoked by Gecko via JNI; be careful when modifying signature. */

Might be nice to have some blurb somewhere around here in the file, explaining the sequence of events when drawing.

::: mobile/android/base/gfx/Layer.java
@@ +64,3 @@
>          mTransactionLock = new ReentrantLock();
> +        if (size == null) {
> +            mPosition = new Rect(0, 0, 0, 0);

No need for the parameters here, Rect() creates an empty rectangle.

::: mobile/android/base/gfx/LayerController.java
@@ +95,4 @@
>  
>      /* The new color for the checkerboard. */
>      private int mCheckerboardColor;
> +    private boolean mCheckerboardShouldShowChecks = true;

We usually don't initialise non-static class variables like this, but I see it's already crept in below anyway...

@@ -145,5 @@
>      public void setForceRedraw() {
>          mForceRedraw = true;
>      }
>  
> -    public LayerClient getLayerClient()           { return mLayerClient; }

I don't object to the removal, but was there a reason? Is it just unused now? (will it remain so?)

@@ +222,5 @@
>          }
>  
> +        // For rotations, we want the focus point to be at the top left.
> +        boolean rotation = (size.width > oldWidth && size.height < oldHeight) ||
> +                           (size.width < oldWidth && size.height > oldHeight);

this doesn't seem like a fool-proof way of detecting rotation, could we not just check Display.getRotation()?

@@ +365,5 @@
>          if (adjustedViewport.right > pageSize.width) adjustedViewport.right = pageSize.width;
>          if (adjustedViewport.bottom > pageSize.height) adjustedViewport.bottom = pageSize.height;
>  
> +        RectF displayPort = (mLayerClient == null ? new RectF() : mLayerClient.getDisplayPort());
> +        return !displayPort.contains(adjustedViewport);

Seeing as the displayport is wrong here, this will return true too often.

@@ +384,5 @@
>          PointF origin = mViewportMetrics.getOrigin();
>          PointF newPoint = new PointF(origin.x, origin.y);
> +        float zoom = mViewportMetrics.getZoomFactor();
> +        viewPoint.x /= zoom;
> +        viewPoint.y /= zoom;

Is this correct? It sounds like you'd want to get the scale factor between the viewport metrics and the root layer (mViewportMetrics.getZoomFactor()/mRootLayer.getResolution())

If not, care to add some comments here?

::: mobile/android/base/gfx/LayerRenderer.java
@@ +74,5 @@
>  
>  /**
>   * The layer renderer implements the rendering logic for a layer view.
>   */
>  public class LayerRenderer implements GLSurfaceView.Renderer {

I've reviewed this as part of bug 723036.

::: mobile/android/base/gfx/LayerView.java
@@ +92,5 @@
>  
>          setFocusable(true);
>          setFocusableInTouchMode(true);
> +
> +        createGLThread();

Is doing this here going to impact startup time in a way that wouldn't have happened before?

::: mobile/android/base/gfx/NinePatchTileLayer.java
@@ +51,5 @@
>   *
>   * For more information on nine-patch bitmaps, see the following document:
>   *   http://developer.android.com/guide/topics/graphics/2d-graphics.html#nine-patch
>   */
>  public class NinePatchTileLayer extends TileLayer {

I've reviewed this as part of bug 723036.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +55,5 @@
>  
>  /**
>   * Draws a small rect. This is scaled to become a scrollbar.
>   */
>  public class ScrollbarLayer extends TileLayer {

Also, see my comments in bug 723036.

@@ +266,5 @@
>          if (!initialized())
>              return;
>  
> +        // Create the shader program, if necessary
> +        // XXX Can the context's LayerRenderer

erk, seems this line was accidentally left over... Remove.

::: mobile/android/base/gfx/SurfaceTextureLayer.java
@@ +60,1 @@
>  public class SurfaceTextureLayer extends Layer implements SurfaceTexture.OnFrameAvailableListener {

Should this be ported before we land?

::: mobile/android/base/gfx/TileLayer.java
@@ +207,4 @@
>  
>          if (newlyCreated || dirtyRect.contains(bufferRect)) {
> +            GLES20.glTexImage2D(GLES20.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width,
> +                                mSize.height, 0, glInfo.format, glInfo.type, imageBuffer);

This isn't equivalent to what it replaced. If texture size differs from buffer size, this will cause problems.

On the other hand, if with GLES20 we get NPOT textures and we're planning on using them, the distinction between buffer and texture size can be removed entirely.

::: mobile/android/base/gfx/VirtualLayer.java
@@ +63,5 @@
> +
> +        // it is safe to drop the transaction lock in this instance (i.e. for the
> +        // VirtualLayer that is just a shadow of what gecko is painting) because
> +        // the position and resolution of this layer are never used for anything
> +        // meaningful.

hmmm... Maybe a list of what it is actually used for, so that when we run into concurrent access issues, we can look here and say "oh, well X isn't on that list, so this must've changed"?

::: mobile/android/chrome/content/browser.js
@@ +1465,5 @@
> +        frameLoader.renderMode = Ci.nsIFrameLoader.RENDER_MODE_ASYNC_SCROLL;
> +        frameLoader.clampScrollPosition = false;
> +    } else {
> +        // Turn off clipping so we can buffer areas outside of the browser element.
> +        frameLoader.clipSubdocument = false;

This else case seems unnecessary - the XUL document is no longer structured so that it's possible to buffer outside of the browser element.

Can we just assume GL layers? It seems things are very unlikely to work otherwise...

@@ +1677,1 @@
>      // Update the viewport to current dimensions

This comment should be updated.

@@ +1755,5 @@
>          if (target.defaultView != this.browser.contentWindow)
>            return;
>  
> +        // Sample the background color of the page and pass it along. (This is used to draw the
> +        // checkerboard.)

Is it ok to just sample here and not when drawing? Is the latter a speed issue? Can we catch changes in bg-color if we don't check it when drawing?

We may not care that much about this, but a comment should be added, mentioning that changes won't be caught if we don't.

@@ +2209,5 @@
>            ViewportHandler.updateMetadata(this);
>            this.documentIdForCurrentViewport = ViewportHandler.getIdForDocument(contentDocument);
> +          // FIXME: This is a workaround for the fact that we suppress draw events.
> +          // This means we need to retrigger a draw event here since we might
> +          // have suppressed a draw event before documentIdForCurrentViewport

Is there an associated bug number with this FIXME?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 602991 [details] [diff] [review]
Compositor (Widget)

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

didn't get very far with the review, but the flight is boarding.

::: widget/android/AndroidBridge.cpp
@@ +1061,5 @@
> +void
> +AndroidBridge::RegisterCompositor()
> +{
> +    ALOG_BRIDGE("AndroidBridge::RegisterCompositor");
> +    JNIEnv *env = GetJNIForThread();

what thread is this called on?

@@ +1075,5 @@
> +
> +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### Acquire()");
> +    sController.Acquire(env, glController);
> +    sController.SetGLVersion(2);
> +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "Registered Compositor");

please remove this logging. I see other instances, remove it all. If some of it is actually useful, make sure it logs at the right level (this would probably be ANDROID_LOG_VERBOSE).

::: widget/android/AndroidJavaWrappers.h
@@ +160,2 @@
>  public:
> +    virtual void operator()(nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY) = 0;

just a style nit, I 'd rather this be callback() or notifyTransform() than operator(). Mostly because callers make this look like a function pointer.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 602988 [details] [diff] [review]
Compositor (GFX)

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +94,5 @@
> +
> +void
> +CompositorParent::PauseComposition()
> +{
> +  printf_stderr("Pause composition\n");

Should we assert that we're on the compositor thread here?

@@ +148,5 @@
> +#endif
> +
> +  printf_stderr("Schedule composition\n");
> +  mCurrentCompositeTask = NewRunnableMethod(this, &CompositorParent::Composite);
> +  if (!initialComposition && delta.ToMilliseconds() < 15) {

A comment as to why we want to do this would be handy.

@@ +206,5 @@
> +  Layer* root = mLayerManager->GetRoot();
> +
> +  nsTArray<Layer*> queue;
> +  queue.AppendElement(root);
> +  for (unsigned i = 0; i < queue.Length(); i++) {

A while loop might be less confusing here, since the array is modified in the body of the loop; you could pop the first element off. For efficiency you'd need a linked list or deque though.

I don't feel terribly strongly about this, regardless.

@@ +250,5 @@
> +{
> +  return aTransform._11;
> +}
> +
> +static double GetYScale(const gfx3DMatrix& aTransform)

Putting these two in gfx3DMatrix wouldn't go awry.

@@ +279,5 @@
> +    nsIntPoint scrollCompensation(
> +      (mScrollOffset.x / tempScaleDiffX - metricsScrollOffset.x) * mXScale,
> +      (mScrollOffset.y / tempScaleDiffY - metricsScrollOffset.y) * mYScale);
> +    ViewTransform treeTransform(-scrollCompensation, mXScale,
> +                              mYScale);

indentation incorrect

@@ +285,5 @@
> +
> +    shadow->SetShadowTransform(shadowTransform);
> +  } else {
> +    ViewTransform treeTransform(nsIntPoint(0,0), mXScale,
> +                              mYScale);

indentation incorrect

@@ +303,5 @@
> +
> +  Layer* root = mLayerManager->GetRoot();
> +  if (!root) {
> +    return;
> +  }

Is this necessary?

::: gfx/layers/ipc/CompositorParent.h
@@ +42,5 @@
>  #define mozilla_layers_CompositorParent_h
>  
> +// Enable this pref to turn on compositor performance warning.
> +// This will print warnings if the compositor isn't meeting
> +// it's responsiveness objectives:

its

@@ +116,5 @@
>    void ScheduleComposition();
> +  void TransformShadowTree();
> +
> +  // Platform specific functions
> +#ifdef MOZ_WIDGET_ANDROID

I'm not ultra-enthusiastic about the fact that we have this platform-specific code inside CompositorParent. I suspect we will need to do some refactoring when we start doing OMTC in earnest.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 602991 [details] [diff] [review]
Compositor (Widget)

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

mostly clean up stuff, but enough of them that I'd like to see another patch

::: widget/android/AndroidBridge.cpp
@@ +1859,5 @@
> +AndroidBridge::ScheduleComposite()
> +{
> +    if (mCompositorParent) {
> +        mCompositorParent->ScheduleRenderOnCompositorThread(*mCompositorThread);
> +    }

the AndroidBridge is for making calls into java. nsWindow could hold the reference to the compositor parent and thread statically and have static methods for operations on it.

@@ +1886,5 @@
> +    mViewTransformGetter = &aViewTransformGetter;
> +}
> +
> +void
> +AndroidBridge::GetViewTransform(nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY)

I don't see this getting called in this patch, but I suspect it shouldn't be in the bridge either

::: widget/android/AndroidFlexViewWrapper.cpp
@@ +39,5 @@
> +
> +
> +static AndroidGLController sController;
> +
> +static const char *sEGLDisplayClassName = "com/google/android/gles_jni/EGLDisplayImpl";

const char const* k_____ for all of these, or just a #define since they're only used once

or just use the string literals <--- this is my preference

just be consistent.

@@ +104,5 @@
> +                                              "()Ljavax/microedition/khronos/egl/EGLContext;");
> +    jGetEGLSurfaceMethod = aJEnv->GetMethodID(jClass, "getEGLSurface",
> +                                              "()Ljavax/microedition/khronos/egl/EGLSurface;");
> +    jProvideEGLSurfaceMethod = aJEnv->GetMethodID(jClass, "provideEGLSurface",
> +                                                  "()Ljavax/microedition/khronos/egl/EGLSurface;");

again, stay consistent

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

since you're saving the JNIEnv, we need to be sure that all methods on this class are called on the same thread. A debug assertion would probably be a good idea

@@ +121,5 @@
> +    mJObj = aJEnv->NewGlobalRef(aJObj);
> +}
> +
> +void
> +AndroidGLController::Acquire(JNIEnv* aJEnv)

I don't see this ever get called, drop

::: widget/android/AndroidFlexViewWrapper.h
@@ +76,5 @@
> +public:
> +    static void Init(JNIEnv* aJEnv);
> +
> +    void Acquire(JNIEnv *aJEnv, jobject aJObj);
> +    void Acquire(JNIEnv *aJEnv);

I don't see this get called, drop

I didn't check the rest of the methods on this class, but any others aren't used they should be dropped as well

::: widget/android/AndroidJNI.cpp
@@ +896,3 @@
>  {
> +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### scheduleComposite()");
> +    AndroidBridge::Bridge()->ScheduleComposite();

as noted earlier, this shouldn't be calling into the android bridge

::: widget/android/AndroidJavaWrappers.cpp
@@ +691,5 @@
> +    if (!isNull()) {
> +        Dispose();
> +    }
> +
> +    wrapped_obj = GetJNIForThread()->NewGlobalRef(jobj);

we seem to be getting and releasing the global ref on the wrapped_obj in several subclasses of WrappedJavaObject. It would be better to do it in WrappedJavaObject.

@@ +825,5 @@
> +    return GetJNIForThread()->CallObjectMethod(wrapped_obj, jGetHolderMethod);
> +}
> +
> +void
> +AndroidGeckoLayerClient::GetViewTransform(AndroidViewTransform& aViewTransform)

the only place I see this called is in the transform getter, so if that really isn't called, this can be dropped as well.

::: widget/android/nsWindow.cpp
@@ +1177,5 @@
> +    // BEGIN HACK: gl layers
> +    nsPaintEvent event(true, NS_PAINT, this);
> +    nsIntRect tileRect(0, 0, gAndroidBounds.width, gAndroidBounds.height);
> +    event.region = tileRect;
> +#endif

delete

@@ +1195,5 @@
> +    status = DispatchEvent(&event);
> +
> +    return;
> +    // END HACK: gl layers
> +#endif

delete

::: widget/nsIWidget.h
@@ +1090,5 @@
> +     *
> +     * @param aManager The drawing LayerManager.
> +     * @param aWidgetRect The current widget rect that is being drawn.
> +     */
> +    virtual void DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect) = 0;

I think roc needs to review this change

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #12)
> > +pref("layers.offmainthreadcomposition.enabled", true);
> > +pref("layers.acceleration.draw-fps", true);
> 
> Do we want this pref to default to true when we merge?
> 

No, we should turn off the fps layer, I think.

> > -                if (getLayerController().getLayerClient() != mSoftwareLayerClient)
> > -                    return;
> 
> Why was this removed?
> 

Removed in 2d8b1fecdc95. Previously getLayerClient() could return either a PlaceholderLayerClient or a GeckoSoftwareLayerClient. In the new code, mLayerClient is used directly (and can never be a PlaceholderLayerClient), and is compared to null prior to use. In the cases where before we would get the placeholder, we now bail out because mLayerClient is null. Note that the LayerClient base class itself was removed, PlaceholderLayerClient and GeckoLayerClient no longer share a base class, and the getLayerClient() method was removed.

> >              mLayerController = new LayerController(this);
> > -            mPlaceholderLayerClient = PlaceholderLayerClient.createInstance(this);
> > -            mLayerController.setLayerClient(mPlaceholderLayerClient);
> 
> Why was this removed?
> 

Code refactoring in cd5baf17f079. The line of code that replaces this is:
mPlaceholderLayerClient = new PlaceholderLayerClient(mLayerController, mLastViewport);
and the PlaceholderLayerClient constructor has been modified to set things directly on the layer controller as needed.

> >      }
> > +
> > +    public void doCallback(String jsonData) {
> 
> A comment as to what this is would be nice.
> 

This will probably be taken out; it was added in c1161ca4bf13 upon Ehsan's request, and part of that patch was later backed out in 9b10f7e487a8. The remaining pieces should also be backed out, I think. (Related: bug 733896)

> ::: mobile/android/base/gfx/CheckerboardImage.java
> @@ +82,5 @@
> > +        // XXX We don't handle setting the color to white (-1),
> > +        //     there a bug somewhere but I'm not sure where,
> > +        //     let's hardcode the color for now to black and come back and fix it.
> > +        //mMainColor = color;
> > +        mMainColor = 0;
> 
> This should be fixed before landing, I can't imagine it'd be that hard.
> 

BenWa looked at it, and I did as well briefly, and it was non-obvious what the problem was. I've filed bug 734003 to track it; we can make that block maple landing if needed.

> ::: mobile/android/base/gfx/FlexibleGLSurfaceView.java
> @@ +46,5 @@
> > +import android.view.SurfaceHolder;
> > +import android.view.SurfaceView;
> > +
> > +public class FlexibleGLSurfaceView extends SurfaceView implements SurfaceHolder.Callback {
> > +    private static final String LOGTAG = "GeckoFlexibleGLSurfaceView";
> 
> There is very little documentation in this class. I would like to see some
> comments explaining what is going on and why dotted about the place. There
> is too much assumed knowledge here and the interaction between native/Java
> isn't obvious.

I will add some.

> > +            FlexibleGLSurfaceView flexView = (FlexibleGLSurfaceView)GeckoApp.mAppContext.getLayerController().getView();
> > +            Log.e(LOGTAG, "### registerCxxCompositor point C: " + flexView);
> 
> Is all the logging here actually about errors? This should be Log.d.

These were taken out already (e8c2aa855b23).

> > +    // Provides an EGLSurface without assuming ownership of this surface.
> > +    private EGLSurface provideEGLSurface() {
> 
> I'm not sure if the name of this function really implies what it does. I
> wouldn't expect it to set mEGLSurface, given the 'provide' text. Maybe this
> should be changed into a parameter on createEGLSurface?

Ok, I'll do some refactoring on this code. There's definitely unnecessary duplication.

> > +            //if (!mController.swapBuffers() && mController.checkForLostContext()) {
> > +            //    doRecreateSurface();
> 
> If this isn't necessary, let's remove this and roll doRecreateSurface into
> RecreateSurfaceMessage.run. Otherwise, add some comments explaining why this
> is here, why it's commented out, etc.

I'll check with pcwalton about this, but what you're suggesting seems reasonable enough.

> > +
> > +    RectF getDisplayPort() {
> 
> This isn't the same displayport as is in browser.js. This ought to be dealt
> with by bug 732564.
> 

And it is indeed dealt with in bug 732564. :) The displayport here is going to be a subregion of browser.js, except possibly leaking outside the page bounds. Regardless, it's confusing and bad, and bug 732564 makes it better by moving all the browser.js displayport calculations into Java.

> @@ +258,5 @@
> > +        Log.e(LOGTAG, "### getBufferSize " + size);
> > +        return size;
> > +    }
> > +
> > +    public Bitmap getBitmap() {
> 
> If it's unused, can we refactor things so that getBufferSize and getBitmap
> don't exist/are named in a more obvious way?

getBufferSize should be easy to deal with, as nobody outside the class calls it. getBitmap is called by GeckoApp and I don't know what's going to happen with that. I think for now we should leave it as returning null and let bug 725120 cover it.

> > +
> > +    /** This function is invoked by Gecko via JNI; be careful when modifying signature. */
> 
> Might be nice to have some blurb somewhere around here in the file,
> explaining the sequence of events when drawing.
> 

Ok, I will add that.

> > +            mPosition = new Rect(0, 0, 0, 0);
> 
> No need for the parameters here, Rect() creates an empty rectangle.

I will fix that.

> >      private int mCheckerboardColor;
> > +    private boolean mCheckerboardShouldShowChecks = true;
> 
> We usually don't initialise non-static class variables like this, but I see
> it's already crept in below anyway...

I can move this assignment into the constructor.

> >  
> > -    public LayerClient getLayerClient()           { return mLayerClient; }
> 
> I don't object to the removal, but was there a reason? Is it just unused
> now? (will it remain so?)

Yeah, it is unused and should remain so. Ideally I would like to have only one public class in this package so that the rest of the code can't just keep grabbing internals and doing whatever they want, this was a way to get there incrementally. And anyway, in this case, GeckoApp keeps a instance variable that points to the layer client so anybody can get it from there.

> >  
> > +        // For rotations, we want the focus point to be at the top left.
> > +        boolean rotation = (size.width > oldWidth && size.height < oldHeight) ||
> > +                           (size.width < oldWidth && size.height > oldHeight);
> 
> this doesn't seem like a fool-proof way of detecting rotation, could we not
> just check Display.getRotation()?

I ended up taking this code out as part of bug 732564 (part 14). If we're keeping the focus on the top-left there's no need to do anything at all except adjust the zoom level because gecko will keep the scroll position (top-left corner) in the same spot. Related: bug 715898, bug 732428.

> > +        RectF displayPort = (mLayerClient == null ? new RectF() : mLayerClient.getDisplayPort());
> > +        return !displayPort.contains(adjustedViewport);
> 
> Seeing as the displayport is wrong here, this will return true too often.
> 

Agreed. Fixed in bug 732564.

> @@ +384,5 @@
> >          PointF origin = mViewportMetrics.getOrigin();
> >          PointF newPoint = new PointF(origin.x, origin.y);
> > +        float zoom = mViewportMetrics.getZoomFactor();
> > +        viewPoint.x /= zoom;
> > +        viewPoint.y /= zoom;
> 
> Is this correct? It sounds like you'd want to get the scale factor between
> the viewport metrics and the root layer
> (mViewportMetrics.getZoomFactor()/mRootLayer.getResolution())
> 
> If not, care to add some comments here?

Ehsan added this code in be009e74d30f and I hadn't noticed any problems with it so I left it as-is, but it did seem kind of wrong to me as well. I'm not sure who receives the touch events and what coordinate system they expect the events to be in; I will find out and update this as necessary or add a comment.

> >          setFocusable(true);
> >          setFocusableInTouchMode(true);
> > +
> > +        createGLThread();
> 
> Is doing this here going to impact startup time in a way that wouldn't have
> happened before?
> 

I doubt it would impact startup time that much. The thread creation shouldn't be that expensive and it doesn't do anything but sit there waiting for events initially.

> > +        // Create the shader program, if necessary
> > +        // XXX Can the context's LayerRenderer
> 
> erk, seems this line was accidentally left over... Remove.

Will do.

> @@ +60,1 @@
> >  public class SurfaceTextureLayer extends Layer implements SurfaceTexture.OnFrameAvailableListener {
> 
> Should this be ported before we land?
> 

Not sure. snorp said he had a bunch of flash stuff ready to land and we agreed he should wait until I landed bug 732564 (to pick up some of my setResolution changes) or this Friday, whichever happened first. I don't know the nature of his patches but I imagine it will deal with this.

> ::: mobile/android/base/gfx/TileLayer.java
> @@ +207,4 @@
> >  
> >          if (newlyCreated || dirtyRect.contains(bufferRect)) {
> > +            GLES20.glTexImage2D(GLES20.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width,
> > +                                mSize.height, 0, glInfo.format, glInfo.type, imageBuffer);
> 
> This isn't equivalent to what it replaced. If texture size differs from
> buffer size, this will cause problems.
> 
> On the other hand, if with GLES20 we get NPOT textures and we're planning on
> using them, the distinction between buffer and texture size can be removed
> entirely.

Change from 0c53bf892a3c; I'll let BenWa answer this.

> > +        // it is safe to drop the transaction lock in this instance (i.e. for the
> > +        // VirtualLayer that is just a shadow of what gecko is painting) because
> > +        // the position and resolution of this layer are never used for anything
> > +        // meaningful.
> 
> hmmm... Maybe a list of what it is actually used for, so that when we run
> into concurrent access issues, we can look here and say "oh, well X isn't on
> that list, so this must've changed"?

Ok, will do.

> > +    } else {
> > +        // Turn off clipping so we can buffer areas outside of the browser element.
> > +        frameLoader.clipSubdocument = false;
> 
> This else case seems unnecessary - the XUL document is no longer structured
> so that it's possible to buffer outside of the browser element.
> 
> Can we just assume GL layers? It seems things are very unlikely to work
> otherwise...

Yeah, I'll take out the kUsingGLLayers variable and the else clause.

> 
> @@ +1677,1 @@
> >      // Update the viewport to current dimensions
> 
> This comment should be updated.
> 

I remove the comment as part of bug 732564.

> @@ +1755,5 @@
> >          if (target.defaultView != this.browser.contentWindow)
> >            return;
> >  
> > +        // Sample the background color of the page and pass it along. (This is used to draw the
> > +        // checkerboard.)
> 
> Is it ok to just sample here and not when drawing? Is the latter a speed
> issue? Can we catch changes in bg-color if we don't check it when drawing?
> 
> We may not care that much about this, but a comment should be added,
> mentioning that changes won't be caught if we don't.

Ok, I'll add a comment to that effect.

> >            ViewportHandler.updateMetadata(this);
> >            this.documentIdForCurrentViewport = ViewportHandler.getIdForDocument(contentDocument);
> > +          // FIXME: This is a workaround for the fact that we suppress draw events.
> > +          // This means we need to retrigger a draw event here since we might
> > +          // have suppressed a draw event before documentIdForCurrentViewport
> 
> Is there an associated bug number with this FIXME?

Not that I'm aware of. This code is removed as part of bug 732564.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (:kats) from comment #16)
> > >      }
> > > +
> > > +    public void doCallback(String jsonData) {
> > 
> > A comment as to what this is would be nice.
> > 
> 
> This will probably be taken out; it was added in c1161ca4bf13 upon Ehsan's
> request, and part of that patch was later backed out in 9b10f7e487a8. The
> remaining pieces should also be backed out, I think. (Related: bug 733896)
> 

Correction: it was added in c1161ca4bf13, then removed in 9b10f7e487a8. Then added back in 75d48305a2ef, and removed again in 6cf00c870da2. Then added back in b520f34d78f1 with a follow-up to fix bustage in 4f5bd60be09e. And *then* 580381e2805c backed out part of it, leaving behind this doCallback junk. I have a patch to back out the rest, and will post it on bug 733896 once it is tested.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Compositor (Mobile supplemental)

This is an incremental patch that addresses most of the review comments on the "Mobile" patch. I also uploaded a patch to bug 733896 to get rid of the event callback stuff. I have not yet addressed the provideEGLSurface and doRecreateSurface comments (need to talk to pcwalton about that). Also I noticed that setPositionAndResolution may already be invalid because we're now using the bounds of the virtual layer when determining where to draw checkerboard. So I added a comment to that effect and filed bug 734800 for it.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Compositor (Widget supplemental) - Simplify JNI for GetViewTransform

(In reply to Brad Lassey [:blassey] from comment #13)
> ::: widget/android/AndroidBridge.cpp
> @@ +1061,5 @@
> > +void
> > +AndroidBridge::RegisterCompositor()
> > +{
> > +    ALOG_BRIDGE("AndroidBridge::RegisterCompositor");
> > +    JNIEnv *env = GetJNIForThread();
> 
> what thread is this called on?
> 

I believe it is called from the compositor thread.

> @@ +1075,5 @@
> > +
> > +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### Acquire()");
> > +    sController.Acquire(env, glController);
> > +    sController.SetGLVersion(2);
> > +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "Registered Compositor");
> 
> please remove this logging. I see other instances, remove it all. If some of
> it is actually useful, make sure it logs at the right level (this would
> probably be ANDROID_LOG_VERBOSE).
> 

This has been taken out with subsequent patches on maple.

> ::: widget/android/AndroidJavaWrappers.h
> @@ +160,2 @@
> >  public:
> > +    virtual void operator()(nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY) = 0;
> 
> just a style nit, I 'd rather this be callback() or notifyTransform() than
> operator(). Mostly because callers make this look like a function pointer.

Patch attached that gets rid of the unnecessarily convoluted code required to invoke GetViewTransform. It is now just a simple JNI call without operator overloading and function pointers, the same as SetFirstPaintViewport and SetPageSize. (This patch applies on top of the "Compositor (Widget)" patch).

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 604836 [details] [diff] [review]
Compositor (Widget supplemental) - Simplify JNI for GetViewTransform

Unrequesting review; as per IRC discussion I will request a review on a combined widget patch that addresses all of the review comments, as opposed to supplemental patches on top of the base patch.

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 604835 [details] [diff] [review]
Compositor (Mobile supplemental)

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

Very nice, r+ with the two comments addressed.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +108,5 @@
>      }
>  
>      /** This function is invoked by Gecko via JNI; be careful when modifying signature. */
>      public boolean beginDrawing(int width, int height, String metadata) {
>          // If we've changed surface types, cancel this draw

Should this comment still be here?

@@ +306,5 @@
> +      * The compositor invokes this function just before compositing a frame where the document
> +      * is different from the document composited on the last frame. In these cases, the viewport
> +      * information we have in Java is no longer valid and needs to be replaced with the new
> +      * viewport information provided. setPageSize will never be invoked on the same frame that
> +      * this function is invoked on; and this function will always be called prior to getViewTranform.

s/getViewTranform/getViewTransform/ (missing s)

Comment 22

[Ali Juma [:ajuma]]

Attachment: Address GFX review comments

This patch addresses the review comments for the GFX patch.

(In reply to Joe Drew (:JOEDREW!) from comment #14)
> Comment on attachment 602988 [details] [diff] [review]
> Compositor (GFX)
> 
> Review of attachment 602988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +94,5 @@
> > +
> > +void
> > +CompositorParent::PauseComposition()
> > +{
> > +  printf_stderr("Pause composition\n");
> 
> Should we assert that we're on the compositor thread here?

Yes (and so should ResumeComposition() and Composite()). Doing this means that CompositorParent needs to have a pointer for the compositor thread, so I've added that. But then SchedulePauseOnCompositorThread() and ScheduleResumeOnCompositorThread() don't need to take the compositor thread as an argument, so they no longer do.


> @@ +148,5 @@
> > +#endif
> > +
> > +  printf_stderr("Schedule composition\n");
> > +  mCurrentCompositeTask = NewRunnableMethod(this, &CompositorParent::Composite);
> > +  if (!initialComposition && delta.ToMilliseconds() < 15) {
> 
> A comment as to why we want to do this would be handy.

Done.


> @@ +206,5 @@
> > +  Layer* root = mLayerManager->GetRoot();
> > +
> > +  nsTArray<Layer*> queue;
> > +  queue.AppendElement(root);
> > +  for (unsigned i = 0; i < queue.Length(); i++) {
> 
> A while loop might be less confusing here, since the array is modified in
> the body of the loop; you could pop the first element off. For efficiency
> you'd need a linked list or deque though.
> 
> I don't feel terribly strongly about this, regardless.

I've replaced it with a while loop. We can do something fancier if this turns out to be a performance issue, but I don't expect it will be.


> @@ +250,5 @@
> > +{
> > +  return aTransform._11;
> > +}
> > +
> > +static double GetYScale(const gfx3DMatrix& aTransform)
> 
> Putting these two in gfx3DMatrix wouldn't go awry.

Done.


> @@ +279,5 @@
> > +    nsIntPoint scrollCompensation(
> > +      (mScrollOffset.x / tempScaleDiffX - metricsScrollOffset.x) * mXScale,
> > +      (mScrollOffset.y / tempScaleDiffY - metricsScrollOffset.y) * mYScale);
> > +    ViewTransform treeTransform(-scrollCompensation, mXScale,
> > +                              mYScale);
> 
> indentation incorrect

Fixed.


> @@ +285,5 @@
> > +
> > +    shadow->SetShadowTransform(shadowTransform);
> > +  } else {
> > +    ViewTransform treeTransform(nsIntPoint(0,0), mXScale,
> > +                              mYScale);
> 
> indentation incorrect

Fixed.


> @@ +303,5 @@
> > +
> > +  Layer* root = mLayerManager->GetRoot();
> > +  if (!root) {
> > +    return;
> > +  }
> 
> Is this necessary?

No. In fact, that entire method (AsyncRender()) is unnecessary, since the only useful thing it does is call ScheduleComposition(). We now just use ScheduleComposition() directly instead.

 
> ::: gfx/layers/ipc/CompositorParent.h
> @@ +42,5 @@
> >  #define mozilla_layers_CompositorParent_h
> >  
> > +// Enable this pref to turn on compositor performance warning.
> > +// This will print warnings if the compositor isn't meeting
> > +// it's responsiveness objectives:
> 
> its

Fixed.

Comment 23

[Ali Juma [:ajuma]]

Landed the patch addressing GFX review comments:
https://hg.mozilla.org/projects/maple/rev/306e3275b774

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Compositor (Mobile supplemental)

Update the supplemental patch to address Cwiiis' review comments on GLController and GLThread. I removed some commented out code in GLThread, and modified provideEGLSurface in GLController to not store the returned surface in mEGLSurface. (Yesterday I thought that didn't work but it turned out to be interference from d2340d74fc19). Cwiiis looked at the incremental diff on IRC, so carrying r+.

Comment 25

[Ali Juma [:ajuma]]

Attachment: Address Layout review comments

This implements the two fixes requested by mats in Comment 11. Carrying r=mats from there.

Comment 26

[Ali Juma [:ajuma]]

Landed the patch addressing Layout review comments:
https://hg.mozilla.org/projects/maple/rev/86b8fd1b875b

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Landed the patch addressing mobile review comments:
https://hg.mozilla.org/projects/maple/rev/0742dd20781e

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 602991 [details] [diff] [review]
Compositor (Widget)

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

::: widget/nsIWidget.h
@@ +1090,5 @@
> +     *
> +     * @param aManager The drawing LayerManager.
> +     * @param aWidgetRect The current widget rect that is being drawn.
> +     */
> +    virtual void DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect) = 0;

roc was happy in theory with a DrawUnder/DrawOver in bug 595180, and I reviewed that code. I'm going to say that we don't need his review here.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Compositor (Widget supplemental)

This attachment is just the incremental diff of everything that needs to land on maple to address all of your review comments. I'll upload a new mc <-> maple diff of the widget code shortly.

(In reply to Brad Lassey [:blassey] from comment #13)
> ::: widget/android/AndroidBridge.cpp
> @@ +1061,5 @@
> > +void
> > +AndroidBridge::RegisterCompositor()
> > +{
> > +    ALOG_BRIDGE("AndroidBridge::RegisterCompositor");
> > +    JNIEnv *env = GetJNIForThread();
> 
> what thread is this called on?
> 

The compositor thread. Added a comment to that effect.

> @@ +1075,5 @@
> > +
> > +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### Acquire()");
> > +    sController.Acquire(env, glController);
> > +    sController.SetGLVersion(2);
> > +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "Registered Compositor");
> 
> please remove this logging. I see other instances, remove it all. If some of
> it is actually useful, make sure it logs at the right level (this would
> probably be ANDROID_LOG_VERBOSE).
> 

Done.

> ::: widget/android/AndroidJavaWrappers.h
> @@ +160,2 @@
> >  public:
> > +    virtual void operator()(nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY) = 0;
> 
> just a style nit, I 'd rather this be callback() or notifyTransform() than
> operator(). Mostly because callers make this look like a function pointer.

I removed all the indirection and operator overloading around the view transform getter so now it's much simpler.

(In reply to Brad Lassey [:blassey] from comment #15)
> ::: widget/android/AndroidBridge.cpp
> @@ +1859,5 @@
> > +AndroidBridge::ScheduleComposite()
> > +{
> > +    if (mCompositorParent) {
> > +        mCompositorParent->ScheduleRenderOnCompositorThread(*mCompositorThread);
> > +    }
> 
> the AndroidBridge is for making calls into java. nsWindow could hold the
> reference to the compositor parent and thread statically and have static
> methods for operations on it.
> 

Done; I moved those fields into nsWindow and redirected calls as needed.

> @@ +1886,5 @@
> > +    mViewTransformGetter = &aViewTransformGetter;
> > +}
> > +
> > +void
> > +AndroidBridge::GetViewTransform(nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY)
> 
> I don't see this getting called in this patch, but I suspect it shouldn't be
> in the bridge either

GetViewTransform (which has been modified since the original version of this patch) gets called directly from the compositor on the compositor thread (in gfx/layers/ipc/CompositorParent.cpp). SetFirstPaintViewport and SetPageSize are also called in this way.

> ::: widget/android/AndroidFlexViewWrapper.cpp
> @@ +39,5 @@
> > +
> > +
> > +static AndroidGLController sController;
> > +
> > +static const char *sEGLDisplayClassName = "com/google/android/gles_jni/EGLDisplayImpl";
> 
> const char const* k_____ for all of these, or just a #define since they're
> only used once
> 
> or just use the string literals <--- this is my preference
> 
> just be consistent.

Much of this code was unneeded; removed large parts of it and inlined the string literals for the remainder.

> @@ +104,5 @@
> > +                                              "()Ljavax/microedition/khronos/egl/EGLContext;");
> > +    jGetEGLSurfaceMethod = aJEnv->GetMethodID(jClass, "getEGLSurface",
> > +                                              "()Ljavax/microedition/khronos/egl/EGLSurface;");
> > +    jProvideEGLSurfaceMethod = aJEnv->GetMethodID(jClass, "provideEGLSurface",
> > +                                                  "()Ljavax/microedition/khronos/egl/EGLSurface;");
> 
> again, stay consistent

Done.

> 
> @@ +116,5 @@
> > +
> > +void
> > +AndroidGLController::Acquire(JNIEnv* aJEnv, jobject aJObj)
> > +{
> > +    mJEnv = aJEnv;
> 
> since you're saving the JNIEnv, we need to be sure that all methods on this
> class are called on the same thread. A debug assertion would probably be a
> good idea

Done.

> @@ +121,5 @@
> > +    mJObj = aJEnv->NewGlobalRef(aJObj);
> > +}
> > +
> > +void
> > +AndroidGLController::Acquire(JNIEnv* aJEnv)
> 
> I don't see this ever get called, drop
> 

Done.

> ::: widget/android/AndroidFlexViewWrapper.h
> @@ +76,5 @@
> > +public:
> > +    static void Init(JNIEnv* aJEnv);
> > +
> > +    void Acquire(JNIEnv *aJEnv, jobject aJObj);
> > +    void Acquire(JNIEnv *aJEnv);
> 
> I don't see this get called, drop
> 
> I didn't check the rest of the methods on this class, but any others aren't
> used they should be dropped as well

Done, a lot of them were unused and dropped.

> 
> ::: widget/android/AndroidJNI.cpp
> @@ +896,3 @@
> >  {
> > +    __android_log_print(ANDROID_LOG_ERROR, "Gecko", "### scheduleComposite()");
> > +    AndroidBridge::Bridge()->ScheduleComposite();
> 
> as noted earlier, this shouldn't be calling into the android bridge
> 

This now calls into nsWindow.

> ::: widget/android/AndroidJavaWrappers.cpp
> @@ +691,5 @@
> > +    if (!isNull()) {
> > +        Dispose();
> > +    }
> > +
> > +    wrapped_obj = GetJNIForThread()->NewGlobalRef(jobj);
> 
> we seem to be getting and releasing the global ref on the wrapped_obj in
> several subclasses of WrappedJavaObject. It would be better to do it in
> WrappedJavaObject.

This is the only instance left where we do this, so I left it as is. (The other place was the GeckoEvent which was backed out).

> @@ +825,5 @@
> > +    return GetJNIForThread()->CallObjectMethod(wrapped_obj, jGetHolderMethod);
> > +}
> > +
> > +void
> > +AndroidGeckoLayerClient::GetViewTransform(AndroidViewTransform& aViewTransform)
> 
> the only place I see this called is in the transform getter, so if that
> really isn't called, this can be dropped as well.

Rearranged this code as mentioned above.

> ::: widget/android/nsWindow.cpp
> @@ +1177,5 @@
> > +    // BEGIN HACK: gl layers
> > +    nsPaintEvent event(true, NS_PAINT, this);
> > +    nsIntRect tileRect(0, 0, gAndroidBounds.width, gAndroidBounds.height);
> > +    event.region = tileRect;
> > +#endif
> 
> delete
> 
> @@ +1195,5 @@
> > +    status = DispatchEvent(&event);
> > +
> > +    return;
> > +    // END HACK: gl layers
> > +#endif
> 
> delete

Done.

> ::: widget/nsIWidget.h
> @@ +1090,5 @@
> > +     *
> > +     * @param aManager The drawing LayerManager.
> > +     * @param aWidgetRect The current widget rect that is being drawn.
> > +     */
> > +    virtual void DrawWindowUnderlay(LayerManager* aManager, nsIntRect aRect) = 0;
> 
> I think roc needs to review this change

joe r+'d it above.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Compositor (Widget + supplemental)

This patch was generated by
hg diff -r d87ad51531b5 -r tip widget/

where "tip" is maple/rev/70fe7536c40c + the supplemental changes in attachment 605557 [details] [diff] [review]. See previous comment for a description of how the review comments were addressed.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 605559 [details] [diff] [review]
Compositor (Widget + supplemental)

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

::: widget/android/AndroidFlexViewWrapper.cpp
@@ +72,5 @@
> +
> +void
> +AndroidGLController::SetGLVersion(int aVersion)
> +{
> +    NS_ASSERTION((void*)pthread_self() == mThread, "Something is calling AndroidGLController::SetGLVersion from the wrong thread!");

rather than repeat this over and over, make a macro:

#define ASSERT_THREAD() NS_ASSERTION((void*)pthread_self() == mThread, "Something is calling " ## __PRETTY_FUNCTION__ ## " from the wrong thread!");

Comment 32

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] from comment #31)
> rather than repeat this over and over, make a macro:
> 
> #define ASSERT_THREAD() NS_ASSERTION((void*)pthread_self() == mThread,
> "Something is calling " ## __PRETTY_FUNCTION__ ## " from the wrong thread!");

This didn't work because apparently in recent GCCs __PRETTY_FUNCTION__ is a variable and not a string constant. The only way to embed it into an existing string is to do some sort of sprintf thing, which I figured was too much work. Seeing as NS_ASSERTION already prints the file/line number, I just took the function name out of your above macro and used that.

https://hg.mozilla.org/projects/maple/rev/8e11e5b9c935

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Merged maple into m-c:

https://hg.mozilla.org/mozilla-central/rev/936ef50fa498