Last Comment Bug 711543 - Get thumbnail images of tabs loaded in the background, and display them in the tab menu
: Get thumbnail images of tabs loaded in the background, and display them in th...
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P4 normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 11:06 PST by Ian Barlow (:ibarlow)
Modified: 2012-03-07 04:13 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
patch (221 bytes, patch)
2012-01-13 10:59 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (4.82 KB, patch)
2012-01-15 17:26 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (4.59 KB, patch)
2012-01-16 20:25 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review-
Details | Diff | Splinter Review
patch (5.49 KB, patch)
2012-01-17 14:02 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Ian Barlow (:ibarlow) 2011-12-16 11:06:11 PST

    
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-12-28 09:45:55 PST
*** Bug 713874 has been marked as a duplicate of this bug. ***
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-01-13 10:59:56 PST
Created attachment 588467 [details] [diff] [review]
patch
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-13 12:01:14 PST
Bad patch?
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-01-15 17:26:12 PST
Created attachment 588792 [details] [diff] [review]
patch
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-01-16 20:25:27 PST
Created attachment 589097 [details] [diff] [review]
patch

updated to apply
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-16 20:40:06 PST
Comment on attachment 589097 [details] [diff] [review]
patch


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

>+    void getAndProcessThumbnailForTab(Tab tab) {
>+        Bitmap bitmap = null;
>+        if (Tabs.getInstance().isSelectedTab(tab))
>+            mSoftwareLayerClient.getBitmap();

I kinda assume you want to set bitmap = mSoftwareLayerClient.getBitmap();
But honestly, I kinda like not tying to the layer system to get the thumbnail. Maybe we should stop trying to use it at all?

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

>     public void updateThumbnail(final Bitmap b) {

>+                GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
>+                    public void run() {
>+                        GeckoApp.mAppContext.onTabsChanged(tab);
>+                    }
>+                });

What are we doing here? Causing the TabsTray to update when the tab's thumbnail changes? I guess that's OK.

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

>+    public void refreshThumbnails() {
>+        Iterator<Tab> iterator = tabs.values().iterator();
>+        while (iterator.hasNext())
>+            GeckoApp.mAppContext.getAndProcessThumbnailForTab(iterator.next());
>+    }

I do not want to update tabs every time the tabs menu is created, so let's remove this. Tab thumbnails are taken when the page loads and should not be updated afterward.

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

>+        Tabs.getInstance().refreshThumbnails();

Same here. If the | GeckoApp.mAppContext.onTabsChanged(tab); | runnable above is only there to handle updating the TabTray when a refresh happens, let's remove it too.
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-17 11:47:07 PST
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 589097 [details] [diff] [review]
> patch
> 
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+    void getAndProcessThumbnailForTab(Tab tab) {
> >+        Bitmap bitmap = null;
> >+        if (Tabs.getInstance().isSelectedTab(tab))
> >+            mSoftwareLayerClient.getBitmap();
> 
> I kinda assume you want to set bitmap = mSoftwareLayerClient.getBitmap();
> But honestly, I kinda like not tying to the layer system to get the
> thumbnail. Maybe we should stop trying to use it at all?
yea, I do want bitmap = mSoftwareLayerClient.getBitmap()

getting the bitmap from the layer system avoids a substantial amount of work and passing around of data. Its an optimization really, and one we should take advantage of if the conditions are right.

> 
> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> 
> >     public void updateThumbnail(final Bitmap b) {
> 
> >+                GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
> >+                    public void run() {
> >+                        GeckoApp.mAppContext.onTabsChanged(tab);
> >+                    }
> >+                });
> 
> What are we doing here? Causing the TabsTray to update when the tab's
> thumbnail changes? I guess that's OK.

yup
> 
> >diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java
> 
> >+    public void refreshThumbnails() {
> >+        Iterator<Tab> iterator = tabs.values().iterator();
> >+        while (iterator.hasNext())
> >+            GeckoApp.mAppContext.getAndProcessThumbnailForTab(iterator.next());
> >+    }
> 
> I do not want to update tabs every time the tabs menu is created, so let's
> remove this. Tab thumbnails are taken when the page loads and should not be
> updated afterward.
this is generic, asking all tabs to update their thumbnails and not tied to the menu

> 
> >diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
> 
> >+        Tabs.getInstance().refreshThumbnails();
> 
> Same here. If the | GeckoApp.mAppContext.onTabsChanged(tab); | runnable
> above is only there to handle updating the TabTray when a refresh happens,
> let's remove it too.

As of now, background tabs are not updating their thumbnails. This basically refreshes it asynchronously when they tab menu is shown, which I think is a good thing. You can see when looking at the tab menu that one of your background tabs has changed.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-17 11:58:24 PST
Note to self: Using "Tab:Screenshot" will always capture a fixed location thumbnail. If the web page is scrolled down, we still capture at 0, 0. With that in mind, I am less worried about some of the patch.

(In reply to Brad Lassey [:blassey] from comment #7)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > Comment on attachment 589097 [details] [diff] [review]
> > patch
> > 
> > 
> > >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> > 
> > >+    void getAndProcessThumbnailForTab(Tab tab) {
> > >+        Bitmap bitmap = null;
> > >+        if (Tabs.getInstance().isSelectedTab(tab))
> > >+            mSoftwareLayerClient.getBitmap();
> > 
> > I kinda assume you want to set bitmap = mSoftwareLayerClient.getBitmap();
> > But honestly, I kinda like not tying to the layer system to get the
> > thumbnail. Maybe we should stop trying to use it at all?
> yea, I do want bitmap = mSoftwareLayerClient.getBitmap()
> 
> getting the bitmap from the layer system avoids a substantial amount of work
> and passing around of data. Its an optimization really, and one we should
> take advantage of if the conditions are right.

We might run into trouble if the thumbnails use different capture rules. If capturing a thumbnail is as async and cheap as you say below, this might not be an issue. mSoftwareLayerClient.getBitmap(); is certainly more costly from a UI thread point of view. I guess we need to get the full bitmap anyway for "lastscreen". Yuck.

> > >diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java
> > 
> > >+    public void refreshThumbnails() {
> > >+        Iterator<Tab> iterator = tabs.values().iterator();
> > >+        while (iterator.hasNext())
> > >+            GeckoApp.mAppContext.getAndProcessThumbnailForTab(iterator.next());
> > >+    }
> > 
> > I do not want to update tabs every time the tabs menu is created, so let's
> > remove this. Tab thumbnails are taken when the page loads and should not be
> > updated afterward.
> this is generic, asking all tabs to update their thumbnails and not tied to
> the menu

Can we skip any tab that already has a thumbnail?

> > >diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
> > 
> > >+        Tabs.getInstance().refreshThumbnails();
> > 
> > Same here. If the | GeckoApp.mAppContext.onTabsChanged(tab); | runnable
> > above is only there to handle updating the TabTray when a refresh happens,
> > let's remove it too.
> 
> As of now, background tabs are not updating their thumbnails. This basically
> refreshes it asynchronously when they tab menu is shown, which I think is a
> good thing. You can see when looking at the tab menu that one of your
> background tabs has changed.

OK. I could be sold on this.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-01-17 12:16:42 PST
(In reply to Mark Finkle (:mfinkle) from comment #8)
> > > 
> > > I do not want to update tabs every time the tabs menu is created, so let's
> > > remove this. Tab thumbnails are taken when the page loads and should not be
> > > updated afterward.
> > this is generic, asking all tabs to update their thumbnails and not tied to
> > the menu
> 
> Can we skip any tab that already has a thumbnail?
I don't know about skipping, since the point is to refresh the thumbnails in the background. But we could prioritize tabs with no thumbnails over tabs with thumbnails, is that what you want to do?
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2012-01-17 14:00:39 PST
(In reply to Mark Finkle (:mfinkle) from comment #8)
> We might run into trouble if the thumbnails use different capture rules. If
> capturing a thumbnail is as async and cheap as you say below, this might not
> be an issue. mSoftwareLayerClient.getBitmap(); is certainly more costly from
> a UI thread point of view. I guess we need to get the full bitmap anyway for
> "lastscreen". Yuck.
What is costly on the UI thread?
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-01-17 14:02:23 PST
Created attachment 589296 [details] [diff] [review]
patch
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-17 14:09:24 PST
(In reply to Brad Lassey [:blassey] from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > We might run into trouble if the thumbnails use different capture rules. If
> > capturing a thumbnail is as async and cheap as you say below, this might not
> > be an issue. mSoftwareLayerClient.getBitmap(); is certainly more costly from
> > a UI thread point of view. I guess we need to get the full bitmap anyway for
> > "lastscreen". Yuck.
> What is costly on the UI thread?

I guess the creation is off UI thread, but the amount of memory it can eat up can be denied.

Reviewing...
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-17 14:15:43 PST
Comment on attachment 589296 [details] [diff] [review]
patch


>diff -r 1cfc17f1bd13 -r 97935fd604dd mobile/android/base/Tab.java

>-    private Drawable mThumbnail;
>+    protected Drawable mThumbnail = null;

No need. Let's go back to your previous appraoch for the refreshThumbnails, where we don't prioritize

>diff -r 1cfc17f1bd13 -r 97935fd604dd mobile/android/base/Tabs.java

>+    public void refreshThumbnails() {

I didn't want to add a priority here, just avoid work altogether. If we can't, I'd rather go back to your previous appraoch for the refreshThumbnails, where we don't prioritize based on thumbnails being null.

I am worried that we will negatively affect single core devices with this patch, but only testing will tell.

r+ with a revert to previous refreshThumbnail approach
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-01-18 02:52:42 PST
https://hg.mozilla.org/mozilla-central/rev/5f522d344657
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-01-18 08:39:13 PST
Comment on attachment 589296 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
background tabs will not get thumbnails. Also, OOM conditions will not be handled when processing thumbnails in general.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
The main risk is doing the extra work (and extra memory) to produce thumbnails potentially causing OOMs. This is condition is caught in the patch, so hopefully it is mitigated in that way.
Comment 16 Brad Lassey [:blassey] (use needinfo?) 2012-01-20 08:52:40 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/7963e6276292
Comment 17 Paul Feher 2012-03-07 04:13:38 PST
Verified fixed on:
Nightly Fennec 13.0a1 (2012-03-06)
Device: HTC Desire Z
OS: Android 2.3.3

Note You need to log in before you can comment on or make changes to this bug.