773667 - Code clean up in ScreenshotLayer

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Geoff Brown [:gbrown]]

There is some suspected-dead-code in ScreenshotLayer referencing createBitmap -- we should remove that if possible.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: remove unused code in ScreenshotLayer

One call to createBitmap remains -- it is definitely called.

This passes try: https://tbpl.mozilla.org/?tree=Try&rev=362ee3458cfc

Comment 2

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

Comment on attachment 642562 [details] [diff] [review]
remove unused code in ScreenshotLayer

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

clearing the review for these questions. Please re-request when you answer.

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +52,5 @@
>              mBufferSize = mImageSize;
>              mHasImage = true;
>              mImage.setBitmap(data, width, height, CairoImage.FORMAT_RGB16_565, rect);
>          } else {
> +            throw new RuntimeException("unexpected size in setBitmap: w="+width+" h="+height);

where is this caught? I don't want to crash due to this.

@@ +148,5 @@
>              mFormat = format;
>              if (width == bitmap.getWidth() && height == bitmap.getHeight()) {
>                  tmp = bitmap;
>              } else {
> +                throw new RuntimeException("unexpected size in setBitmap: w="+width+" h="+height);

same question here

Comment 3

[Geoff Brown [:gbrown]]

Attachment: remove unused code in ScreenshotLayer

The exceptions in the previous patch were uncaught; in this patch, we log an error instead.

Comment 4

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

Comment on attachment 644813 [details] [diff] [review]
remove unused code in ScreenshotLayer

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

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +42,5 @@
>              mBufferSize = mImageSize;
>              mHasImage = true;
>              mImage.setBitmap(data, width, height, CairoImage.FORMAT_RGB16_565, rect);
>          } else {
> +            Log.e(LOGTAG, "### unexpected size in setBitmap: w="+width+" h="+height);

I'd still like to throw here, I'd just like it to be declared and caught so we get a stack and still don't crash.

@@ +142,2 @@
>              } else {
> +                Log.e(LOGTAG, "### unexpected size in setBitmap: w="+width+" h="+height);

same. Sorry if this wasn't clear before.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: remove unused code in ScreenshotLayer

Like this? I've gone back to throwing exceptions and added try/catch/Log to handle them.

Comment 6

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb440488598

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4fb440488598