Code clean up in ScreenshotLayer

RESOLVED FIXED in Firefox 17

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

unspecified
Firefox 17
x86
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
There is some suspected-dead-code in ScreenshotLayer referencing createBitmap -- we should remove that if possible.
(Assignee)

Comment 1

6 years ago
Created attachment 642562 [details] [diff] [review]
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
Attachment #642562 - Flags: review?(blassey.bugs)
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
Attachment #642562 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

6 years ago
Created attachment 644813 [details] [diff] [review]
remove unused code in ScreenshotLayer

The exceptions in the previous patch were uncaught; in this patch, we log an error instead.
Attachment #642562 - Attachment is obsolete: true
Attachment #644813 - Flags: review?(blassey.bugs)
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.
Attachment #644813 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 5

6 years ago
Created attachment 644963 [details] [diff] [review]
remove unused code in ScreenshotLayer

Like this? I've gone back to throwing exceptions and added try/catch/Log to handle them.
Attachment #644813 - Attachment is obsolete: true
Attachment #644963 - Flags: review?(blassey.bugs)
Attachment #644963 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 6

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

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/4fb440488598
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.