698371 - Update PageThumbs to support remote content

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Raymond Lee [:raymondlee]]

Canvas.drawWindow requires contentWindow as the first argument.

Comment 1

[Tom S [:evilpie]]

Seems to work already!

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Brian Smith (:briansmith), was [e-mail] (: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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 5

[Dão Gottwald [:dao]]

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 9

[Tom S [:evilpie]]

I think this might be useful for bug 863512.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 12

[(no longer active)]

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

Comment 13

[Jim Mathies [:jimm]]

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.

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[Jim Mathies [:jimm]]

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.

Comment 16

[Jim Mathies [:jimm]]

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?

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 18

[Jim Mathies [:jimm]]

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

Comment 19

[Dão Gottwald [:dao]]

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

Comment 20

[Jim Mathies [:jimm]]

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

Comment 21

[Dão Gottwald [:dao]]

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

Comment 22

[Jim Mathies [:jimm]]

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

Comment 23

[Chris Peterson [:cpeterson]]

This bug should be fixed for the e10s M2 milestone.

Comment 24

[Chris Peterson [:cpeterson]]

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

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 27

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 28

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 29

[Jim Mathies [:jimm]]

Attachment: wip

Comment 30

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 31

[Jim Mathies [:jimm]]

Attachment: wip

- cleanup

Comment 32

[Jim Mathies [:jimm]]

Attachment: wip

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8121abb0743c

Comment 33

[Jim Mathies [:jimm]]

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.

Comment 34

[Jim Mathies [:jimm]]

Attachment: wip

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

Comment 35

[Jim Mathies [:jimm]]

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 36

[Jim Mathies [:jimm]]

Attachment: pt 1 - move common PageThumbs code to PageThumbsUtils

Comment 37

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

Comment 38

[Jim Mathies [:jimm]]

Attachment: pt 3 - browser changes

Comment 39

[Jim Mathies [:jimm]]

Attachment: pt 4 - test updates

Comment 40

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

Comment 41

[Jim Mathies [:jimm]]

Attachment: pt 3 - browser changes

Comment 42

[Jim Mathies [:jimm]]

Attachment: pt 4 - e10s tests updates

Comment 43

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=2b0d5850fcf8

Comment 44

[Jim Mathies [:jimm]]

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 45

[Jim Mathies [:jimm]]

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

Comment 46

[Dão Gottwald [:dao]]

Can you rename PageThumbsUtils to PageThumbUtils?

Comment 47

[Jim Mathies [:jimm]]

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

Sure.

Comment 48

[Jim Mathies [:jimm]]

Dão - review ping? :)

Comment 49

[Dão Gottwald [:dao]]

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 50

[Dão Gottwald [:dao]]

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?

Comment 51

[Jim Mathies [:jimm]]

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

Comment 52

[Dão Gottwald [:dao]]

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

Comment 53

[Jim Mathies [:jimm]]

(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

Comment 54

[Dão Gottwald [:dao]]

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

Comment 55

[Jim Mathies [:jimm]]

Attachment: pt 1 - move common PageThumbs code to PageThumbUtils (r=dao)

Comment 56

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

Comment 57

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

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

Comment 59

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

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!

Comment 60

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

right patch this time.

Comment 61

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

- cleanup

Comment 62

[Dão Gottwald [:dao]]

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 63

[Dão Gottwald [:dao]]

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?

Comment 64

[Jim Mathies [:jimm]]

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.

Comment 65

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

Comment 66

[Jim Mathies [:jimm]]

Attachment: pt 2 - PageThumbs e10s compat work

Comment 67

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

Comment 68

[Jim Mathies [:jimm]]

(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

Comment 69

[Jim Mathies [:jimm]]

Attachment: p2 + p3 rollup

Comment 70

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

Comment 71

[Jim Mathies [:jimm]]

Attachment: p2 + p3 rollup

Comment 72

[Jim Mathies [:jimm]]

review ping?

Comment 73

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

- cleaned up use of determineCropSize return values.

Comment 74

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

sorry, right patch this time.

Comment 75

[Jim Mathies [:jimm]]

Attachment: pt 4 - front end callers update

Comment 76

[Jim Mathies [:jimm]]

Attachment: pt 4 - front end callers update and tests

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

Comment 77

[Jim Mathies [:jimm]]

Attachment: pt 4 - front end callers update and tests

- cleanup, including filling in some bug numbers

Comment 78

[Jim Mathies [:jimm]]

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

Comment 79

[Dão Gottwald [:dao]]

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?

Comment 80

[Jim Mathies [:jimm]]

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

Comment 81

[Jim Mathies [:jimm]]

Attachment: pt 3 - determine crop sizes async

Comment 82

[Jim Mathies [:jimm]]

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

Comment 83

[Dão Gottwald [:dao]]

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

Comment 84

[Jim Mathies [:jimm]]

Attachment: pt 3 - fixup determine crop size use

ok, removed.

Comment 85

[Dão Gottwald [:dao]]

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?

Comment 86

[Jim Mathies [:jimm]]

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

Comment 87

[Dão Gottwald [:dao]]

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?

Comment 88

[Jim Mathies [:jimm]]

(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 89

[Jim Mathies [:jimm]]

Attachment: pt 3 - fixup determine crop size use

Comment 90

[Jim Mathies [:jimm]]

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.

Comment 91

[Jim Mathies [:jimm]]

(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 92

[Dão Gottwald [:dao]]

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 93

[Dão Gottwald [:dao]]

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.

Comment 94

[Jim Mathies [:jimm]]

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

Comment 95

[Jim Mathies [:jimm]]

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

Comment 96

[Jim Mathies [:jimm]]

Attachment: pt 4 - front end callers update and tests

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

Comment 97

[Dão Gottwald [:dao]]

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.

Comment 98

[Jim Mathies [:jimm]]

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

Comment 99

[Dão Gottwald [:dao]]

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

Comment 100

[Jim Mathies [:jimm]]

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

Comment 101

[Jim Mathies [:jimm]]

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

Comment 102

[Jim Mathies [:jimm]]

Attachment: pt 4 - front end callers update and tests

- fixed outdated use of determineCropSize in local tab rendering code

Comment 103

[Jim Mathies [:jimm]]

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

Comment 104

[Dão Gottwald [:dao]]

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.

Comment 105

[Jim Mathies [:jimm]]

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!

Comment 107

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 108

[Jim Mathies [:jimm]]

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

Comment 109

[Jim Mathies [:jimm]]

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

Comment 110

[Carsten Book [:Tomcat]]

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