Closed Bug 1399200 Opened 7 years ago Closed 7 years ago

Thumbnails stop loading when images redirect or have black background

Categories

(Firefox :: New Tab Page, defect)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: Mardak, Assigned: ahillier)

References

Details

Attachments

(1 file)

Bug 1399200 - Don't stop loading thumbnail when image redirects
59 bytes, text/x-review-board-request
Mardak
: review+
Details
Need to fix up PageThumbUtils.jsm's createImageThumbnailCanvas and backgroundPageThumbsContent.js's _captureCurrentPage from bug 1397390
Comment on attachment 8907199 [details] Bug 1399200 - Don't stop loading thumbnail when image redirects https://reviewboard.mozilla.org/r/178874/#review183962 Needs a test for some failure case. Whatever is easiest is fine… maybe force `isImage` with `data:text/plain,` ? ::: toolkit/components/thumbnails/PageThumbUtils.jsm:131 (Diff revision 1) > image = doc.createElementNS(this.HTML_NAMESPACE, "img"); > - } > - if (!image.complete) { > await new Promise(resolve => { > image.onload = () => resolve(); > image.onerror = () => { throw new Error("Image failed to load"); } Does this throw actually go anywhere? We might need a `reject()` in addition to the `resolve` ::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js:192 (Diff revision 1) > this._loadAboutBlank(); > }); > - }); > + }; > + let win = docShell.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + win.requestIdleCallback(() => doCapture().catch(ex => this.failCurrentCapture(ex))); We'll want a test probably with a 0-width image to make sure the failReason gets propagated correctly. In particular, the method is `this._failCurrentCapture` (with underscore).
Attachment #8907199 - Flags: review?(edilee)
Comment on attachment 8907199 [details] Bug 1399200 - Don't stop loading thumbnail when image redirects https://reviewboard.mozilla.org/r/178874/#review184062 Pretty close. Just a couple issues to clean up ::: toolkit/components/thumbnails/PageThumbUtils.jsm:154 (Diff revision 4) > // estimate that the focus of a tall image is most likely to be near the top > // (e.g., the face of a person). > const canvas = this.createCanvas(window, width, Math.min(height, width)); > - canvas.getContext("2d").drawImage(image, 0, 0, width, height); > + const context = canvas.getContext("2d"); > + context.fillStyle = backgroundColor; > + context.fillRect(0, 0, width, width); We probably want to fill the whole `width` AND `height` not just a square? ::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js:196 (Diff revision 4) > + let win = docShell.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + win.requestIdleCallback(() => doCapture().catch(ex => { > + this._failCurrentCapture(ex.message) > + delete this._currentCapture; > + this._startNextCapture(); Looks like the `delete` and `start` should just move into `_failCurrentCapture` instead of duplicating the code near "BAD_URI"
Attachment #8907199 - Flags: review?(edilee) → review-
See Also: → https://github.com/mozilla/activity-stream/issues/3433
Summary: Thumbnails stop loading when images redirect → Thumbnails stop loading when images redirect or have black background
Comment on attachment 8907199 [details] Bug 1399200 - Don't stop loading thumbnail when image redirects https://reviewboard.mozilla.org/r/178874/#review184062 > We probably want to fill the whole `width` AND `height` not just a square? We crop the image to width x width so this is correct
(In reply to Adam Hillier from comment #7) > We crop the image to width x width so this is correct Ah right. Probably best to pull out `const croppedHeight = Math.min(height, width)` and use that in the 2 places.
Comment on attachment 8907199 [details] Bug 1399200 - Don't stop loading thumbnail when image redirects https://reviewboard.mozilla.org/r/178874/#review184084 Just share the `Math.min(height, width)` to a variable and good to land
Attachment #8907199 - Flags: review?(edilee) → review+
Blocks: 1399226
Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ce559fa15078 Don't stop loading thumbnail when image redirects r=Mardak
https://hg.mozilla.org/mozilla-central/rev/ce559fa15078
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: