[Page Thumbnails] Save screenshots of redirecting sites

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Boriss, Assigned: ttaubert)

Tracking

Trunk
Firefox 14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13 verified)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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]).
(Reporter)

Comment 1

6 years ago
Created attachment 577399 [details]
Screenshot: Example of thumbnails on new tab page rendering partially or not at all
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

6 years ago
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

6 years ago
For thumbnails like that you can use only the logo of the site. Or only the favicon.
Hardware: x86 → All
Target Milestone: Firefox 11 → ---
Version: 11 Branch → Trunk
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.
No longer depends on: 704882
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.
Assignee: nobody → ttaubert
Blocks: 716538
No longer blocks: 455553
Status: NEW → ASSIGNED
Summary: New tab page thumbnails often partial or empty → [Page Thumbnails] Save screenshots of redirecting sites
No longer blocks: 716538
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 :)
Blocks: 497543
Duplicate of this bug: 729148
(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

5 years ago
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
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.
Attachment #605939 - Flags: review?(dao)
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.
Attachment #605939 - Flags: review?(dao) → feedback?(dao)
Duplicate of this bug: 735984
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.
Attachment #605939 - Attachment is obsolete: true
Attachment #605939 - Flags: feedback?(dao)
Attachment #606217 - Flags: review?(dao)
Created attachment 606218 [details] [diff] [review]
patch v2

Sorry, forgot to add a file.
Attachment #606217 - Attachment is obsolete: true
Attachment #606217 - Flags: review?(dao)
Attachment #606218 - Flags: review?(dao)
Dão, can you please review this? We'd really like to have and backport it to Aurora.
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?
(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.
Created attachment 608016 [details] [diff] [review]
patch v3
Attachment #606218 - Attachment is obsolete: true
Attachment #606218 - Flags: review?(dao)
Attachment #608016 - Flags: review?(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?
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.
(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".
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.
Attachment #608016 - Attachment is obsolete: true
Attachment #608016 - Flags: review?(dao)
Attachment #608670 - Flags: review?(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.
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.
Attachment #608670 - Attachment is obsolete: true
Attachment #608670 - Flags: review?(dao)
Attachment #608684 - Flags: review?(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
Attachment #608684 - Flags: review?(dao) → review+
(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
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
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
Attachment #608684 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4173214920c3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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.
Attachment #608684 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/85b2776948f6
status-firefox13: --- → fixed
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.
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.