Canvas.drawWindow requires contentWindow as the first argument.

Seems to work already!

I verified that this is working on larch by browsing several canvas demos. If there are issues more specific than "canvas doesn't work at all", let's file them as separate bugs.

(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #2) > I verified that this is working on larch by browsing several canvas demos. AFAIK drawWindow (that's what this bug is really about, see comment 0) isn't exposed to web content and not used on the web.

Could you explain this bug more, Dão? I'm having trouble finding the code that you're talking about? Is it related to Panorama?

http://hg.mozilla.org/mozilla-central/annotate/382f676d0ed9/dom/webidl/CanvasRenderingContext2D.webidl#l214 Any chrome code using that method currently needs to pass in the content window as the first element. Maybe this works with CPOWs or something, but I imagine performance may be suboptimal.

So you're saying that chrome code wants to draw directly into a content window's canvas element? When do we do that?

No, this is about chrome code calling drawWindow (on a chrome window's canvas element) with a content window as the first argument.

Ah, I see. There's some interesting code in metro that looks related to this. Maybe we can do something like that. http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.js#1555

I think this might be useful for bug 863512.

This blocks running any reftest in e10s, right? As reftest inherently relies on drawWindows.

I just wanted to remark that the asyncDrawXULElement call that I alluded to in comment 8 isn't actually implemented. http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#3730 I think we'll just have to use drawWindow in the child and then pack up the resulting image and send it over IPC.

Jim, can you please back out this changeset when you land the fix for this? This changeset is a workaround for this bug. Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/e024c7a1fef9

I don't think the context needs to support an e10s compat drawWindow. We just need a way to ask remote browsers for their thumbnails asynchronously. Then we can adapt existing consumers to use the new method of retrieval. Working on an experimental fix for that currently.

If we tackle this at the PageThumbs level, blocked bug 863512 is probably a dupe of this.

One thing I think we're going to have to do here is deprecate PageThumbs captureToCanvas. We can touch up our own callers, but there are as couple addons that use this that will need to be updated. captureToCanvas would continue to work in-process, but would return a blank canvas and warn for e10s windows. Also one caller I'm not sure how to fix (yet) is tab drag and drop, but I haven't really had a chance to dig into it.

An alternative here would be to make drawWindow work with e10s, but that would mean all these captureToCanvas callers would block on content processes. IMO that's a bad idea, so I'd like to go the route of deprecating the sync api instead. Dão, do you see any issues with this?

(In reply to Jim Mathies [:jimm] from comment #15) > One thing I think we're going to have to do here is deprecate PageThumbs > captureToCanvas. We can touch up our own callers, but there are as couple > addons that use this that will need to be updated. captureToCanvas would > continue to work in-process, but would return a blank canvas and warn for > e10s windows. If it's a big problem, we can write a shim for this one that waits on the child using a CPOW.

(In reply to Bill McCloskey (:billm) from comment #17) > (In reply to Jim Mathies [:jimm] from comment #15) > > One thing I think we're going to have to do here is deprecate PageThumbs > > captureToCanvas. We can touch up our own callers, but there are as couple > > addons that use this that will need to be updated. captureToCanvas would > > continue to work in-process, but would return a blank canvas and warn for > > e10s windows. > > If it's a big problem, we can write a shim for this one that waits on the > child using a CPOW. https://mxr.mozilla.org/addons/search?string=captureToCanvas It's not, two callers plus our own b2g emulators.

(In reply to Jim Mathies [:jimm] from comment #15) > One thing I think we're going to have to do here is deprecate PageThumbs > captureToCanvas. We could change the semantics of captureToCanvas and let it fill the canvas asynchronously, right? This would work for most consumers, I think, but it would probably break the drag-and-drop case where I think the image is needed synchronously, so we'd need a separate solution for that.

(In reply to Dão Gottwald [:dao] from comment #19) > (In reply to Jim Mathies [:jimm] from comment #15) > > One thing I think we're going to have to do here is deprecate PageThumbs > > captureToCanvas. > > We could change the semantics of captureToCanvas and let it fill the canvas > asynchronously, right? This would work for most consumers, I think, but it > would probably break the drag-and-drop case where I think the image is > needed synchronously, so we'd need a separate solution for that. Yes. I have two concerns there, consumers that might grab a thumbnail and then paint additional content on top would see unexplained behavior. Also, tracking callers thumbs in PageThumbs seems like a pita. A new api that is explicitly async (asyncCaptureToCanvas?) that takes a callback or returns a Promise would avoid this. The d&d thing is going to be tricky. We only use it for tab drag so I'm tempted to just turn it off for e10s windows. Maybe that's a temporary solution, or maybe we make it permanent, I'm not sure. Like I mentioned I haven't had a chance to dig into it much yet.

(In reply to Jim Mathies [:jimm] from comment #20) > > We could change the semantics of captureToCanvas and let it fill the canvas > > asynchronously, right? This would work for most consumers, I think, but it > > would probably break the drag-and-drop case where I think the image is > > needed synchronously, so we'd need a separate solution for that. > > Yes. I have two concerns there, consumers that might grab a thumbnail and > then paint additional content on top would see unexplained behavior. Also, > tracking callers thumbs in PageThumbs seems like a pita. > > A new api that is explicitly async (asyncCaptureToCanvas?) that takes a > callback or returns a Promise would avoid this. What do you mean by "tracking callers thumbs"? You just need to keep a reference to the canvas element so you can draw to it, right? And you need that either way, no matter if call back, return a promise or do nothing once you're done drawing. I still think we can adjust captureToCanvas to that end, since it will just work for most consumers. Where it doesn't work people will notice and can add their callback function or use the promise or whichever functionality we add there. > The d&d thing is going to be tricky. We only use it for tab drag so I'm > tempted to just turn it off for e10s windows. Maybe that's a temporary > solution, or maybe we make it permanent, I'm not sure. Like I mentioned I > haven't had a chance to dig into it much yet. Maybe dt.setDragImage can be called asynchronously? I'm not sure this needs to happen directly in the dragstart handler.

(In reply to Dão Gottwald [:dao] from comment #21) > (In reply to Jim Mathies [:jimm] from comment #20) > > > We could change the semantics of captureToCanvas and let it fill the canvas > > > asynchronously, right? This would work for most consumers, I think, but it > > > would probably break the drag-and-drop case where I think the image is > > > needed synchronously, so we'd need a separate solution for that. > > > > Yes. I have two concerns there, consumers that might grab a thumbnail and > > then paint additional content on top would see unexplained behavior. Also, > > tracking callers thumbs in PageThumbs seems like a pita. > > > > A new api that is explicitly async (asyncCaptureToCanvas?) that takes a > > callback or returns a Promise would avoid this. > > What do you mean by "tracking callers thumbs"? You just need to keep a > reference to the canvas element so you can draw to it, right? And you need > that either way, no matter if call back, return a promise or do nothing once > you're done drawing. I was thinking the callback could return a canvas in the async version so we wouldn't have to track canvas objects in PageThumbs. > I still think we can adjust captureToCanvas to that end, since it will just > work for most consumers. Where it doesn't work people will notice and can > add their callback function or use the promise or whichever functionality we > add there. Ok, I'll update captureToCanvas to draw async. I'll probably go ahead and implement asyncCaptureToCanvas for completeness. :) We can decide if we want to land it during reviews.

This bug should be fixed for the e10s M2 milestone.

Brad says we don't need to block M2 on page thumbnails.

Noting that https://bugzilla.mozilla.org/show_bug.cgi?id=1055507#c16 mentions possibly backing out the patch for 1055507 when bug 698371 is fixed.

There's a couple open issues here - - with multiple requests to requestThumbnail, callers might get the wrong notification. - I'm not a fan of the cropped dims that need to get sent over to the child still.

- moved remote-browser bits over into PageThumbs. Dealing with some issues with Services.appShell.hiddenDOMWindow not loading content in this. I might have to create the canvas and image using something else.

This is working well with ctrl-tab. I need to check some of the other callers.

- cleanup

Looks like BackgroundPageThumbs needs some additional work here. We create a remote <browser> to load pages in to, then create a PageThumbs as a utility over in the child process for snapping thumbnails, once we have a thumbnail we send it back to the parent as a blob. backgroundPageThumbsContent.js should be able to do this work on its own without using PageThumbs, which is incompatible now that it takes a browser vs. a window as the main param.

- split common routines out into PageThumbsUtils - use PageThumbsUtils in backgroundPageThumbsContent instead of PageThumbs.

This is looking pretty good for in-process. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5de4d5d00dd Currently working on toolkit thumbnail tests with e10s. Most run fine, a few are broken due to other e10s issues. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d116f2c7fff

Comment on attachment 8496440 [details] [diff] [review] pt 1 - move common PageThumbs code to PageThumbsUtils I'll add more to PageThumbsUtils in the next patch.

Comment on attachment 8496445 [details] [diff] [review] pt 2 - PageThumbs e10s compat work Review of attachment 8496445 [details] [diff] [review]: ----------------------------------------------------------------- Bulk of the work: - cleanup PageThumbs a bit - add async support to the PageThumbs apis that don't currently support it - add remote browser support

Can you rename PageThumbsUtils to PageThumbUtils?

(In reply to Dão Gottwald [:dao] from comment #46) > Can you rename PageThumbsUtils to PageThumbUtils? Sure.

Dão - review ping? :)

Comment on attachment 8496440 [details] [diff] [review] pt 1 - move common PageThumbs code to PageThumbsUtils >+ let sbWidth = {__exposedProps__: {'value': 'rw'}}, >+ sbHeight = {__exposedProps__: {'value': 'rw'}}; nit: use double quotes >+ Cu.reportError("Unable to get scrollbar size in _determineCropSize."); s/_determineCropSize/determineCropSize/

Comment on attachment 8496445 [details] [diff] [review] pt 2 - PageThumbs e10s compat work >--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 11:34:37 2014 -0500 >+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 12:57:27 2014 -0500 >-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; >-const HTML_NS = "http://www.w3.org/1999/xhtml"; >- > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > Cu.import("resource://gre/modules/PageThumbs.jsm"); >+Cu.import("resource://gre/modules/PageThumbsUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); >- let iframe = hostWindow.document.createElementNS(HTML_NS, "iframe"); >+ let iframe = >+ hostWindow.document.createElementNS(PageThumbsUtils.HTML_NAMESPACE, >+ "iframe"); >- let browser = this._parentWin.document.createElementNS(XUL_NS, "browser"); >+ let browser = >+ this._parentWin.document.createElementNS(PageThumbsUtils.XUL_NAMESPACE, >+ "browser"); I'm not convinced that these changes are useful... >- * Captures a thumbnail for the given window. >- * @param aWindow The DOM window to capture a thumbnail from. >+ * Asynchronously returns a thumbnail as a stream for the given >+ * window. >+ * >+ * @param aBrowser The <browser> to capture a thumbnail from. > * @param aCallback The function to be called when the thumbnail has been > * captured. The first argument will be the data stream > * containing the image data. > */ >- capture: function PageThumbs_capture(aWindow, aCallback) { >+ captureToStream: function PageThumbs_captureToStream(aBrowser, aCallback) { Why did you rename this function when its semantics didn't change? > captureAndStore: function PageThumbs_captureAndStore(aBrowser, aCallback) { > if (!this._prefEnabled()) { > return; > } > >+ if (aBrowser.isRemoteBrowser) { >+ // e10s - we need channel information. bug ??? >+ this._captureAndStoreHelper(aBrowser, aBrowser.currentURI.spec, false, aCallback); >+ } else { >+ let channel = aBrowser.docShell.currentDocumentChannel; >+ let originalURL = channel.originalURI.spec; >+ // see if this was an error response. >+ let wasError = PageThumbsUtils.isChannelErrorResponse(channel); >+ this._captureAndStoreHelper(aBrowser, originalURL, wasError, aCallback); >+ } >+ }, >+ >+ _captureAndStoreHelper: function (aBrowser, aOriginalURL, aWasError, aCallback) { > let url = aBrowser.currentURI.spec; >- let channel = aBrowser.docShell.currentDocumentChannel; >- let originalURL = channel.originalURI.spec; >- >- // see if this was an error response. >- let wasError = this._isChannelErrorResponse(channel); "_captureAndStoreHelper" doesn't say much about what this method is doing. It doesn't look like captureAndStore needs to be split into two methods in the first place, you can just keep using local variables. >- /** >- * Given a channel, returns true if it should be considered an "error >- * response", false otherwise. >- */ >- _isChannelErrorResponse: function(channel) { >- // No valid document channel sounds like an error to me! >- if (!channel) >- return true; >- if (!(channel instanceof Ci.nsIHttpChannel)) >- // it might be FTP etc, so assume it's ok. >- return false; >- try { >- return !channel.requestSucceeded; >- } catch (_) { >- // not being able to determine success is surely failure! >- return true; >- } >- }, Why did you move this? >@@ -42,17 +40,17 @@ const backgroundPageThumbsContent = { > addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_WINDOW); > }, > > observe: function (subj, topic, data) { > // Arrange to prevent (most) popup dialogs for this window - popups done > // in the parent (eg, auth) aren't prevented, but alert() etc are. > // disableDialogs only works on the current inner window, so it has > // to be called every page load, but before scripts run. >- if (subj == content.document) { >+ if (content && subj == content.document) { > content. > QueryInterface(Ci.nsIInterfaceRequestor). > getInterface(Ci.nsIDOMWindowUtils). > disableDialogs(); > } > }, When would content be null? >- >- canvas.toBlob(blob => { >- capture.imageBlob = blob; >+ canvas.toBlob(function (aBlob) { >+ capture.imageBlob = aBlob; > // Load about:blank to finish the capture and wait for onStateChange. > this._loadAboutBlank(); >- }); >+ }.bind(this)); Why this change?

(In reply to Dão Gottwald [:dao] from comment #50) > Comment on attachment 8496445 [details] [diff] [review] > pt 2 - PageThumbs e10s compat work > > >--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 11:34:37 2014 -0500 > >+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 12:57:27 2014 -0500 > > >-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > >-const HTML_NS = "http://www.w3.org/1999/xhtml"; > >- > > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > > > Cu.import("resource://gre/modules/PageThumbs.jsm"); > >+Cu.import("resource://gre/modules/PageThumbsUtils.jsm"); > > Cu.import("resource://gre/modules/Services.jsm"); > > >- let iframe = hostWindow.document.createElementNS(HTML_NS, "iframe"); > >+ let iframe = > >+ hostWindow.document.createElementNS(PageThumbsUtils.HTML_NAMESPACE, > >+ "iframe"); > > >- let browser = this._parentWin.document.createElementNS(XUL_NS, "browser"); > >+ let browser = > >+ this._parentWin.document.createElementNS(PageThumbsUtils.XUL_NAMESPACE, > >+ "browser"); > > I'm not convinced that these changes are useful... I was just trying to create a useful utils module for this code? Eventually I think we'll want to access PageThumbUtils from content as well. > >- * Captures a thumbnail for the given window. > >- * @param aWindow The DOM window to capture a thumbnail from. > >+ * Asynchronously returns a thumbnail as a stream for the given > >+ * window. > >+ * > >+ * @param aBrowser The <browser> to capture a thumbnail from. > > * @param aCallback The function to be called when the thumbnail has been > > * captured. The first argument will be the data stream > > * containing the image data. > > */ > >- capture: function PageThumbs_capture(aWindow, aCallback) { > >+ captureToStream: function PageThumbs_captureToStream(aBrowser, aCallback) { > > Why did you rename this function when its semantics didn't change? This module has pretty good naming on the public interface: captureToBlob, captureToCanvas, captureAndStore, .. but it also has this odd call named 'capture'. There are no callers of capture afaict. The method returns a stream asynchronously. So I renamed it to something more appropriate and matching the other apis. We could remove this as well, but it seems useful. > "_captureAndStoreHelper" doesn't say much about what this method is doing. > It doesn't look like captureAndStore needs to be split into two methods in > the first place, you can just keep using local variables. sure, will update. > >- /** > >- * Given a channel, returns true if it should be considered an "error > >- * response", false otherwise. > >- */ > >- _isChannelErrorResponse: function(channel) { > >- // No valid document channel sounds like an error to me! > >- if (!channel) > >- return true; > >- if (!(channel instanceof Ci.nsIHttpChannel)) > >- // it might be FTP etc, so assume it's ok. > >- return false; > >- try { > >- return !channel.requestSucceeded; > >- } catch (_) { > >- // not being able to determine success is surely failure! > >- return true; > >- } > >- }, > > Why did you move this? Hmm, looks like I moved that when I was messing around with channel information. I ran into issues there however. I think this change can come out. > >@@ -42,17 +40,17 @@ const backgroundPageThumbsContent = { > > addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_WINDOW); > > }, > > > > observe: function (subj, topic, data) { > > // Arrange to prevent (most) popup dialogs for this window - popups done > > // in the parent (eg, auth) aren't prevented, but alert() etc are. > > // disableDialogs only works on the current inner window, so it has > > // to be called every page load, but before scripts run. > >- if (subj == content.document) { > >+ if (content && subj == content.document) { > > content. > > QueryInterface(Ci.nsIInterfaceRequestor). > > getInterface(Ci.nsIDOMWindowUtils). > > disableDialogs(); > > } > > }, > > When would content be null? This showed up as a random orange test failure, on one of the e10s thumbnail tests. I really don't know the circumstances, but it didn't break anything and the test succeeded. > >- > >- canvas.toBlob(blob => { > >- capture.imageBlob = blob; > >+ canvas.toBlob(function (aBlob) { > >+ capture.imageBlob = aBlob; > > // Load about:blank to finish the capture and wait for onStateChange. > > this._loadAboutBlank(); > >- }); > >+ }.bind(this)); > > Why this change? Looks like some debugging related changes, will remove.

(In reply to Jim Mathies [:jimm] from comment #51) > > >--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 11:34:37 2014 -0500 > > >+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm Sat Sep 27 12:57:27 2014 -0500 > > > > >-const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > > >-const HTML_NS = "http://www.w3.org/1999/xhtml"; > > >- > > > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > > > > > Cu.import("resource://gre/modules/PageThumbs.jsm"); > > >+Cu.import("resource://gre/modules/PageThumbsUtils.jsm"); > > > Cu.import("resource://gre/modules/Services.jsm"); > > > > >- let iframe = hostWindow.document.createElementNS(HTML_NS, "iframe"); > > >+ let iframe = > > >+ hostWindow.document.createElementNS(PageThumbsUtils.HTML_NAMESPACE, > > >+ "iframe"); > > > > >- let browser = this._parentWin.document.createElementNS(XUL_NS, "browser"); > > >+ let browser = > > >+ this._parentWin.document.createElementNS(PageThumbsUtils.XUL_NAMESPACE, > > >+ "browser"); > > > > I'm not convinced that these changes are useful... > > I was just trying to create a useful utils module for this code? It doesn't seem to simplify anything. Five lines removed, seven lines added. Generally I don't think sharing these two namespace URL strings has merit. > > >+ * @param aBrowser The <browser> to capture a thumbnail from. > > > * @param aCallback The function to be called when the thumbnail has been > > > * captured. The first argument will be the data stream > > > * containing the image data. > > > */ > > >- capture: function PageThumbs_capture(aWindow, aCallback) { > > >+ captureToStream: function PageThumbs_captureToStream(aBrowser, aCallback) { > > > > Why did you rename this function when its semantics didn't change? > > This module has pretty good naming on the public interface: captureToBlob, > captureToCanvas, captureAndStore, .. but it also has this odd call named > 'capture'. There are no callers of capture afaict. The method returns a > stream asynchronously. So I renamed it to something more appropriate and > matching the other apis. We could remove this as well, but it seems useful. If it's unused I think we should probably remove it. If we want to keep it for add-ons, then we should probably keep the name too to preserve compatibility.

(In reply to Dão Gottwald [:dao] from comment #52) > (In reply to Jim Mathies [:jimm] from comment #51) > > > >+ let browser = > > > >+ this._parentWin.document.createElementNS(PageThumbsUtils.XUL_NAMESPACE, > > > >+ "browser"); > > > > > > I'm not convinced that these changes are useful... > > > > I was just trying to create a useful utils module for this code? > > It doesn't seem to simplify anything. Five lines removed, seven lines added. > Generally I don't think sharing these two namespace URL strings has merit. How do you feel about the rest of the utils code? > > > >- capture: function PageThumbs_capture(aWindow, aCallback) { > > > >+ captureToStream: function PageThumbs_captureToStream(aBrowser, aCallback) { > > > > > > Why did you rename this function when its semantics didn't change? > > > > This module has pretty good naming on the public interface: captureToBlob, > > captureToCanvas, captureAndStore, .. but it also has this odd call named > > 'capture'. There are no callers of capture afaict. The method returns a > > stream asynchronously. So I renamed it to something more appropriate and > > matching the other apis. We could remove this as well, but it seems useful. > > If it's unused I think we should probably remove it. If we want to keep it > for add-ons, then we should probably keep the name too to preserve > compatibility. I didn't find any addon callers, so I'll remove 'capture'. https://mxr.mozilla.org/addons/search?string=PageThumbs.capture

(In reply to Jim Mathies [:jimm] from comment #53) > (In reply to Dão Gottwald [:dao] from comment #52) > > (In reply to Jim Mathies [:jimm] from comment #51) > > > > >+ let browser = > > > > >+ this._parentWin.document.createElementNS(PageThumbsUtils.XUL_NAMESPACE, > > > > >+ "browser"); > > > > > > > > I'm not convinced that these changes are useful... > > > > > > I was just trying to create a useful utils module for this code? > > > > It doesn't seem to simplify anything. Five lines removed, seven lines added. > > Generally I don't think sharing these two namespace URL strings has merit. > > How do you feel about the rest of the utils code? Better, except for isChannelErrorResponse as mentioned earlier.

- simplified captureToCanvas, no need to split handling up based on remoteness.

This is a follow up that adds an async variant of determineCropSize for remote browsers. We were sending sync ipc calls every time we called determineCropSize, which we don't want to do!

right patch this time.

- cleanup

Comment on attachment 8498859 [details] [diff] [review] pt 1 - move common PageThumbs code to PageThumbUtils (r=dao) >+this.PageThumbUtils = { >+ HTML_NAMESPACE: "http://www.w3.org/1999/xhtml", As with XUL_NAMESPACE, I don't think this is worth sharing.

Comment on attachment 8498873 [details] [diff] [review] pt 2 - PageThumbs e10s compat work >--- a/toolkit/components/thumbnails/PageThumbs.jsm Thu Oct 02 09:02:48 2014 -0500 >+++ b/toolkit/components/thumbnails/PageThumbs.jsm Thu Oct 02 09:03:44 2014 -0500 >-"use strict"; Why? >+ _remoteThumbId: 1, Can this be a variable in this jsm's global scope rather than a property on the exported object? >+ this.captureToCanvas(aBrowser, canvas, function () { >+ let type = this.contentType; >+ // Fetch the canvas data on the next event loop tick so that we allow >+ // some event processing in between drawing to the canvas and encoding >+ // its data. We want to block the UI as short as possible. See bug 744100. >+ canvas.toBlob(function asBlob(blob) { >+ deferred.resolve(blob, type); >+ }); >+ }); I don't think this.contentType will work in that anonymous function. How about: this.captureToCanvas(aBrowser, canvas, () => { // Fetch the canvas data on the next event loop tick so that we allow // some event processing in between drawing to the canvas and encoding // its data. We want to block the UI as short as possible. See bug 744100. canvas.toBlob(blob => { deferred.resolve(blob, this.contentType); }); }); Also, that comment about "the next event loop tick" doesn't quite seem to make sense since you're not using Services.tm.currentThread.dispatch here. >+ let telemetry = Services.telemetry; >+ telemetry.getHistogramById("FX_THUMBNAILS_CAPTURE_TIME_MS") >+ .add(new Date() - telemetryCaptureTime); I don't see the point of the local variable which is used only once. You didn't introduce this, but since you're touching it can you please change it to: Services.telemetry .getHistogramById("FX_THUMBNAILS_CAPTURE_TIME_MS") .add(new Date() - telemetryCaptureTime); >+ Task.spawn((function () { >+ let data = >+ yield this._captureRemoteThumbnail(aBrowser, sw, sh, scale, >+ PageThumbUtils.THUMBNAIL_BG_COLOR); >+ let canvas = data.thumbnail; >+ let ctx = canvas.getContext("2d"); >+ let imgData = ctx.getImageData(0, 0, canvas.width, canvas.height); >+ aCanvas.getContext("2d").putImageData(imgData, 0, 0); >+ if (aCallback) { >+ aCallback(aCanvas); >+ } >+ }).bind(this)); Use () => {...} instead of (function () {...}).bind(this)? > let ctx = aCanvas.getContext("2d"); >- > // Scale the canvas accordingly. > ctx.save(); > ctx.scale(scale, scale); >- > try { > // Draw the window contents to the canvas. Why remove these blank lines? Seems like they're supposed to structure the code. >+ _captureRemoteThumbnail: function (aBrowser, aWidth, aHeight, >+ aScaleFactor, aCssBackground) { Can't you just use PageThumbUtils.THUMBNAIL_BG_COLOR directly instead of the aCssBackground argument? Similarly, it seems like you could call PageThumbUtils.determineCropSize and spare the aWidth, aHeight and aScaleFactor arguments. >--- a/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js Thu Oct 02 09:02:48 2014 -0500 >+++ b/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js Thu Oct 02 09:03:44 2014 -0500 > capture.canvasDrawTime = new Date() - canvasDrawDate; >- > canvas.toBlob(blob => { Why?

Updated per most comments, some remaining comments below - (In reply to Dão Gottwald [:dao] from comment #63) > Comment on attachment 8498873 [details] [diff] [review] > > Also, that comment about "the next event loop tick" doesn't quite seem to > make sense since you're not using Services.tm.currentThread.dispatch here. Not sure what that was about, I didn't write it. I just took it out. > >+ _captureRemoteThumbnail: function (aBrowser, aWidth, aHeight, > >+ aScaleFactor, aCssBackground) { > > Can't you just use PageThumbUtils.THUMBNAIL_BG_COLOR directly instead of the > aCssBackground argument? > Similarly, it seems like you could call PageThumbUtils.determineCropSize and > spare the aWidth, aHeight and aScaleFactor arguments. This code is going to undergo some changes in part 3, where a async determineCropSize is needed in the top async block. So I'd like to leave this alone for now. We can discuss in part 3.

(In reply to Dão Gottwald [:dao] from comment #63) > >+ Task.spawn((function () { > >+ let data = > >+ yield this._captureRemoteThumbnail(aBrowser, sw, sh, scale, > >+ PageThumbUtils.THUMBNAIL_BG_COLOR); > >+ let canvas = data.thumbnail; > >+ let ctx = canvas.getContext("2d"); > >+ let imgData = ctx.getImageData(0, 0, canvas.width, canvas.height); > >+ aCanvas.getContext("2d").putImageData(imgData, 0, 0); > >+ if (aCallback) { > >+ aCallback(aCanvas); > >+ } > >+ }).bind(this)); > > Use () => {...} instead of (function () {...}).bind(this)? Apparently you can't - you get "SyntaxError: arrow function may not contain yield" http://wiki.ecmascript.org/doku.php?id=harmony:arrow_function_syntax

review ping?

- cleaned up use of determineCropSize return values.

sorry, right patch this time.

- folded part 4 in which had a few front end changes and enabled some thumbnail tests with e10s.

- cleanup, including filling in some bug numbers

review ping - sorry to bug, we're trying to get m3s landed this week.

Comment on attachment 8503268 [details] [diff] [review] pt 3 - determine crop sizes async Can the crop size be determined in the Browser:Thumbnail:Request handler rather than having the parent process get it from the content process only to pass it back to there for the actual thumbnail request?

(In reply to Dão Gottwald [:dao] from comment #79) > Comment on attachment 8503268 [details] [diff] [review] > pt 3 - determine crop sizes async > > Can the crop size be determined in the Browser:Thumbnail:Request handler > rather than having the parent process get it from the content process only > to pass it back to there for the actual thumbnail request? Yes looks like it can, I can pass the canvas dims instead.

I'm not making use of determineCropSizeAsync anymore, but I'm pretty sure we want to keep that as a companion to determineCropSize?

(In reply to Jim Mathies [:jimm] from comment #82) > I'm not making use of determineCropSizeAsync anymore, but I'm pretty sure we > want to keep that as a companion to determineCropSize? No, it can go away.

ok, removed.

Comment on attachment 8505527 [details] [diff] [review] pt 3 - fixup determine crop size use > * @param aWindow The content window. >- * @param aCanvas The target canvas. >+ * @param aThumbWidth, aThumbHeight The target canvas dims. > * @return An array containing width, height and scale. > */ >- determineCropSize: function (aWindow, aCanvas) { >+ determineCropSize: function (aWindow, aThumbWidth, aThumbHeight) { >+ if (Cu.isCrossProcessWrapper(aWindow)) { >+ throw new Error('Do not pass cpows here.'); >+ } > let utils = aWindow.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > // aWindow may be a cpow, add exposed props security values. >- let sbWidth = {__exposedProps__: {"value": "rw"}}, >- sbHeight = {__exposedProps__: {"value": "rw"}}; >+ let sbWidth = {}, sbHeight = {}; > > try { > utils.getScrollbarSize(false, sbWidth, sbHeight); > } catch (e) { > // This might fail if the window does not have a presShell. > Cu.reportError("Unable to get scrollbar size in determineCropSize."); > sbWidth.value = sbHeight.value = 0; > } > > // Even in RTL mode, scrollbars are always on the right. > // So there's no need to determine a left offset. >- let sw = aWindow.innerWidth - sbWidth.value; >- let sh = aWindow.innerHeight - sbHeight.value; >+ let width = aWindow.innerWidth - sbWidth.value; >+ let height = aWindow.innerHeight - sbHeight.value; > >- let {width: thumbnailWidth, height: thumbnailHeight} = aCanvas; >- let scale = Math.min(Math.max(thumbnailWidth / sw, thumbnailHeight / sh), 1); >- let scaledWidth = sw * scale; >- let scaledHeight = sh * scale; >+ let scale = Math.min(Math.max(aThumbWidth / width, aThumbHeight / height), 1); >+ let scaledWidth = width * scale; >+ let scaledHeight = height * scale; > >- if (scaledHeight > thumbnailHeight) >- sh -= Math.floor(Math.abs(scaledHeight - thumbnailHeight) * scale); >+ if (scaledHeight > aThumbHeight) >+ height -= Math.floor(Math.abs(scaledHeight - aThumbWidth) * scale); > >- if (scaledWidth > thumbnailWidth) >- sw -= Math.floor(Math.abs(scaledWidth - thumbnailWidth) * scale); >+ if (scaledWidth > aThumbWidth) >+ width -= Math.floor(Math.abs(scaledWidth - aThumbHeight) * scale); > >- return [sw, sh, scale]; >+ return [width, height, scale]; > } > }; Looks like you can avoid these changes and let the Browser:Thumbnail:Request pass in the canvas element (after setting width and height on it, see below). >+ thumbnail.width = Math.round(width * scale); >+ thumbnail.height = Math.round(height * scale); Shouldn't you just use aMessage.data.canvasWidth and aMessage.data.canvasHeight here?

> >+ thumbnail.width = Math.round(width * scale); > >+ thumbnail.height = Math.round(height * scale); > > Shouldn't you just use aMessage.data.canvasWidth and > aMessage.data.canvasHeight here? The dims are similar, but not the same. We would skip the adjustments that determineCropSize does for thumbnail aspect ratio and scrollbars.

I don't understand why that matters, given that determineCropSize currently works with a canvas where width and height are preset. Is that currently not behaving as expected?

(In reply to Dão Gottwald [:dao] from comment #87) > I don't understand why that matters, given that determineCropSize currently > works with a canvas where width and height are preset. Is that currently not > behaving as expected? Well the dims of the canvas will be slightly larger than the area we paint to. Then we ship that canvas over as a blob and render the entire image to the canvas the caller passed in. I'm guessing there will be margins on the thumbnails, but I'll make the change and test, see how things look.

Comment on attachment 8505553 [details] [diff] [review] pt 3 - fixup determine crop size use I didn't see any borders with ctrl-tab so, looks ok.

(In reply to Jim Mathies [:jimm] from comment #90) > Comment on attachment 8505553 [details] [diff] [review] > pt 3 - fixup determine crop size use > > I didn't see any borders with ctrl-tab so, looks ok. In looking at this a bit more, this makes sense. In browser-child we render cropped width and heights to the entire thumbnail. So there shouldn't be an issue with margins.

Comment on attachment 8505553 [details] [diff] [review] pt 3 - fixup determine crop size use Remove the unused gRemoteMsgId and fix indentation which is off in Determinecropsize where you declare width and height.

Comment on attachment 8503343 [details] [diff] [review] pt 4 - front end callers update and tests Please get rid of refreshThumbnailAsync which is basically just an alias for 'capture'. Looks like the optional callback isn't used, so no need to add support for that either. Also don't shuffle around the code in tabbrowser.xml, just update the captureToCanvas call.

(In reply to Dão Gottwald [:dao] from comment #93) > Comment on attachment 8503343 [details] [diff] [review] > pt 4 - front end callers update and tests > > Please get rid of refreshThumbnailAsync which is basically just an alias for > 'capture'. Looks like the optional callback isn't used, so no need to add > support for that either. I was trying to provide an async version of this api for future use. Will remove though I guess. > Also don't shuffle around the code in tabbrowser.xml, just update the > captureToCanvas call. There's nothing to update here for the captureToCanvas call. I can just take my improvements out. Although we'll take the overhead of prepping that canvas knowing it doesn't actually work.

(In reply to Jim Mathies [:jimm] from comment #94) > (In reply to Dão Gottwald [:dao] from comment #93) > > Comment on attachment 8503343 [details] [diff] [review] > > pt 4 - front end callers update and tests > > > > Please get rid of refreshThumbnailAsync which is basically just an alias for > > 'capture'. Looks like the optional callback isn't used, so no need to add > > support for that either. > > I was trying to provide an async version of this api for future use. Will > remove though I guess. > > > Also don't shuffle around the code in tabbrowser.xml, just update the > > captureToCanvas call. > > There's nothing to update here for the captureToCanvas call. I can just take > my improvements out. Although we'll take the overhead of prepping that > canvas knowing it doesn't actually work. I'm going to leave the tab drag code in. Without it we get an empty white tab drag image or an empty frame on mac, which looks broken. With my change nothing shows up, which is what I wanted.

- removed socialchat change - removed improvements to browser-tabPreviews.js

Comment on attachment 8506127 [details] [diff] [review] pt 4 - front end callers update and tests (In reply to Jim Mathies [:jimm] from comment #94) > > Please get rid of refreshThumbnailAsync which is basically just an alias for > > 'capture'. Looks like the optional callback isn't used, so no need to add > > support for that either. > > I was trying to provide an async version of this api for future use. Will > remove though I guess. You're already making 'capture' itself implicitly async, which is fine. We can also add an optional callback to that method if and when needed. You still need to update the captureToCanvas call, which appears to be missing in this patch. > > Also don't shuffle around the code in tabbrowser.xml, just update the > > captureToCanvas call. > > There's nothing to update here for the captureToCanvas call. Yes, there is, since you changed the signature of captureToCanvas. > I'm going to leave the tab drag code in. Without it we get an empty white > tab drag image or an empty frame on mac, which looks broken. With my change > nothing shows up, which is what I wanted. We'll need to treat that as a bug either way. Having no thumbnail for detaching tabs isn't really shippable. Reindenting all the code just adds an extra layer to its hg annotations for no real benefit that I can see.

> > There's nothing to update here for the captureToCanvas call. > > Yes, there is, since you changed the signature of captureToCanvas. Ah yes, sorry, window -> browser. My mistake. Will update. > > I'm going to leave the tab drag code in. Without it we get an empty white > > tab drag image or an empty frame on mac, which looks broken. With my change > > nothing shows up, which is what I wanted. > > We'll need to treat that as a bug either way. Having no thumbnail for > detaching tabs isn't really shippable. Reindenting all the code just adds an > extra layer to its hg annotations for no real benefit that I can see. I think a minor code / formatting change to avoid displaying something that is currently broken seems pretty worthy right? We have a follow up polish bug on file for drag thumbnails, there's no assurance we'll actually get that working, hence disabling existing code for now.

(In reply to Jim Mathies [:jimm] from comment #98) > I think a minor code / formatting change to avoid displaying something that > is currently broken seems pretty worthy right? Like I said, skipping all of that code just implements another broken state that I do not think is shippable.

(In reply to Dão Gottwald [:dao] from comment #99) > (In reply to Jim Mathies [:jimm] from comment #98) > > I think a minor code / formatting change to avoid displaying something that > > is currently broken seems pretty worthy right? > > Like I said, skipping all of that code just implements another broken state > that I do not think is shippable. Ok, I'll keep the currently broken behavior active.

Hmm, some where in all these changes about page aspect ratios broke. Need to go back and figure out where that fell down.

- fixed outdated use of determineCropSize in local tab rendering code

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0a91e01c47c3 remote: https://tbpl.mozilla.org/?tree=Try&rev=0a91e01c47c3

Comment on attachment 8506196 [details] [diff] [review] pt 4 - front end callers update and tests >--- a/browser/base/content/browser-thumbnails.js Wed Oct 15 11:28:36 2014 -0500 >+++ b/browser/base/content/browser-thumbnails.js Thu Oct 16 10:42:24 2014 -0500 >+ // Don't take screenshots of about: pages. >+ if (aBrowser.currentURI.schemeIs("about")) >+ return false; >+ >+ // FIXME e10s work around, we need channel information. bug 1073957 >+ if (!aBrowser.docShell) { >+ return true; >+ } nit 1: local prevailing style is to omit these braces nit 2: indentation of your FIXME comment is off >--- a/browser/components/tabview/tabitems.js Wed Oct 15 11:28:36 2014 -0500 >+++ b/browser/components/tabview/tabitems.js Thu Oct 16 10:42:24 2014 -0500 >+ gPageThumbnails.captureToCanvas(this.tab.linkedBrowser, this.canvas, function () { >+ this._sendToSubscribers("painted"); >+ }.bind(this)); Looks like () => {...} instead of function () {...}.bind(this) should work here.

remote: https://hg.mozilla.org/integration/fx-team/rev/17bbdeffa8a remote: https://hg.mozilla.org/integration/fx-team/rev/d004bfd7f70 remote: https://hg.mozilla.org/integration/fx-team/rev/81378dee5a6 remote: https://hg.mozilla.org/integration/fx-team/rev/6c6d420a3ac Thanks!

e10s bc3 was failing with these, so backed out in https://hg.mozilla.org/integration/fx-team/rev/5e9facc28af9 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=938938&repo=fx-team

(In reply to Wes Kocher (:KWierso) from comment #107) > e10s bc3 was failing with these, so backed out in > https://hg.mozilla.org/integration/fx-team/rev/5e9facc28af9 > > https://treeherder.mozilla.org/ui/logviewer.html#?job_id=938938&repo=fx-team This test was failing on linux but succeeding on mac and win locally. The cause is covered by bug 1084637. I'm going to disable on all platforms for e10s.

remote: https://hg.mozilla.org/integration/fx-team/rev/00b3768628ae remote: https://hg.mozilla.org/integration/fx-team/rev/57f8522b4b09 remote: https://hg.mozilla.org/integration/fx-team/rev/d67b8fed95c6 remote: https://hg.mozilla.org/integration/fx-team/rev/d700cacf3994 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=58e9d738fc2e

https://hg.mozilla.org/mozilla-central/rev/00b3768628ae https://hg.mozilla.org/mozilla-central/rev/57f8522b4b09 https://hg.mozilla.org/mozilla-central/rev/d67b8fed95c6 https://hg.mozilla.org/mozilla-central/rev/d700cacf3994