726929 - large session restore screenshots taken too often

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: patch

This is slowing down several things, including the tabs tray and various dialogs

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 2

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

(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;

Comment 3

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

(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.

Comment 4

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

Attachment: patch, with comments

re-requesting review since I didn't drop the title check

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f28fe2f0fe66

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

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

(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?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.