1528108 - Drop support for PageThumbUtils.createCanvas with null window

Status: RESOLVED FIXED

(Firefox :: Shell Integration, enhancement, P2)

Description

[:Felipe Gomes (needinfo for replies!)]

PageThumbUtils.createCanvas takes a window as a parameter, and if one is not given, it falls back to the hidden window.

The only consumer of the API that does this is the WindowsPreviewPerTab code. But it doesn't need to do this, because it has a window readily available.

So we should make it pass the window, and drop support for the hidden window fallback from the PageThumbs code.

Comment 1

[Monika Manuela Hengki]

Hi, I'd like to work on this.

Comment 2

[Mark Banner (:standard8)]

Thanks Monika, I've assigned you to it.

Comment 3

[Monika Manuela Hengki]

Attachment: Bug 1528108 - Drop support for PageThumbUtils.createCanvas with null window

Comment 4

[Mark Banner (:standard8)]

Jim, I believe you did some of the work on PageThumbs for making it work with E10s.

The patch has pointed out that PageThumbs.captureToBlob and PageThumbs._captureToCanvas are both currently using aBrowser.contentWindow for the window to attach to - however, with e10s that is now null for remote tabs.

At the moment, these are falling back to using the hidden window as a result of the null.

Do you think that we could replace this with just the window that the browser is in (i.e. a chrome window), or should this really be a content window and we're going to need to do some child process handling?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Mark Banner (:standard8) from comment #4)

Jim, I believe you did some of the work on PageThumbs for making it work
with E10s.

The patch has pointed out that
[PageThumbs.captureToBlob](https://searchfox.org/mozilla-central/rev/
2f1020dc4176d38dd5f3d0496f3c46849862ee0b/toolkit/components/thumbnails/
PageThumbs.jsm#166) and
[PageThumbs._captureToCanvas](https://searchfox.org/mozilla-central/rev/
2f1020dc4176d38dd5f3d0496f3c46849862ee0b/toolkit/components/thumbnails/
PageThumbs.jsm#264) are both currently using aBrowser.contentWindow for
the window to attach to - however, with e10s that is now null for remote
tabs.

We added e10s safe code paths for thumbnails. For example -

https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/toolkit/components/thumbnails/PageThumbs.jsm#338

https://searchfox.org/mozilla-central/source/toolkit/actors/ThumbnailsChild.jsm

This should all be working.

At the moment, these are falling back to using the hidden window as a result
of the null.

Do you think that we could replace this with just the window that the
browser is in (i.e. a chrome window), or should this really be a content
window and we're going to need to do some child process handling?

These thumbnails show up in various user facing areas, newtabpage and windows taskbar thumbnails. Doubt front end UX would approve cluttering these with chrome.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Oh I don't think this changes anything in what gets drawn. This window is just the "host" window to create the <canvas> element. But the actual call to canvas.drawWindow takes its own parameter of what to draw.
Also, in this case, this code is being used to draw the whole browser window, not just the content area, since this is the screen-wide preview when you hover the mouse on a thumbnail preview.

So I don't think this patch is changing any behavior, other than creating the <canvas> element in the browser.xul window instead of the hidden window.

Comment 7

[Mark Banner (:standard8)]

Ok, so the thunbnails are correctly captured, this is just about which window the canvases are attached to for creating the thumbnails themselves.

Since the canvas elements aren't actually inserted to the window, they're therefore hidden and we can attach them to any window.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77e55a8270d1
Drop support for PageThumbUtils.createCanvas with null window r=Standard8

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/77e55a8270d1

Comment 10

[jaromanda]

I suspect this code change has broken taskbar previews in Windows 10 as I'm now getting

TypeError: doc is undefined PageThumbUtils.jsm:34:18

and

TypeError: this.canvasPreview is undefinedWindowsPreviewPerTab.jsm:183:5

When hovering over firefox on the Taskbar

Comment 11

[Mark Banner (:standard8)]

(In reply to jaromanda from comment #10)

I suspect this code change has broken taskbar previews in Windows 10 as I'm now getting

I've just tried this on today's nightly (26 March, 68.0a1), and it is all working fine for me.

If you can still see it, please can you file a bug with as much detail for reproducing as possible, e.g. what you do from startup.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

I was able to reproduce it. It's a non-default feature, to reproduce it you need to check "Show tab previews in the Windows Taskbar" in about:preferences (a.k.a. browser.taskbar.previews.enable). It makes us generate one preview per tab, as opposed to the default of one preview per window.

Comment 13

[Mark Banner (:standard8)]

(In reply to :Felipe Gomes (needinfo me!) from comment #12)

I was able to reproduce it. It's a non-default feature, to reproduce it you need to check "Show tab previews in the Windows Taskbar" in about:preferences (a.k.a. browser.taskbar.previews.enable). It makes us generate one preview per tab, as opposed to the default of one preview per window.

Thank you for checking. I didn't realise that was an option.