Thumbnails stop loading when images redirect or have black background

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Activity Streams: Newtab
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Mardak, Assigned: Adam Hillier)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
Need to fix up PageThumbUtils.jsm's createImageThumbnailCanvas and backgroundPageThumbsContent.js's _captureCurrentPage from bug 1397390
Comment hidden (mozreview-request)
(Reporter)

Comment 2

3 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

3 months ago
mozreview-review
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-
(Reporter)

Updated

3 months ago
Summary: Thumbnails stop loading when images redirect → Thumbnails stop loading when images redirect or have black background
(Assignee)

Comment 7

3 months ago
mozreview-review-reply
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
Comment hidden (mozreview-request)
(Reporter)

Comment 9

3 months ago
(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.
(Reporter)

Comment 10

3 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

3 months ago
Blocks: 1399226

Comment 14

3 months ago
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
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.