705911 - [Page Thumbnails] Save screenshots of redirecting sites

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

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)]

Attachment: Screenshot: Example of thumbnails on new tab page rendering partially or not at all

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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]]

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]

For thumbnails like that you can use only the logo of the site. Or only the favicon.

Comment 5

[Girish Sharma [:Optimizer]]

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] (inactive)]

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]

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 9

[Tim Taubert [:ttaubert] (inactive)]

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

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] (inactive)]

Attachment: 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] (inactive)]

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 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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] (inactive)]

Attachment: patch v2

Sorry, forgot to add a file.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Dão, can you please review this? We'd really like to have and backport it to Aurora.

Comment 17

[Dão Gottwald [:dao]]

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] (inactive)]

(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] (inactive)]

Attachment: patch v3

Comment 20

[Dão Gottwald [:dao]]

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] (inactive)]

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]]

(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] (inactive)]

Attachment: 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]]

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] (inactive)]

Attachment: 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]]

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] (inactive)]

(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] (inactive)]

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: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/4173214920c3

Comment 30

[Alex Keybl [:akeybl]]

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] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/85b2776948f6

Comment 32

[Virgil Dicu [:virgil] [QA]]

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.