tab thumbnail is sometimes outdated

RESOLVED INVALID

Status

Firefox for Android Graveyard
General
RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Bug Flags:
in-testsuite ?

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 550864 [details]
screenshot

See screenshot.

The android dev website was down and when it was finally up, the icon wasn't updated...
This happens because we do not keep updating the preview. We take a snapshot once the page initially loads, and that's it.
Summary: The preview icon in the left panel is sometimes outdated → tab thumbnail is sometimes outdated
Duping. Reopen if you mean something different
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 515667
(Assignee)

Comment 3

7 years ago
I didn't really meant to have the thumbnail being live only updated on page load. I don't think it's a good behavior to refresh a page, get another content but don't change the thumbnail.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 4

7 years ago
Created attachment 552776 [details] [diff] [review]
Patch v1

What about this?
Assignee: nobody → mounir
Status: REOPENED → ASSIGNED
Attachment #552776 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review]
Comment on attachment 552776 [details] [diff] [review]
Patch v1

We should check HG blame for the reason why we added _drawThumb. It was not always there, so there must be a reason (bug) for why we added it.
(Assignee)

Comment 6

7 years ago
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 552776 [details] [diff] [review]
> Patch v1
> 
> We should check HG blame for the reason why we added _drawThumb. It was not
> always there, so there must be a reason (bug) for why we added it.

If I understand it correctly, the idea was to draw the thumbnail after the first paint and after the page load. In other words, with my patch, it will be drawn twice. If this is sensitive, we could probably set drawThumb to true at startLoading. I believe the result would be the same.
(Assignee)

Comment 7

7 years ago
Created attachment 560073 [details] [diff] [review]
Patch v2

So I did have a deeper look and _drawThumb was here to be sure we draw the thumbnail when the first paint was done AND the page was loaded. We were trying to draw the thumbnail at the first paint but were canceling that if the page was still loading. At page load we were always updating the thumbnail. Both calls were making sure the thumbnail was updating in at least one situation.

My patch makes sure we call updateThumbnail() only when the loading is done and we makes us listening to the first paint if the first paint isn't done at that point.
Attachment #552776 - Attachment is obsolete: true
Attachment #552776 - Flags: review?(mark.finkle)
Attachment #560073 - Flags: review?(mark.finkle)
Comment on attachment 560073 [details] [diff] [review]
Patch v2

>   endLoading: function endLoading() {

>+    if (!this._delayedThumbnail) {
>+      if (this._firstPaint) {
>+        this.updateThumbnail();
>+      } else if (!this._delayedThumbnail) {

Can't this just be an "else" ? We are already in | if (!this._delayedThumbnail) |

r+ with the "else" change, if it's OK
Attachment #560073 - Flags: review?(mark.finkle) → review+
Is there a way to add a test for this situation? If so, can you make one?
(Assignee)

Comment 11

7 years ago
(In reply to Mark Finkle (:mfinkle) from comment #10)
> We have some simple thumbnail tests here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/
> browser_thumbnails.js

Unfortunately, this test file doesn't check the thumbnail content but the position and size. I wonder how I could test the content and I believe I should do that later. It will require me to do testing on my phone which I still haven't done yet.
(Assignee)

Updated

7 years ago
Flags: in-testsuite?
Whiteboard: [needs review] → [needs landing]
(Assignee)

Updated

7 years ago
Whiteboard: [needs landing] → [needs tests][inbound]
(Assignee)

Updated

7 years ago
Attachment #560073 - Flags: checkin+
(Assignee)

Updated

7 years ago
Whiteboard: [needs tests][inbound] → [needs tests fixes]
(Assignee)

Updated

7 years ago
Attachment #560073 - Flags: checkin+ → checkin-
Duplicate of this bug: 689090
Blocks: 627354
I was able to reproduce this issue by following these steps:
1. Open Fennec
2. Open a new blank tab
3. Bookmark the blank page opened at step 2
4. In the same tab, browse to any webpage (ex: www.mozilla.com) and wait until the page is fully loaded
5. Tap on URL Bar > Bookmarks tab
6. Tap on about:blank page 
7. Check the tab thumbnail if it's updated

Actual result:
After step 7, there will be the same thumbnail at it was after step 4.

Notes:
- after step 7, if about:firefox will be opened in the same tab, actually it will open in a new tab. For the one opened at step 2, about:firefox will be auto-filled in the URL Bar.
- this issue occurs on the latest Aurora and Nightly builds.

--
Build ID: Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111009
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2
(Assignee)

Comment 14

5 years ago
Time has passed and this patch no longer makes sense (it was for the XUL product).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → INVALID
Whiteboard: [needs tests fixes]
You need to log in before you can comment on or make changes to this bug.