If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

large session restore screenshots taken too often

RESOLVED FIXED in Firefox 13

Status

()

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

People

(Reporter: blassey, Assigned: blassey)

Tracking

unspecified
Firefox 13
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 596937 [details] [diff] [review]
patch

This is slowing down several things, including the tabs tray and various dialogs
Attachment #596937 - Flags: review?(mark.finkle)
Comment on attachment 596937 [details] [diff] [review]
patch


>+                ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
>+                String viewportJSON = null;
>+                if (viewportMetrics != null)
>+                    viewportJSON = viewportMetrics.toJSON();
>+                if (viewportJSON == null || viewportJSON.equals(mLastViewport) &&
>+                    mLastTitle.equals(lastHistoryEntry.mTitle) &&
>+                    mLastSnapshotUri.equals(lastHistoryEntry.mUri))
>                     return;

Can we drop the mLastTitle check and move the mLastSnapshotUri check into it's own if-block above the viewportMetrics check?
Attachment #596937 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Comment on attachment 596937 [details] [diff] [review]
> patch
> 
> 
> >+                ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
> >+                String viewportJSON = null;
> >+                if (viewportMetrics != null)
> >+                    viewportJSON = viewportMetrics.toJSON();
> >+                if (viewportJSON == null || viewportJSON.equals(mLastViewport) &&
> >+                    mLastTitle.equals(lastHistoryEntry.mTitle) &&
> >+                    mLastSnapshotUri.equals(lastHistoryEntry.mUri))
> >                     return;
> 
> Can we drop the mLastTitle check and move the mLastSnapshotUri check into
> it's own if-block above the viewportMetrics check?

My thought was that if the title changed, our screenshot was probably stale. Also, now that I look at this it was supposed to be:


> >+    if ((viewportJSON == null || viewportJSON.equals(mLastViewport)) &&
> >+        mLastTitle.equals(lastHistoryEntry.mTitle) &&
> >+        mLastSnapshotUri.equals(lastHistoryEntry.mUri))
> >             return;
(In reply to Brad Lassey [:blassey] from comment #2)
> My thought was that if the title changed, our screenshot was probably stale.
> Also, now that I look at this it was supposed to be:
> 
> 
> > >+    if ((viewportJSON == null || viewportJSON.equals(mLastViewport)) &&
> > >+        mLastTitle.equals(lastHistoryEntry.mTitle) &&
> > >+        mLastSnapshotUri.equals(lastHistoryEntry.mUri))
> > >             return;

Actually, scratch that. What's in the patch is right. Also, this apparently needs a comment since it confused me.
Created attachment 597172 [details] [diff] [review]
patch, with comments

re-requesting review since I didn't drop the title check
Assignee: nobody → blassey.bugs
Attachment #596937 - Attachment is obsolete: true
Attachment #597172 - Flags: review?(mark.finkle)
Attachment #597172 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/f28fe2f0fe66
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
(Assignee)

Updated

6 years ago
Duplicate of this bug: 727609
From IRC discussion, here are more options for improvement:

1) reducing the number of spurious document-stop events we're getting (~20-30 on CNN)
2) better coalescing of the screenshot runnables
3) not screenshotting during a pan
4) fixing getBitmap() to work with OGL layers
(In reply to Kartikaya Gupta (:kats) from comment #7)
> From IRC discussion, here are more options for improvement:
> 
> 1) reducing the number of spurious document-stop events we're getting
> (~20-30 on CNN)
> 2) better coalescing of the screenshot runnables
> 3) not screenshotting during a pan
> 4) fixing getBitmap() to work with OGL layers

Kats, can you please file follow up bugs for each of those?
Depends on: 727701
Depends on: 727703
I filed bug 727701 and bug 727703 for the first two items on the list. The third one actually shouldn't be a problem; if the (one and only) document-stop event happens while the user is panning, there shouldn't be a problem with using that image as the thumbnail, and the runnable-coalescing should prevent grabbing multiple useless screenshots because the user is panning.

For the 4th item on the list, there's already bug 725120 which seems like a general catch-all for getBitmap(), so I'll add a comment there.
You need to log in before you can comment on or make changes to this bug.