Last Comment Bug 705911 - [Page Thumbnails] Save screenshots of redirecting sites
: [Page Thumbnails] Save screenshots of redirecting sites
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 729148 735984 (view as bug list)
Depends on:
Blocks: 497543
  Show dependency treegraph
 
Reported: 2011-11-28 15:30 PST by Jennifer Morrow [:Boriss] (UX)
Modified: 2012-04-24 06:41 PDT (History)
13 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Screenshot: Example of thumbnails on new tab page rendering partially or not at all (197.90 KB, image/png)
2011-11-28 15:31 PST, Jennifer Morrow [:Boriss] (UX)
no flags Details
patch v1 (13.60 KB, patch)
2012-03-14 13:59 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (10.44 KB, patch)
2012-03-15 08:04 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (11.10 KB, patch)
2012-03-15 08:05 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (9.79 KB, patch)
2012-03-21 10:35 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v4 (10.19 KB, patch)
2012-03-23 05:43 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v5 (10.35 KB, patch)
2012-03-23 06:49 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jennifer Morrow [:Boriss] (UX) 2011-11-28 15:30:48 PST
Currently, on new tab page in ux-branch, thumbnails often display partially or not at all.  

As :limi notes, this seems to happen especially when sites a) redirect (as in the case of Google Reader in attachment 1 [details] [diff] [review]), or render using XmlHttpRequest calls (as in Google+ and Yammer in attachment 1 [details] [diff] [review]), or when parts of the page uses Flash (as in Gizmodo and v.mozilla.com (Vidyo in attachment 1 [details] [diff] [review]).
Comment 1 Jennifer Morrow [:Boriss] (UX) 2011-11-28 15:31:37 PST
Created attachment 577399 [details]
Screenshot: Example of thumbnails on new tab page rendering partially or not at all
Comment 2 Tim Taubert [:ttaubert] 2011-11-30 05:16:42 PST
Flash (or any type of plugins) is currently disabled when loading the pages in the background to create thumbnails. This is mainly because of performance reasons and to prevent sounds from playing. (We'd need an option to disable <audio> tags, too.)

I guess those pages don't redirect properly but rather via a <meta> tag. So I get the page 'load' event and take the screenshot. Not sure how to find out when's the right time to take the screenshot...

JavaScript is currently also disabled when rendering in the background but could of course be easily enabled again. The difficult part is (like above) figuring out when to take the screenshot/the page has finished loading. IIRC even with JavaScript enabled pages like TBPL have only partial thumbnails.
Comment 3 Chris Lee [:clee] 2011-11-30 16:21:59 PST
Looks like both Chrome and Safari have similar challenges rendering the thumbnails for many less popular sites.  

Is there a way we can handle redirect sites better?  And for thumbnails that are blank, is there a stock image we can fill in so it's not just grayed out?
Comment 4 Tiger.711 2011-12-01 08:46:16 PST
For thumbnails like that you can use only the logo of the site. Or only the favicon.
Comment 5 Girish Sharma [:Optimizer] 2012-01-09 08:06:42 PST
May be now, when the Thumbnail Service used by New Tab Page does not loads the page in background to provide the thumbnail, we can have the thumbnail of pages containing Flash or JavaScript rendered things (like canvas etc) or a redirected page. At least for the New Tab Page's Purpose.
Comment 6 Tim Taubert [:ttaubert] 2012-01-09 10:58:35 PST
Yes, now that thumbnails are being rendered while browsing we capture Flash (plugins in general) and of course every other stuff that is visible on the page. The only thing remaining here is that we need to detect redirects.
Comment 7 Alex Limi (:limi) — Firefox UX Team 2012-02-01 17:05:23 PST
If we just ignore or stop redirects from showing up in the list, shouldn't that solve it? (3 of my 9 thumbnails are currently redirects, often from http to https :)
Comment 8 Tim Taubert [:ttaubert] 2012-02-21 09:20:32 PST
*** Bug 729148 has been marked as a duplicate of this bug. ***
Comment 9 Tim Taubert [:ttaubert] 2012-02-21 10:26:02 PST
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #7)
> If we just ignore or stop redirects from showing up in the list, shouldn't
> that solve it? (3 of my 9 thumbnails are currently redirects, often from
> http to https :)

Actually we already exclude these and show redirect targets only *but* this information gets lost when using Sync. I talked to mak and he said this would need a Sync fix and of course wouldn't fix already existing data. Also, users can drag any kind of link onto the grid so we should be able to capture screenshots for redirects.
Comment 10 Jithin Emmanuel 2012-02-22 23:36:30 PST
There is one firefox addon which does this and their interface looks nice also. https://addons.mozilla.org/en-US/firefox/addon/super-start/?src=cb-dl-featured
Comment 11 Tim Taubert [:ttaubert] 2012-03-14 13:59:47 PDT
Created attachment 605939 [details] [diff] [review]
patch v1

Added a history observer that tracks visits and the pages redirecting to them. When capturing a screenshot of a site that is a redirect target we copy the cache entries and create those for the redirect sources.
Comment 12 Tim Taubert [:ttaubert] 2012-03-14 14:35:42 PDT
Comment on attachment 605939 [details] [diff] [review]
patch v1

Let's change that into a feedback request. There are some things missing:

1) We need to clean the redirect data for private browsing and sanitization.
2) We should maybe have an upper limit for redirect entries.
3) Symlinking cache entries instead of copying would be nice but I'm not sure if that's doable with the current architecture. If we implement that ourselves we'd need some kind of meta data storage.
Comment 13 Girish Sharma [:Optimizer] 2012-03-14 23:14:36 PDT
*** Bug 735984 has been marked as a duplicate of this bug. ***
Comment 14 Tim Taubert [:ttaubert] 2012-03-15 08:04:20 PDT
Created attachment 606217 [details] [diff] [review]
patch v2

So I found a *way* easier approach while fixing another bug. I think this is ready for review.

(In reply to Tim Taubert [:ttaubert] from comment #12)
> 3) Symlinking cache entries instead of copying would be nice but I'm not
> sure if that's doable with the current architecture. If we implement that
> ourselves we'd need some kind of meta data storage.

AFAIK we can't do that with the current architecture but we can file a follow-up for this. Also, we're going to re-evaluate our thumbnail storage backend and symlinks will definitely be a part of this.
Comment 15 Tim Taubert [:ttaubert] 2012-03-15 08:05:51 PDT
Created attachment 606218 [details] [diff] [review]
patch v2

Sorry, forgot to add a file.
Comment 16 Tim Taubert [:ttaubert] 2012-03-21 06:04:03 PDT
Dão, can you please review this? We'd really like to have and backport it to Aurora.
Comment 17 Dão Gottwald [:dao] 2012-03-21 09:41:18 PDT
Comment on attachment 606218 [details] [diff] [review]
patch v2

>   _capture: function Thumbnails_capture(aBrowser) {
>-    if (this._shouldCapture(aBrowser))
>-      this._pageThumbs.captureAndStore(aBrowser);
>+    if (!this._shouldCapture(aBrowser))
>+      return;
>+
>+    this._pageThumbs.captureAndStore(aBrowser, function () {
>+      let channel = aBrowser.docShell.currentDocumentChannel;
>+      let currentURL = aBrowser.currentURI.spec;
>+      let originalURL = channel.originalURI.spec;
>+
>+      // We've been redirected. Create a copy of the current
>+      // thumbnail for the redirect source.
>+      if (currentURL != originalURL)
>+        this._pageThumbsCache.copy(currentURL, originalURL);
>+    }.bind(this));
>   },

I wonder if captureAndStore should behave like this automatically. Any particular reason why you decided to implement it this way?
Comment 18 Tim Taubert [:ttaubert] 2012-03-21 09:56:57 PDT
(In reply to Dão Gottwald [:dao] from comment #17)
> I wonder if captureAndStore should behave like this automatically. Any
> particular reason why you decided to implement it this way?

No, that's kind of legacy code from patch v1 where I had a separate object to do all the redirect tracking and I felt it didn't belong there. But as the solution is now easier and cleaner I also feel like it should be the default behavior. Will upload a new patch.
Comment 19 Tim Taubert [:ttaubert] 2012-03-21 10:35:53 PDT
Created attachment 608016 [details] [diff] [review]
patch v3
Comment 20 Dão Gottwald [:dao] 2012-03-23 05:05:45 PDT
I'm not sure I understand why we need to store the image twice (once for each URI). Is this because of the sync bug?
Comment 21 Tim Taubert [:ttaubert] 2012-03-23 05:13:57 PDT
So first, yes we need it because of bug 559175. Ideally we would have only the redirect targets displayed but the bug doesn't even have a patch or any kind of activity yet and there's no way to fix old sync data - so this will be around for quite some time.

Second, a user can drag any kind of link onto the tab grid, which can redirect to some other site and I think we should display a thumbnail for this, too.

Unfortunately the cache isn't able to create symlinks to cache entries. We're looking into other storage solutions for thumbnails and will consider it for whatever our new storage could look like.
Comment 22 Dão Gottwald [:dao] 2012-03-23 05:26:25 PDT
(In reply to Tim Taubert [:ttaubert] from comment #21)
> So first, yes we need it because of bug 559175. Ideally we would have only
> the redirect targets displayed but the bug doesn't even have a patch or any
> kind of activity yet and there's no way to fix old sync data - so this will
> be around for quite some time.
> 
> Second, a user can drag any kind of link onto the tab grid, which can
> redirect to some other site and I think we should display a thumbnail for
> this, too.

Would you please document this in captureAndStore?

While you're at it, rename "copy" to "_copy".
Comment 23 Tim Taubert [:ttaubert] 2012-03-23 05:43:50 PDT
Created attachment 608670 [details] [diff] [review]
patch v4

(In reply to Dão Gottwald [:dao] from comment #22)
> Would you please document this in captureAndStore?

Done.

> While you're at it, rename "copy" to "_copy".

Done.
Comment 24 Dão Gottwald [:dao] 2012-03-23 05:58:55 PDT
Comment on attachment 608670 [details] [diff] [review]
patch v4

>   /**
>    * Captures a thumbnail for the given browser and stores it to the cache.
>+   * If the given browser's current URL is a redirect target we create a copy
>+   * of its thumbnail and store it under the key of the redirect source
>+   * (the original URL) to be able to provide thumbnails for both of them.

That's not what I meant. I'd like to see the reason for doing this documented. This can be an inline comment, since callers of captureAndStore don't need to know about this.
Comment 25 Tim Taubert [:ttaubert] 2012-03-23 06:49:58 PDT
Created attachment 608684 [details] [diff] [review]
patch v5

(In reply to Dão Gottwald [:dao] from comment #24)
> That's not what I meant. I'd like to see the reason for doing this
> documented. This can be an inline comment, since callers of captureAndStore
> don't need to know about this.

Done.
Comment 26 Dão Gottwald [:dao] 2012-03-23 07:31:16 PDT
Comment on attachment 608684 [details] [diff] [review]
patch v5

>   /**
>+   * Copies an existing cache entry's data to a new cache entry.
>+   * @param aSourceKey The key that contains the data to copy.
>+   * @param aTargetKey The key that will be the copy of aSourceKey's data.
>+   * @param aCallback The function to be called when finished copying (optional).
>+   */
>+  _copy: function Cache_copy(aSourceKey, aTargetKey, aCallback) {
>+    let sourceEntry, targetEntry, waitingCount = 2;
>+
>+    function finish() {
>+      if (sourceEntry)
>+        sourceEntry.close();
>+
>+      if (targetEntry)
>+        targetEntry.close();
>+
>+      if (aCallback)
>+        aCallback();

remove aCallback since it's unused
Comment 27 Tim Taubert [:ttaubert] 2012-03-23 11:42:59 PDT
(In reply to Dão Gottwald [:dao] from comment #26)
> remove aCallback since it's unused

Done.

https://hg.mozilla.org/integration/fx-team/rev/4173214920c3
Comment 28 Tim Taubert [:ttaubert] 2012-03-23 13:56:47 PDT
Comment on attachment 608684 [details] [diff] [review]
patch v5

[Approval Request Comment]
Regression caused by (bug #): no regression
User impact if declined: no newtab thumbnails for redirecting sites (includes popular sites such as gmail)
Testing completed (on m-c, etc.): will merge to m-c today
Risk to taking this patch (and alternatives if risky): low risk, this patch doesn't change existing behavior, it only creates copies of thumbnails and contains a test for the new behavior
String changes made by this patch: none
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-23 16:25:10 PDT
https://hg.mozilla.org/mozilla-central/rev/4173214920c3
Comment 30 Alex Keybl [:akeybl] 2012-03-26 15:28:33 PDT
Comment on attachment 608684 [details] [diff] [review]
patch v5

[Triage Comment]
This is in support of FF13's new new tab page. Given where we are in the schedule, approved for Aurora 13.
Comment 31 Tim Taubert [:ttaubert] 2012-03-29 02:29:57 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/85b2776948f6
Comment 32 Virgil Dicu [:virgil] [QA] 2012-04-24 06:41:39 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120423 Firefox/13.0a2
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120424 Firefox/13.0a2
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120423 Firefox/13.0a2

verified with the following steps in Firefox 13 Aurora:
1. Log in to gmail.
2. Open a new tab.
3. The gmail thumbnails are now displayed correctly.

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