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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Mardak, Assigned: ahillier)

References

Details

Attachments

(1 file)

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