Closed Bug 1397390 Opened 7 years ago Closed 7 years ago

Support better thumbnails for image URLs

Categories

(Firefox :: New Tab Page, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: k88hudson, Assigned: ahillier)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Right now our thumbnails code doesn't handle image URLs very gracefully. We'd like to use thumbnails to cache preview images in Activity Stream, so let's add some code to handle drawing images directly to a canvas and specifying an appropriate resizing.
Assignee: nobody → ahillier
Attachment #8905211 - Flags: review?(edilee)
Comment on attachment 8905211 [details]
Bug 1397390 - Support better thumbnails for image urls

https://reviewboard.mozilla.org/r/176982/#review182046

::: toolkit/components/thumbnails/PageThumbUtils.jsm:126
(Diff revision 1)
> +    const [imageWidth, imageHeight] = await new Promise((resolve, reject) => {
> +      image.onload = () => (image.naturalWidth > 0 && image.naturalHeight > 0) ?
> +        resolve([image.naturalWidth, image.naturalHeight]) :
> +        reject(new Error("Image has zero dimension"));
> +      image.onerror = () => reject(new Error("Image failed to load"));
> +      image.src = imageUrl;

This seems a bit roundabout to wait for backgroundPageThumbsContent.js to call here after having already loaded the image only to yet again load the image?

Can we just reuse the existing `<img>` that should be the only child of the page?

::: toolkit/components/thumbnails/PageThumbUtils.jsm:135
(Diff revision 1)
> +    const height = width * imageHeight / imageWidth;
> +    const canvas = doc.createElementNS(this.HTML_NAMESPACE, "canvas");
> +    canvas.width = width;
> +    canvas.height = height;
> +    canvas.mozOpaque = true;
> +    canvas.imageSmoothingEnabled = true;

Seems like this canvas creation part could pretty much just call `createCanvas`?
adw, any thoughts on if there's a better way to just take an image url to save the thumbnail?
Flags: needinfo?(adw)
Comment on attachment 8905211 [details]
Bug 1397390 - Support better thumbnails for image urls

https://reviewboard.mozilla.org/r/176982/#review182046

> This seems a bit roundabout to wait for backgroundPageThumbsContent.js to call here after having already loaded the image only to yet again load the image?
> 
> Can we just reuse the existing `<img>` that should be the only child of the page?

I thought about doing that, Kate and I discussed it. Doing it this way gives a little more flexibility as it means that if in the future we have a url that we know points to an image we can use this function directly. And as the browser just loaded the image it should in all circumstances be cached so it's not an extra network request. But I agree it's a little odd so I am happy to do reuse the <img> if you'd prefer.
Attached image 224px vs 448px
Looks like we might want to take the screenshot at 2x resolution like we do for page thumbnails. The left one is 224px width vs 448px on right.
(In reply to Adam Hillier from comment #4)
> in the future we have a url that we know points to an image we can use this function directly
Well, the current use case already has an <img>, so probably better to support a known case than the possible future case. If that "thumbnail for url" functionality is needed, we can update `createImageThumbnailCanvas` to do `createElement` or just `new Image()`. (Or a caller could possibly just as easily do `new Image()`)

> the browser just loaded the image it should in all circumstances be cached
> so it's not an extra network request.
I'm not so sure as I believe the background page thumb tries to have a special context separate from normal user browsing behavior. Even if necko is doing something independent of the contexts, it probably follows various no-cache headers, although then again, the thumbnails we want for activity stream probably are made to be cache-able. Would be nice to check, but I don't think it's necessary for fixing this bug.
(In reply to Ed Lee :Mardak from comment #3)
> adw, any thoughts on if there's a better way to just take an image url to
> save the thumbnail?

A DOM/Gecko person would know better than me, but this looks useful: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawImage

You need an <img> first, but from the bug comments it sounds like you have one already?

The reason backgroundPageThumbs is "background" is that it loads pages you want to capture but that aren't currently loaded by the user.  (It uses a remote browser to do so, but that's really just an implementation detail and isn't the meaning of "background" in this case.)  It should just work for pages that are actually images, I think?

If all you have are generic URLs, then using backgroundPageThumbs to capture them is fine, that's what it's for.

If you have pages or images that are already loaded, then it seems like you could probably just use one of the existing PageThumbs methods, or add one that does specifically what you need.

If you have URLs that you know are images, then if it were me I'd probably try that canvasContext.drawImage() above first, by adding a new PageThumbs method if necessary.  The only thing is that loading those images potentially might jank up the browser, at least more so than loading them through backgroundPageThumbs's remote browser?  idk
Flags: needinfo?(adw)
(In reply to Ed Lee :Mardak from comment #6)
> I'm not so sure as I believe the background page thumb tries to have a
> special context separate from normal user browsing behavior.

This is true fwiw, "tries" being the operative word.  (There've been problems in the past where it hasn't been separate enough.)
(In reply to Drew Willcoxon :adw from comment #7)
> If you have URLs that you know are images, then if it were me I'd probably
> try that canvasContext.drawImage() above first, by adding a new PageThumbs
> method if necessary.  The only thing is that loading those images
> potentially might jank up the browser, at least more so than loading them
> through backgroundPageThumbs's remote browser?  idk
Right. I was originally thinking, why not skip the whole queue thing and load up an Image() if we have the url, but yeah in the context of activity stream, we might be requesting several at a time in the background from a preloaded browser right when the user is trying to browse to something, so maybe doing the usual queue would be better.

Just to be clear, the "already have <img>" is from the backgroundPageThumbsContent context, where we told it to navigate to the image url, and the Firefox behavior is to render a html page with an <img> as the body's only child.

So here, we *are* requesting the thumbnail for an image url with BackgroundPageThumbs.capture*(imageUrl). Whether we should additinoally expose PageThumbUtils.createImageThumbnailCanvas(imageUrl vs Image), it now sounds like more of the latter as the main public interface already allows less-janky capturing of image urls, so this more internal method can be better optimized to just reuse the existing Image.
We were thinking about some optimizations like you described (e.g. bypass the unnecessary steps of loading the entire browser, etc.) however, I feel like reusing the existing pathway for screenshots is less risky for now, since that's what we would be using as a fallback for highlights anyway and the queuing/caching logic is already written. I'd definitely be down for some work to optimize the screenshot stuff as a follow-up in the future
Comment on attachment 8905211 [details]
Bug 1397390 - Support better thumbnails for image urls

https://reviewboard.mozilla.org/r/176982/#review182426

::: toolkit/components/thumbnails/PageThumbUtils.jsm:127
(Diff revisions 1 - 2)
> +  async createImageThumbnailCanvas(window, url, targetWidth = 448) {
> +    // 224px is the width of cards in ActivityStream; capture thumbnails at 2x
>      const doc = (window || Services.appShell.hiddenDOMWindow).document;
> -    const image = doc.createElementNS(this.HTML_NAMESPACE, "img");
> -    const [imageWidth, imageHeight] = await new Promise((resolve, reject) => {
> -      image.onload = () => (image.naturalWidth > 0 && image.naturalHeight > 0) ?
> +
> +    let image = doc.querySelector("img");
> +    if (!image || image.src !== url || !image.complete) {

You don't need to create a new image element if image.complete is false, you could just attach the onload listener to it.
Comment on attachment 8905211 [details]
Bug 1397390 - Support better thumbnails for image urls

https://reviewboard.mozilla.org/r/176982/#review182520

Looks good to me, what do you think Mardak?
Attachment #8905211 - Flags: review?(khudson) → review+
Comment on attachment 8905211 [details]
Bug 1397390 - Support better thumbnails for image urls

https://reviewboard.mozilla.org/r/176982/#review182522
Attachment #8905211 - Flags: review?(edilee) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4a955a640a52
Support better thumbnails for image urls r=k88hudson,Mardak
Looks like `Error` typo:
      throw new error("Image has zero dimension");
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/04e5cb3e68b4
Support better thumbnails for image urls r=k88hudson,Mardak
https://hg.mozilla.org/mozilla-central/rev/04e5cb3e68b4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1399200
Flags: needinfo?(ahillier)
Component: Activity Streams: Newtab → New Tab Page
Blocks: 1706449
You need to log in before you can comment on or make changes to this bug.