Last Comment Bug 726272 - [Page Thumbnails] don't capture error pages
: [Page Thumbnails] don't capture error pages
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
: 736054 (view as bug list)
Depends on: 805338
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-11 00:33 PST by Chris Peterson [:cpeterson]
Modified: 2012-10-24 22:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
patch v1 (WIP) (988 bytes, patch)
2012-03-15 07:42 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v2 (2.52 KB, patch)
2012-03-15 08:29 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dietrich: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-02-11 00:33:51 PST
AR:
One of my New Tab thumbnails (which happens to be http://mail.google.com) is labeled "Problem loading page" (Firefox's "server not found" error page).

ER:
The New Tab should not show "Problem loading page" thumbnails. It could substitute the thumbnail's hostname or just omit the thumbnail entirely until a real page title is found.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-15 07:39:27 PDT
*** Bug 736054 has been marked as a duplicate of this bug. ***
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-15 07:42:44 PDT
Created attachment 606209 [details] [diff] [review]
patch v1 (WIP)

Needs a testcase.
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-15 08:29:04 PDT
Created attachment 606226 [details] [diff] [review]
patch v2

Patch with a test. I accessed gBrowserThumbnails._shouldCapture() directly because it's really hard to tell whether a thumbnails has *not* been captured.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-15 11:00:59 PDT
Can you instead check channel instanceof nsIHttpChannel && channel.requestSucceeded ?
Comment 5 Dietrich Ayala (:dietrich) 2012-03-15 14:44:25 PDT
Comment on attachment 606226 [details] [diff] [review]
patch v2

Review of attachment 606226 [details] [diff] [review]:
-----------------------------------------------------------------

what gavin said. is a more explicit test that way.
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-16 03:30:40 PDT
So (channel.requestSucceeded == true) and (request.status == 0) because that refers to the internal load of "about:neterror". Should we stick with the old approach or is there any other solution?
Comment 7 Dietrich Ayala (:dietrich) 2012-03-16 07:42:19 PDT
Comment on attachment 606226 [details] [diff] [review]
patch v2

Review of attachment 606226 [details] [diff] [review]:
-----------------------------------------------------------------

old approach is fine for now. i can't think of any other redirect-to-about scenarios, so let's roll with this and update as needed if bugs start flowing in.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-16 08:16:36 PDT
https://hg.mozilla.org/integration/fx-team/rev/657e7edcb91c
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-17 01:28:31 PDT
Comment on attachment 606226 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: error pages can overwrite thumbnails in cases of network outage
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low risk, very small patch with a test
String changes made by this patch: none
Comment 11 Siddhartha Dugar [:sdrocking] 2012-03-17 23:10:57 PDT
This makes it impossible to have thumbnails for pages like about:config (manually pinned). Is that acceptable?
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-18 00:44:16 PDT
We've never taken screenshots of about: pages (this bug is only about error pages). But that doesn't mean we couldn't.
Comment 13 Siddhartha Dugar [:sdrocking] 2012-03-18 00:49:39 PDT
I thought they were missing because of this patch. I was wrong.

The problem happens in Aurora also, which I believe doesn't contain this patch. Please create a followup bug to show thumbnails for about:config and similar pages.
Comment 14 Alex Keybl [:akeybl] 2012-03-20 13:29:48 PDT
Comment on attachment 606226 [details] [diff] [review]
patch v2

[Triage Comment]
Low risk patch in support of correctness in a new feature. Approved for Aurora 13.
Comment 15 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-22 01:57:58 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/df303dada20e
Comment 16 Paul Silaghi, QA [:pauly] 2012-05-16 08:00:24 PDT
Page Thumbnails don't capture error pages.
Verified fixed on FF 13b3:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

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