Tab thumbnails have black right and bottom edges

RESOLVED WORKSFORME

Status

()

Firefox for Android
General
P2
normal
RESOLVED WORKSFORME
7 years ago
2 years ago

People

(Reporter: mfinkle, Assigned: sriram)

Tracking

unspecified
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox11 affected, firefox12 affected, firefox13 unaffected, blocking-fennec1.0 +, fennec11+)

Details

(Whiteboard: tabs-ux)

Attachments

(3 attachments)

Created attachment 592216 [details]
screenshot

The code to create thumbnails must not be using the right density, or the density is wrong. The selected tab is showing fine - it is using the special full thumbnail case. The background tabs have bad image sizes. I can't tell if the size sent to Javascript is wrong or the bitmap creation in Java is wrong.
(Reporter)

Updated

7 years ago
Assignee: nobody → sriram
Whiteboard: tabs-ux
(Assignee)

Comment 1

7 years ago
I am not sure why source is overwritten here: https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/GeckoApp.java#l622
I believe it should be destination. However, changing it to destination doesn't help. Where can i find the implementation of this screenshot in Gecko side? Doesn't it take the destination argument?

Also, https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/Tabs.java#l306 - making this true will get bigger background screenshots. However, I would like to know the impact of this change.
(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> I am not sure why source is overwritten here:
> https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/GeckoApp.
> java#l622
> I believe it should be destination. However, changing it to destination
> doesn't help. Where can i find the implementation of this screenshot in
> Gecko side? Doesn't it take the destination argument?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1579
(Assignee)

Comment 3

7 years ago
The attachment: https://bugzilla.mozilla.org/attachment.cgi?id=594354 in bug 723251 fixes this issue. There needs some cleanup in that patch, which I will post in this bug once that lands.
(Assignee)

Comment 4

7 years ago
Created attachment 594850 [details] [diff] [review]
Patch

This patch removes the black parts on background tabs. However, I still see issues with foreground tabs. :Arreth's patch might help fixing that.
Attachment #594850 - Flags: review?(mark.finkle)
Comment on attachment 594850 [details] [diff] [review]
Patch


>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java

>     int getMinScreenshotHeight() {
>-        if (sMinScreenshotHeight != 0)
>-            return sMinScreenshotHeight;
>-        return sMinScreenshotHeight = (int)(getMinDim() * kThumbnailHeight);
>+        if (sMinScreenshotHeight == 0)
>+            initMetrics();
>+        return sMinScreenshotHeight = (sMinScreenshotWidth * kThumbnailHeight / kThumbnailWidth);

We should skip the sMinScreenshotHeight calculation too. Put it in the if block and just retur sMinScreenshtoWidth:

          if (sMinScreenshotHeight == 0) {
              initMetrics();
              sMinScreenshotHeight = sMinScreenshotWidth * kThumbnailHeight / kThumbnailWidth;
          }
          return sMinScreenshotHeight;

r+ with that change
Attachment #594850 - Flags: review?(mark.finkle) → review+
Keywords: fennecnative-releaseblocker
This landed on inbound 2 weeks ago but the bug's still open without a central commit message. THUMBNAIL BUG, Y U NO CLOSE?
(Assignee)

Comment 8

6 years ago
The latest clone of inbound doesnt have my changes :(
(Assignee)

Comment 9

6 years ago
I get it. I am sorry. This was backed out due to OOM failures.
There has been a lot of changes going on in thumbnails and I'm waiting for a final word the approach that is going to be used. The current code doesn't fix black areas on foreground tabs.
status-firefox11: --- → affected
status-firefox13: --- → unaffected
blocking-fennec1.0: --- → +
Status: NEW → ASSIGNED
This seems to be working for me
Keywords: qawanted
I still see this.

--
Galaxy Nexus (Android 4.0.2)
Nightly (03/13)
Keywords: qawanted
(Assignee)

Comment 12

6 years ago
I dont see this happening after bug 735790 was fixed. Please let me know if there is any problem with any site.
Created attachment 607605 [details]
Nightly (03/20)

Screenshot taken viewing techmeme.com today with 03/20 Nightly (Galaxy Nexus). Now the thumbnail is not missing the bottom, but the right side. Is this another bug, or does this warrant closure on this particular bug?

Comment 14

6 years ago
As per IRC, can you revisit the page and then refresh about home?  Can't reproduce here on Galaxy Nexus.

Updated

6 years ago
Keywords: qawanted
(In reply to JP Rosevear [:jpr] from comment #14)
> As per IRC, can you revisit the page and then refresh about home?  Can't
> reproduce here on Galaxy Nexus.

No difference; the thumbnail is still missing it's right side even after refresh of about:home and refresh of the page
Works for me getting techmeme onto my topsites list. Sounds like a caching issue. Something the general population won't see.
(In reply to Kevin Brosnan [:kbrosnan] from comment #16)
> Works for me getting techmeme onto my topsites list. Sounds like a caching
> issue. Something the general population won't see.

Didn't take much effort on my end. Tapped the URL bar, tapped a bookmark.

Comment 18

6 years ago
Maybe phone specific?

Comment 19

6 years ago
Any ideas how to get to done on this one?  Aaron, you seem to be the only one who can reproduce this still.  Make you can show it to kats?
Further tested this morning via latest-inbound [1] and I am unable to reproduce anymore. Will re-open if I see this again.

[1] 20120322040235
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd6e82e5edac
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME

Updated

6 years ago
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.