746016 - Cache low res version of the page in the java ui for use instead of checkerboarding

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[JP Rosevear [:jpr]]

Cache low res version of the page in the java ui for use instead of checkerboarding.

Comment 1

[Aaron Train [:aaronmt]]

Sounds awfully like bug 743751

Comment 2

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

Attachment: WIP patch

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Sounds awfully like bug 743751

It is, but we treated that one as talking about a specific method, and this is a different method - I'll retitle that to reflect this.

Comment 4

[JP Rosevear [:jpr]]

At least a beta blocker to flesh this out and figure out any bugs with the implementation.

Comment 5

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

Attachment: WIP patch v2

Did some more work on this. The image is now limited to a max of 2Mb (though this is apparently still too small for planet). Also, it allocates that 2Mb buffer once and reuses it now. Finally, it frees the buffer in the finalize method rather than simply leak it.

Comment 6

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

Attachment: patch to add MozAfterPaint listener

according to the logging, this is adding the listener just fine. But I'm not getting any logging about paints happening.

If anyone has any ideas as to why, I'd appreciate hearing them.

Comment 7

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

Comment on attachment 617227 [details] [diff] [review]
patch to add MozAfterPaint listener

>+        case PAINT_LISTEN_START_EVENT: {
>+            mMetaState = jenv->GetIntField(jobj, jMetaStateField);
>+        }
>+

This needs a break statement, although that won't fix the problem you're having. Are you not getting any events at all? Or just not events at the right time? i.e. if you register this listener and pan to the bottom of the page, does it fire at all?

Comment 8

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

nope, no events at all

Comment 9

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

Attachment: WIP patch

Comment 10

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

Attachment: patch

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 617521 [details] [diff] [review]
patch

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

Looks good, but r- for too many things that need changing/polishing. I'm perhaps not the right person to review nsAppShell either, maybe mfinkle?

::: mobile/android/base/GeckoApp.java
@@ +48,5 @@
>  import org.mozilla.gecko.gfx.Layer;
>  import org.mozilla.gecko.gfx.LayerController;
>  import org.mozilla.gecko.gfx.LayerView;
>  import org.mozilla.gecko.gfx.RectUtils;
> +import org.mozilla.gecko.gfx.ScreenshotLayer;

It doesn't seem like this or the other added import above are used?

::: mobile/android/base/GeckoAppShell.java
@@ +540,2 @@
>                          return;
> +                

Added trailing spaces here.

@@ +545,5 @@
> +                    case SCREENSHOT_WHOLE_PAGE:
> +                        GeckoApp.mAppContext.getLayerClient().setCheckerboardLayer(b);
> +                        break;
> +                    case SCREENSHOT_UPDATE:
> +                        GeckoApp.mAppContext.getLayerClient().updateCheckerboardLayer(b, mLastCheckerboardWidthRatio * x, mLastCheckerboardHeightRatio * y, mLastCheckerboardWidthRatio * width, mLastCheckerboardHeightRatio * height);

Would be nice to break this over 3, maybe 5 lines.

@@ +2121,5 @@
>              GeckoAppShell.notifyFilePickerResult("", id);
>      }
> +
> +    static float sDirtyTop = Float.POSITIVE_INFINITY, sDirtyLeft = Float.POSITIVE_INFINITY;
> +    static float sDirtyBottom = Float.NEGATIVE_INFINITY, sDirtyRight = Float.NEGATIVE_INFINITY;

These should be enclosed in the Runnable below (if possible), and documented.

@@ +2134,5 @@
> +                bottom = sDirtyBottom;
> +                sDirtyTop = Float.POSITIVE_INFINITY;
> +                sDirtyLeft = Float.POSITIVE_INFINITY;
> +                sDirtyBottom = Float.NEGATIVE_INFINITY;
> +                sDirtyRight = Float.NEGATIVE_INFINITY;

Why are these reassigned here? It doesn't seem like they're ever changed. In fact, given that, can't you just assign top/left/right/bottom directly, and without synchronization?

Just dawned on me that this is probably unfinished and they will be changed, or a remnant of an older revision?

@@ +2150,5 @@
> +                GeckoAppShell.screenshotWholePage(tab);
> +            }
> +        }
> +    };
> +    static boolean sIsRepaintRunnablePosted = false;

Should be at the top. Name is self-evident, but comment might be nice anyway.

@@ +2166,5 @@
> +        }
> +    }
> +
> +    static private float mCheckerboardPageWidth, mCheckerboardPageHeight;
> +    static private float mLastCheckerboardWidthRatio, mLastCheckerboardHeightRatio;

These should be at the top, and documented.

@@ +2178,5 @@
> +        float ratio = (float)Math.sqrt((sw * sh) / (ScreenshotLayer.getMaxNumPixels() / 4));
> +        int dw = CairoUtils.nextPowerOfTwo(sw / ratio);
> +        int dh = CairoUtils.nextPowerOfTwo(sh / ratio);
> +        mLastCheckerboardWidthRatio = dw / sw;
> +        mLastCheckerboardHeightRatio = dh / sh;

I'd like to have a bit more commenting in this function to know what's going on - and perhaps more descriptive variable names.

::: mobile/android/base/GeckoEvent.java
@@ +477,4 @@
>          return event;
>      }
>  
> +    public static GeckoEvent createScreenshotEvent(int tabId, int sx, int sy, int sw, int sh, int dx, int dy, int dw, int dh, int token) {

Does changing this event require a corresponding change in xul-fennec?

::: mobile/android/base/gfx/CairoUtils.java
@@ +90,5 @@
> +        return isPowerOf2(val) ? val : nextPowerOfTwo((float)val);
> +                        
> +    }
> +
> +    public static int nextPowerOfTwo(float val) {

We have a more efficient version of this in IntSize already - isPowerOfTwo would be a nice addition there though.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +310,5 @@
> +    public void updateCheckerboardLayer(Bitmap bitmap, float x, float y, float width, float height) {
> +        mLayerRenderer.updateCheckerboardLayer(bitmap, x, y, width, height);
> +    }
> +
> +    public void resetCheckerboard() {

It might be better to add these functions in LayerView instead of here - LayerView already has a few LayerRenderer accessor functions.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +158,4 @@
>          "    gl_FragColor = texture2D(sTexture, vec2(vTexCoord.x, 1.0 - vTexCoord.y));\n" +
>          "}\n";
>  
> +    public void setCheckerboardLayer(Bitmap bitmap) {

Maybe rename to setCheckerboardBitmap?

@@ +167,5 @@
> +            mCheckerboardLayer.endTransaction();
> +        }
> +    }
> +
> +    public void updateCheckerboardLayer(Bitmap bitmap, float x, float y, float width, float height) {

and updateCheckerboardBitmap here?

@@ +196,4 @@
>          CairoImage backgroundImage = new BufferedCairoImage(controller.getBackgroundPattern());
>          mBackgroundLayer = new SingleTileLayer(true, backgroundImage);
>  
> +        mCheckerboardLayer = ScreenshotLayer.create(new IntSize(200, 200));

Why 200x200 to begin with? Why not just new IntSize() (i.e. 0x0)?

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +22,5 @@
> +  static final int SCREENSHOT_SIZE_LIMIT = 1048576;
> +  ScreenshotImage mImage;
> +  private IntSize mImageSize;
> +  private IntSize mBufferSize;
> +  boolean mIsSingleColor;

These should all be commented.

@@ +36,5 @@
> +  
> +  void setBitmap(Bitmap bitmap) {
> +    mImageSize = new IntSize(bitmap.getWidth(), bitmap.getHeight());
> +    int width = CairoUtils.nextPowerOfTwo(bitmap.getWidth());
> +    int height = CairoUtils.nextPowerOfTwo(bitmap.getHeight());

This shouldn't be necessary with GLES 2.0. If indeed it isn't, mBufferSize can be gotten rid of.

@@ +57,5 @@
> +  public static ScreenshotLayer create(Bitmap bitmap) {
> +    IntSize size = new IntSize(bitmap.getWidth(), bitmap.getHeight());
> +    ByteBuffer buffer = GeckoAppShell.allocateDirectBuffer(SCREENSHOT_SIZE_LIMIT * 2);
> +    ScreenshotLayer sl =  new ScreenshotLayer(new ScreenshotImage(buffer, size.width, size.height, CairoImage.FORMAT_RGB16_565), size);
> +    sl.setBitmap(bitmap);

Would be good to have some comments as to what's going on here (I'm aware, but it doesn't read that obviously I don't think).

@@ +86,5 @@
> +
> +	    float vl = context.viewport.left;
> +	    float vr = context.viewport.right;
> +	    float vt = context.viewport.top;
> +	    float vb = context.viewport.bottom;

Rather than always draw this at the size of the viewport and reset it on size-changes, it might be nice to keep track of its separate page size and draw it at the correct ratio here - that way, when the page changes size, there won't be a moment in-between updates where we don't have a buffer.

@@ +134,5 @@
> +    Paint backgroundPaint = new Paint();
> +    backgroundPaint.setColor(Color.rgb(Color.red(backgroundcolor), Color.green(backgroundcolor), Color.blue(backgroundcolor)));
> +    canvas.drawRect(0.0f, 0.0f, mImageSize.width, mImageSize.height, backgroundPaint);
> +    setBitmap(bitmap);
> +    mIsSingleColor = true;

ok, this is sub-optimal but you already told me that :)

::: mobile/android/base/gfx/TileLayer.java
@@ +63,4 @@
>      private IntSize mSize;
>      private int[] mTextureIDs;
>  
> +    public TileLayer(CairoImage image, boolean stretch) {

I don't like this overloading - we should have an enum for draw mode - NORMAL, REPEAT, STRETCH.

::: widget/android/AndroidBridge.cpp
@@ +2185,5 @@
> +    JNIEnv* jenv = AndroidBridge::GetJNIEnv();
> +    if (!jenv)
> +        return;
> +    jenv->CallStaticVoidMethod(AndroidBridge::Bridge()->mGeckoAppShellClass, AndroidBridge::Bridge()->jNotifyPaintedRect, top, left, bottom, right);
> +    

Trailing blank line.

::: widget/android/nsAppShell.cpp
@@ +95,4 @@
>  
>  NS_IMPL_ISUPPORTS_INHERITED1(nsAppShell, nsBaseAppShell, nsIObserver)
>  
> +class AfterPaintListener : public nsIDOMEventListener {

Does this need a destructor to call Unregister()?

@@ +126,5 @@
> +        if (!rects)
> +            return NS_OK;
> +        PRUint32 length;
> +        rects->GetLength(&length);
> +        for (PRUint32 i = 0; i < length; ++i) {

Do we want to just take the bounding rect of all the rects here?

Comment 12

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

> 
> @@ +36,5 @@
> > +  
> > +  void setBitmap(Bitmap bitmap) {
> > +    mImageSize = new IntSize(bitmap.getWidth(), bitmap.getHeight());
> > +    int width = CairoUtils.nextPowerOfTwo(bitmap.getWidth());
> > +    int height = CairoUtils.nextPowerOfTwo(bitmap.getHeight());
> 
> This shouldn't be necessary with GLES 2.0. If indeed it isn't, mBufferSize
> can be gotten rid of.

My testing shows that this is needed. It would be nice if it wasn't needed though.

Comment 13

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

> > +    public static GeckoEvent createScreenshotEvent(int tabId, int sx, int sy, int sw, int sh, int dx, int dy, int dw, int dh, int token) {
> 
> Does changing this event require a corresponding change in xul-fennec?
No, this event is never created in XUL fennec. Also, these extra ints are just added to an existing int array

Comment 14

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

> @@ +86,5 @@
> > +
> > +	    float vl = context.viewport.left;
> > +	    float vr = context.viewport.right;
> > +	    float vt = context.viewport.top;
> > +	    float vb = context.viewport.bottom;
> 
> Rather than always draw this at the size of the viewport and reset it on
> size-changes, it might be nice to keep track of its separate page size and
> draw it at the correct ratio here - that way, when the page changes size,
> there won't be a moment in-between updates where we don't have a buffer.
Let's look at this in a follow up.

Comment 15

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

> 
> @@ +126,5 @@
> > +        if (!rects)
> > +            return NS_OK;
> > +        PRUint32 length;
> > +        rects->GetLength(&length);
> > +        for (PRUint32 i = 0; i < length; ++i) {
> 
> Do we want to just take the bounding rect of all the rects here?
In practice, they come in one at a time, so I don't think it matters

Comment 16

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

Attachment: patch

Comment 17

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

Comment on attachment 617736 [details] [diff] [review]
patch

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

Initial comments (I didn't see what Cwiiis said so there might be some duplicated stuff). I need to go over ScreenshotLayer some more.

::: mobile/android/base/GeckoApp.java
@@ +42,4 @@
>  
>  import org.mozilla.gecko.db.BrowserDB;
>  import org.mozilla.gecko.gfx.FloatSize;
> +import org.mozilla.gecko.gfx.CairoUtils;

Unused import

::: mobile/android/base/GeckoAppShell.java
@@ +123,4 @@
>      private static Boolean sNSSLibsLoaded = false;
>      private static Boolean sLibsSetup = false;
>      private static File sGREDir = null;
> +    private static float mCheckerboardPageWidth, mCheckerboardPageHeight;

These values are never assigned.

@@ +545,5 @@
>                      b.copyPixelsFromBuffer(data);
> +                    switch (token) {
> +                    case SCREENSHOT_WHOLE_PAGE:
> +                        GeckoApp.mAppContext.getLayerController()
> +                            .getView().setCheckerboardBitmap(b);

What if the selected tab has changed by time we get this back?

@@ +549,5 @@
> +                            .getView().setCheckerboardBitmap(b);
> +                        break;
> +                    case SCREENSHOT_UPDATE:
> +                        GeckoApp.mAppContext.getLayerController().getView().updateCheckerboardBitmap(
> +                            b,mLastCheckerboardWidthRatio * x, mLastCheckerboardHeightRatio * y,

Need a space after "b,"

@@ +2126,5 @@
>          if (!GeckoApp.mAppContext.showFilePicker(aMimeType, new AsyncResultHandler(id)))
>              GeckoAppShell.notifyFilePickerResult("", id);
>      }
> +
> +    static class RepaintRunnable implements Runnable{

Need a space after "Runnable"

@@ +2129,5 @@
> +
> +    static class RepaintRunnable implements Runnable{
> +        boolean sIsRepaintRunnablePosted = false;
> +        float sDirtyTop = Float.POSITIVE_INFINITY, sDirtyLeft = Float.POSITIVE_INFINITY;
> +        float sDirtyBottom = Float.NEGATIVE_INFINITY, sDirtyRight = Float.NEGATIVE_INFINITY;

Make member variables private. Also prefix with "m" rather than "s" since the variables are not static.

::: mobile/android/base/Tabs.java
@@ +127,5 @@
>          else
>              GeckoApp.mAppContext.hideAboutHome();
>  
> +        GeckoApp.mAppContext.getLayerController().getView().resetCheckerboard();
> +        GeckoAppShell.screenshotWholePage(tab);

This should be moved to the end of GeckoLayerClient.setFirstPaintViewport.

::: mobile/android/base/gfx/CairoUtils.java
@@ +85,1 @@
>  }

Remove empty line

::: mobile/android/base/gfx/IntSize.java
@@ +84,5 @@
>  
>      /* Returns the power of two that is greater than or equal to value */
>      public static int nextPowerOfTwo(int value) {
> +        if (isPowerOf2(value))
> +            return value;

Is this just a fast code path to avoid the bitshifthing below? How often does this function get called? I'm not sure it's needed and it adds risk.

@@ +107,5 @@
>          return new IntSize(nextPowerOfTwo(width), nextPowerOfTwo(height));
>      }
> +
> +    static boolean isPowerOf2(int val) {
> +        return val == 0 ? false : (val & ~(val - 1))== val;

Need a space before "=="

@@ +108,5 @@
>      }
> +
> +    static boolean isPowerOf2(int val) {
> +        return val == 0 ? false : (val & ~(val - 1))== val;
> +    }

This fails for val == 1.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +423,2 @@
>  
>          mCheckerboardLayer.beginTransaction();  // called on compositor thread

Taking out the condition here will break eideticker, it relies on a solid checkerboard colour.

::: mobile/android/base/gfx/LayerView.java
@@ +198,5 @@
> +    }
> +
> +    public void resetCheckerboard() {
> +        mRenderer.resetCheckerboard();
> +    }

Having these functions here when they do nothing but delegate to LayerRenderer seems like unnecessary clutter. I'd rather have the callers just call getView().getRenderer().resetCheckerboard() and so on.

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +22,5 @@
> +import java.nio.ByteBuffer;
> +import java.nio.IntBuffer;
> +import java.nio.FloatBuffer;
> +
> +public class ScreenshotLayer extends SingleTileLayer {

Indentation needs to be 4-space in this class.

@@ +23,5 @@
> +import java.nio.IntBuffer;
> +import java.nio.FloatBuffer;
> +
> +public class ScreenshotLayer extends SingleTileLayer {
> +  static final int SCREENSHOT_SIZE_LIMIT = 1048576;

Make this private.

@@ +24,5 @@
> +import java.nio.FloatBuffer;
> +
> +public class ScreenshotLayer extends SingleTileLayer {
> +  static final int SCREENSHOT_SIZE_LIMIT = 1048576;
> +  ScreenshotImage mImage;

Make this private.

@@ +31,5 @@
> +  // The size of the bitmap painted in the buffer 
> +  // (may be smaller than mBufferSize due to power of 2 padding)
> +  private IntSize mImageSize;
> +  // Special case to show the page background color prior to painting a screenshot
> +  boolean mIsSingleColor;

Make this private.

@@ +159,5 @@
> +  public void updateBackground(int color) {
> +    if (!mIsSingleColor)
> +      return;
> +
> +    int red =   ((color & 0x00FF0000 >> 16)* 31 / 255);

add a comment as to what this is doing.

@@ +174,5 @@
> +    mImage.mBuffer.put(mImageSize.width + 3, (byte)((red << 3 | green >> 3) & 0x0000FFFF));
> +    mImage.mBuffer.put(mImageSize.width + 2, (byte)((green << 3 | blue) & 0x0000FFFF));
> +    mImage.mBuffer.put(mImageSize.width + 5, (byte)((red << 3 | green >> 3) & 0x0000FFFF));
> +    mImage.mBuffer.put(mImageSize.width + 4, (byte)((green << 3 | blue) & 0x0000FFFF));
> +    mIsSingleColor = true;

This assignment is unnecessary, mIsSingleColor must be true to not bounce out of this function.

@@ +185,5 @@
> +    private int mFormat;
> +
> +    /** Creates a buffered Cairo image from a byte buffer. */
> +    public ScreenshotImage(ByteBuffer inBuffer, int inWidth, int inHeight, int inFormat) {
> +      mBuffer = inBuffer; mSize = new IntSize(inWidth, inHeight); mFormat = inFormat;

One statement per line.

::: mobile/android/base/gfx/TileLayer.java
@@ +61,5 @@
>      private IntSize mSize;
>      private int[] mTextureIDs;
>  
> +    public enum PaintMode { NORMAL, REPEAT, STRETCH };
> +    PaintMode mPaintMode;

This should be private.

@@ +73,5 @@
>          mDirtyRect = new Rect();
>      }
>  
> +    protected boolean repeats() { return mPaintMode == PaintMode.REPEAT; }
> +    protected boolean stretch() { return mPaintMode == PaintMode.STRETCH; }

This should be called "stretches()" for consistency.

Comment 18

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

Attachment: patch

Comment 19

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

Comment on attachment 617883 [details] [diff] [review]
patch

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

Some more comments.

::: mobile/android/app/mobile.js
@@ +725,4 @@
>  pref("direct-texture.force.disabled", false);
>  
>  // show checkerboard pattern on android; we use background colour instead
> +pref("gfx.show_checkerboard_pattern", true);

Intentional change?

::: mobile/android/base/GeckoAppShell.java
@@ +532,4 @@
>              mInputConnection.notifyIMEChange(text, start, end, newEnd);
>      }
>  
> +    public static void notifyScreenShot(final ByteBuffer data, final int tabId, final int x, final int y,

Add a comment that this is invoked by JNI, and add a stub to XUL fennec.

@@ +2181,5 @@
> +    }
> +
> +    static private RepaintRunnable sRepaintRunnable = new RepaintRunnable();
> +
> +    public static void notifyPaintedRect(float top, float left, float bottom, float right) {

Add a comment that this is invoked by JNI, and add a stub to XUL fennec.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +339,5 @@
>              // be out of sync with this first-paint viewport, then we force them back in sync.
>              mLayerController.abortPanZoomAnimation();
>              mLayerController.getView().setPaintState(LayerView.PAINT_BEFORE_FIRST);
> +            GeckoApp.mAppContext.getLayerController().getView().getRenderer().resetCheckerboard();
> +            GeckoAppShell.screenshotWholePage(Tabs.getInstance().getSelectedTab());

Move these to outside the synchronized block. Also you can use mLayerController instead of GeckoApp....getLayerController().

::: mobile/android/base/gfx/LayerView.java
@@ +44,4 @@
>  import org.mozilla.gecko.gfx.InputConnectionHandler;
>  import org.mozilla.gecko.gfx.LayerController;
>  import android.content.Context;
> +import android.graphics.Bitmap;

Unused import

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +62,5 @@
> +    }
> +
> +    public static ScreenshotLayer create() {
> +        // 3 x 3 min for the single color case
> +        return ScreenshotLayer.create(new IntSize(3, 3));

I don't understand why this is 3x3. Shouldn't it be 2x2?

@@ +175,5 @@
> +        int blue =   (color & 0x000000FF) * 31 / 255;
> +        /* For the first byte left shift red by 3 positions such that it is the
> +         * top 5 bits, right shift green by 3 so its 3 most significant are the
> +         * 3 least significant. For the second byte, left shift green by 3 so 
> +         * its 3 least signifcant bits are the 3 most significant bits of the

Shouldn't you shift green up by 5 to accomplish this?

::: widget/android/nsAppShell.cpp
@@ +146,5 @@
> +            Unregister();
> +    }
> +
> +  private:
> +    nsCOMPtr<nsIDOMEventTarget> mEventTarget;

This should be initialized to something somewhere.

@@ +470,3 @@
>  
>          nsTArray<nsIntPoint> points = curEvent->Points();
>          NS_ASSERTION(points.Length() == 2, "Screenshot event does not have enough coordinates");

This assertion is wrong.

Comment 20

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

(In reply to Kartikaya Gupta (:kats) from comment #19)
> Comment on attachment 617883 [details] [diff] [review]
> patch
> 
> Review of attachment 617883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some more comments.
> 
> ::: mobile/android/app/mobile.js
> @@ +725,4 @@
> >  pref("direct-texture.force.disabled", false);
> >  
> >  // show checkerboard pattern on android; we use background colour instead
> > +pref("gfx.show_checkerboard_pattern", true);
> 
> Intentional change?
Yup, otherwise we only show the page's background color



> ::: mobile/android/base/gfx/LayerView.java
> @@ +44,4 @@
> >  import org.mozilla.gecko.gfx.InputConnectionHandler;
> >  import org.mozilla.gecko.gfx.LayerController;
> >  import android.content.Context;
> > +import android.graphics.Bitmap;
> 
> Unused import
> 
> ::: mobile/android/base/gfx/ScreenshotLayer.java
> @@ +62,5 @@
> > +    }
> > +
> > +    public static ScreenshotLayer create() {
> > +        // 3 x 3 min for the single color case
> > +        return ScreenshotLayer.create(new IntSize(3, 3));
> 
> I don't understand why this is 3x3. Shouldn't it be 2x2?
with 2x2 I'm seeing some of the third pixel's color get blended in. 


> 
> ::: widget/android/nsAppShell.cpp
> @@ +146,5 @@
> > +            Unregister();
> > +    }
> > +
> > +  private:
> > +    nsCOMPtr<nsIDOMEventTarget> mEventTarget;
> 
> This should be initialized to something somewhere.
Why? nsCOMPtr inits to null, which is what we want.

Comment 21

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

Attachment: patch

Comment 22

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

Attachment: patch

Comment 23

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

Comment on attachment 617917 [details] [diff] [review]
patch

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

Some final nits but nothing that should block landing if we're in a hurry. Not too much work to fix them up either.

::: mobile/android/base/GeckoAppShell.java
@@ +126,5 @@
>      private static Boolean sNSSLibsLoaded = false;
>      private static Boolean sLibsSetup = false;
>      private static File sGREDir = null;
> +    private static float mCheckerboardPageWidth, mCheckerboardPageHeight;
> +    private static float mLastCheckerboardWidthRatio, mLastCheckerboardHeightRatio;

nit: prefix with s here instead of m.

@@ +546,1 @@
>                          return;

This early-exit condition is here presumably to avoid allocating the bitmap if we don't need it. Consider merging the isSelectedTab exit condition for the other two token types into this condition, or move this condition into the SCREENSHOT_THUMBNAIL case statement.

@@ +548,1 @@
>                      Bitmap b = Bitmap.createBitmap(width, height, Bitmap.Config.RGB_565);

nit: trailing spaces on the blank line here.

@@ +2148,5 @@
> +
> +    static class RepaintRunnable implements Runnable {
> +        boolean mIsRepaintRunnablePosted = false;
> +        float mDirtyTop = Float.POSITIVE_INFINITY, mDirtyLeft = Float.POSITIVE_INFINITY;
> +        float mDirtyBottom = Float.NEGATIVE_INFINITY, mDirtyRight = Float.NEGATIVE_INFINITY;

These should be private.

@@ +2192,5 @@
> +            }
> +        }
> +    }
> +
> +    static private RepaintRunnable sRepaintRunnable = new RepaintRunnable();

Move this to the top of the file.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +423,2 @@
>  
>          mCheckerboardLayer.beginTransaction();  // called on compositor thread

Oh, I misread this code originally. Taking out the early-exit condition here will probably impact performance because now the transaction/invalidate will happen on every single frame of animation, which will cause a texture upload as well. This is probably undesirable. Maybe have a mCheckerboardLayer.updateBackground return a boolean that indicates if stuff changed and only invalidate in that case?

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +25,5 @@
> +import java.nio.FloatBuffer;
> +
> +public class ScreenshotLayer extends SingleTileLayer {
> +    private static final int SCREENSHOT_SIZE_LIMIT = 1048576;
> +    ScreenshotImage mImage;

This should be private

@@ +62,5 @@
> +    }
> +
> +    public static ScreenshotLayer create() {
> +        // 3 x 3 min for the single color case
> +        return ScreenshotLayer.create(new IntSize(3, 3));

Add a comment about why this is 3x3

@@ +189,5 @@
> +        mImage.mBuffer.put(mImageSize.width + 0, (byte)((green << 3 | blue) & 0x0000FFFF));
> +        mImage.mBuffer.put(mImageSize.width + 3, (byte)((red << 3 | green >> 3) & 0x0000FFFF));
> +        mImage.mBuffer.put(mImageSize.width + 2, (byte)((green << 3 | blue) & 0x0000FFFF));
> +        mImage.mBuffer.put(mImageSize.width + 5, (byte)((red << 3 | green >> 3) & 0x0000FFFF));
> +        mImage.mBuffer.put(mImageSize.width + 4, (byte)((green << 3 | blue) & 0x0000FFFF));

The code here still shifts green by 3 instead of 5.

@@ +200,5 @@
> +        private int mFormat;
> +
> +        /** Creates a buffered Cairo image from a byte buffer. */
> +        public ScreenshotImage(ByteBuffer inBuffer, int inWidth, int inHeight, int inFormat) {
> +            mBuffer = inBuffer; mSize = new IntSize(inWidth, inHeight); mFormat = inFormat;

nit: one statement per line

@@ +203,5 @@
> +        public ScreenshotImage(ByteBuffer inBuffer, int inWidth, int inHeight, int inFormat) {
> +            mBuffer = inBuffer; mSize = new IntSize(inWidth, inHeight); mFormat = inFormat;
> +        }
> +
> +        protected void finalize() throws Throwable {

Add an @Override on this function

Comment 24

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

Attachment: patch

Comment 25

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

Comment on attachment 617964 [details] [diff] [review]
patch

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

r+ with comment fixed.

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +37,5 @@
> +    // Force single color, needed for testing
> +    private boolean mForceSingleColor = false;
> +    // Cache the passed in background color to determine if we need to update
> +    // initialized to 0 so it lets the code run to set it to white on init
> +    private int mCurrentBackgroundColor = 0;

This is never assigned anything else.

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7633ec2f52b9

Comment 27

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7633ec2f52b9

Comment 28

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

Comment on attachment 617964 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:

Comment 29

[JP Rosevear [:jpr]]

Comment on attachment 617964 [details] [diff] [review]
patch

Mobile only.

Comment 30

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c584e12ad1bd

Comment 31

[Paul Feher]

Issue is no longer reproducible on Nightly 16.0a1 2012-06-12 and Aurora 15.0a2 2012-06-11 on HTC Desire Z running Android 2.3.3. Low-res screenshots are displayed when scrolling. Marking as verified.