753595 - getScreenshot property for <iframe mozbrowser>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: Work in Progress

Comment 2

[Dale Harvey (:daleharvey)]

Attached the working but completely broken commit as it looks like this will need to be changed in line with #754997

While I am working on that, some recommendation for how to cleanly block screenshots until after the first mozpaint event when an iframes .src is changed would be useful, what is there works but is obviously not clean

Comment 3

[Justin Lebar (not reading bugmail)]

> as it looks like this will need to be changed in line with #754997

I don't think so.  You're not overriding a property on the window object here; adding a method to the frame element should be fine.

> While I am working on that, some recommendation for how to cleanly block screenshots until after the 
> first mozpaint event when an iframes .src is changed would be useful,

I think what you have is the right idea.

But I don't think

  //removeEventListener('MozAfterPaint', this._afterPaintHandler.bind(this));

will work because fn.bind() returns a new function each time, so this._afterPaintHandler.bind(this) is not the same as the function you originally registered as a listener.

Instead of maintaining a list of waitingForScreenshots, you could have each waiter register its own MozAfterPaint listener; that might be a bit cleaner.  The trick is removing those listeners cleanly without arguments.callee (because we're in strict mode).  I don't have a good answer short of disabling strict mode.

I don't think you need UUID's -- each child has only one parent, so if the parent incremented a counter on each request, that would work just as well.

@@ -122,17 +165,19 @@ BrowserElementChild.prototype = 
       if (stateFlags & Ci.nsIWebProgressListener.STATE_START) {
         this._seenLoadStart = true;
+        debug('loadstart');
         sendAsyncMsg('loadstart');
+        _painted = false;

I think you need some scope on _painted.

Comment 4

[Dale Harvey (:daleharvey)]

> Instead of maintaining a list of waitingForScreenshots, you could have each waiter register its own MozAfterPaint listener; > that might be a bit cleaner.  The trick is removing those listeners cleanly without arguments.callee (because we're in strict > mode).  I don't have a good answer short of disabling strict mode.

I like that idea, we can keep a track of this._mozPaintListeners[id] to unbind, or disable strict mode, which is preferable?

> I think you need some scope on _painted.

The problem is _painted needs to have scope inside both _progressListener and BrowserElementChild, vivian was dead against the global _painted but unless I bind nsIWebProgress to BrowserElementChild I didnt see a cleaner way

Comment 5

[Justin Lebar (not reading bugmail)]

> The problem is _painted needs to have scope inside both _progressListener and BrowserElementChild

The progress listener could have a pointer to its BrowserElementChild, and _painted could be stored on BrowserElementChild?

> I like that idea, we can keep a track of this._mozPaintListeners[id] to unbind, or disable strict 
> mode, which is preferable?

Neither of those is particularly pretty.  I don't have a strong opinion, although I reserve the right to have one after looking at the code.   :)

Comment 6

[Justin Lebar (not reading bugmail)]

Also -- and we can kick this to a followup if you'd like -- locationchange is not the right event to reset |painted| on.

We can change locations without changing documents -- for example, we can navigate from foo.html to foo.html#bar, or the page can call history.pushState.

We can also change documents without changing locations (e.g. refreshing the page) although we might still get a locationchange event in that case; I'm not sure.

The event we actually want is basically "my docshell changed its inner window", which is to say "we're now showing a different document."  I think window.onpageshow will do this correctly; you could register a listener in the child and bubble it up to the parent.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Work in Progress

Comment 8

[Dale Harvey (:daleharvey)]

I dont reset painted on onlocationChange, its onStateChange, but yes I didnt know exactly that means and wanted to test what triggered it (vivien mentioned ajax might do so), cheers for the pointer to onpageshow, I will look at switching it to that, but figured its worth getting feedback on this patch

This just adds new event listeners for each blocked request, keep track of them and removes them when called, it seems clean enough to me? also got rid of the uuid (I still want to track the id through the content process since I want to make sure the screenshot only returns to its corresponding request)

Comment 9

[Justin Lebar (not reading bugmail)]

I think this is pretty sane.

I think onStateChange is what we use to set the throbber, so we may not go through a STATE_START / STATE_STOP when doing a load which doesn't hit the network (bfcache).  But maybe it works!

Comment 10

[Dale Harvey (:daleharvey)]

I was about to say loading from bfcache doesnt matter, but it will when we support the goBack / goForward part of the api on the mozbrowser it will

The _painted flag is so we can block the screenshot in the situation that we do

> browser.src = 'http://google.com'
> browser.getScreenshot().onsuccess(......

We want to ensure the page has completed rendering before taking the screenshot, similiarly if we do 

> browser.goBack()
> browser.getScreenshot().onsuccess(......

we want to block until we have rendered the appropriate page, I tested onStateChange when using history.back() within the iframe and it works great, however without browser.goBack() its not particularly nice to test, I think I should wait until goBack etc is implemented before writing tests for those use cases, so I think this commit is about ready for review now

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 14

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 15

[Dale Harvey (:daleharvey)]

Sorry for that multitude of attachments, need to pay more attention, they were all minor changes (renames, forgetting to rebase etc)

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 624635 [details] [diff] [review]
Add a getScreenshot call to mozbrowser

>diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js
>index b4ce4d7..537d0d6 100644
>--- a/dom/base/BrowserElementChild.js
>+++ b/dom/base/BrowserElementChild.js
>@@ -41,16 +41,20 @@ BrowserElementChild.prototype = {
>     debug("Starting up.");
>     sendAsyncMsg("hello");
> 
>     docShell.QueryInterface(Ci.nsIWebProgress)
>             .addProgressListener(this._progressListener,
>                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
>                                  Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
> 
>+    this._painted = false;
>+    this._mozPaintListeners = {};
>+    this._progressListener.browserElementChild = this;

Does it work to set the _progressListener's browserElementChild as 

  _progressListener: {
    browserElementChild: this
  }

?  If not, what you have is fine.  But please make it _browserElementChild for
consistency here.

(This is a convention I picked up from some other files.  I'm not sure it's
solving a real problem, so maybe we could get rid of it.  But if we were to do
that, we should do it all at once.)

>@@ -70,16 +74,56 @@ BrowserElementChild.prototype = {
>                      this._titleChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
> 
>     addEventListener('DOMLinkAdded',
>                      this._iconChangedHandler.bind(this),
>                      /* useCapture = */ true,
>                      /* wantsUntrusted = */ false);
>+
>+    addMessageListener("browser-element-api:getScreenshot",
>+                       this._recvScreenshot.bind(this));

For messages which correspond to events (e.g. "locationchange") I've been using the event name, and I guess the idea is that "getScreenshot" is the name of the method on the frame elem which triggered this?  But since we have a corresponding "got the screenshot" message which doesn't correspond to a method, we should name the two messages with similar conventions.  Let's use "get-screenshot"?

>+  },
>+
>+  _getScreenshot: function(id) {
>+    var canvas = content.document
>+      .createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>+    var ctx = canvas.getContext("2d");
>+    canvas.mozOpaque = true;
>+    canvas.height = content.innerHeight;
>+    canvas.width = content.innerWidth;
>+    ctx.drawWindow(content, 0, 0, content.innerWidth,
>+                   content.innerHeight, "rgb(255,255,255)");
>+    sendAsyncMsg('screenshotloaded', {

Should probably be "screenshot-loaded", since we're not firing an event.  Or maybe "got-screenshot"?

>+      id: id,
>+      screenshot: canvas.toDataURL("image/png")
>+    });
>+  },
>+
>+  _deferGetScreenshot: function(id) {
>+    return function() {
>+      this._painted = true;
>+      this._getScreenshot(id);
>+      removeEventListener('MozAfterPaint', this._mozPaintListeners[id], true, false);
>+      delete this._mozPaintListeners[id];
>+    };

Bind to |this| here instead of when you call this function?  Also, this function needs a better name.  Or you could just shove it into recvScreenshot.

>+  },
>+
>+  _recvScreenshot: function(data) {
>+    var id = data.json.id;
>+    if (this._painted) {
>+      return this._getScreenshot(id);

_getScreenshot() doesn't return anything.

>+    }
>+
>+    this._mozPaintListeners[id] = this._deferGetScreenshot(id).bind(this);
>+    addEventListener('MozAfterPaint',
>+                     this._mozPaintListeners[id],
>+                     /* useCapture = */ true,
>+                     /* wantsUntrusted = */ false);

>@@ -135,16 +179,17 @@ BrowserElementChild.prototype = {
> 
>     onStateChange: function(webProgress, request, stateFlags, status) {
>       if (webProgress != docShell) {
>         return;
>       }
> 
>       if (stateFlags & Ci.nsIWebProgressListener.STATE_START) {
>         this._seenLoadStart = true;
>+        this.browserElementChild._painted = false;
>         sendAsyncMsg('loadstart');
>       }

It wouldn't be a lot more work to listen to the pageshow event, and then we'd have some confidence that this was correct.  I'd rather you did that in this patch, unless there's a good reason to stay with this as is.

>@@ -47,16 +49,19 @@ BrowserElementParent.prototype = {
>     if (!this._browserFramesPrefEnabled()) {
>       var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>       prefs.addObserver(BROWSER_FRAMES_ENABLED_PREF, this, /* ownsWeak = */ true);
>       return;
>     }
> 
>     this._initialized = true;
> 
>+    this._screenshotListeners = {};
>+    this.screenshotReqCounter = 0;

_screenshotReqCounter.

>@@ -79,36 +84,59 @@ BrowserElementParent.prototype = {
> 
>   _setUpMessageManagerListeners: function(frameLoader) {
>     let frameElement = frameLoader.QueryInterface(Ci.nsIFrameLoader).ownerElement;
>     if (!frameElement) {
>       debug("No frame element?");
>       return;
>     }
> 
>+    let unwrappedWin = XPCNativeWrapper.unwrap(frameElement);

unwrappedFrameElement?  And please declare it closer to its first use.

(I'd also be fine with Object.defineProperty(XPCNativeWrapper.unwrap(frameElement), ...))

>     let mm = frameLoader.messageManager;
> 
>     // Messages we receive are handled by functions with parameters
>     // (frameElement, data), where |data| is the message manager's data object.
> 
>     let self = this;
>     function addMessageListener(msg, handler) {
>       mm.addMessageListener('browser-element-api:' + msg, handler.bind(self, frameElement));
>     }
> 
>+    function getScreenshot(callback) {
>+      let id = 'req_' + this.screenshotReqCounter++;
>+      let req = Services.DOMRequest
>+        .createRequest(frameElement.ownerDocument.defaultView);
>+      this._screenshotListeners[id] = req;
>+      mm.sendAsyncMessage('browser-element-api:getScreenshot', {id: id});
>+      return req;
>+    }
>+
>     addMessageListener("hello", this._recvHello);
>     addMessageListener("locationchange", this._fireEventFromMsg);
>     addMessageListener("loadstart", this._fireEventFromMsg);
>     addMessageListener("loadend", this._fireEventFromMsg);
>     addMessageListener("titlechange", this._fireEventFromMsg);
>     addMessageListener("iconchange", this._fireEventFromMsg);
>     addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
> 
>     mm.loadFrameScript("chrome://global/content/BrowserElementChild.js",
>                        /* allowDelayedLoad = */ true);
>+
>+    mm.addMessageListener('browser-element-api:screenshotloaded',
>+                          this._recvScreenshot.bind(this));
>+
>+    Object.defineProperty(unwrappedWin, 'getScreenshot', {
>+      value: getScreenshot.bind(this)
>+    });

You should load the frame script after you've set up all the message listeners.

Also, I think you can get away without Object.defineProperty here and do |unwrappedFrameElement.getScreenshot = ...|.  My understanding is that Object.defineProperty was required for windows because Window objects are weird (defineProperty was defining something on the outer window, the thing which doesn't change when we navigate, but win.foo = bar defined a property on the *inner* window, so would get cleared on navigation.)

I'd also move the Object.defineProperty closer to the getScreenshot function (or vice versa).

I'd like to have another look, if you don't mind.

Comment 17

[Dale Harvey (:daleharvey)]

> It wouldn't be a lot more work to listen to the pageshow event, and then we'd have 
> some confidence that this was correct.  I'd rather you did that in this patch, 
> unless there's a good reason to stay with this as is.

Well it would be onunload we would switch to, this part is to invalidate the flag when the src has been changed, it tells us to wait for the mozpaint event before capturing the screenshot, thats why its set on STATE_START and not STATE_STOP (so pageshow is a possible replacement for mozpaint)

fixing up the rest, cheers

Comment 18

[Justin Lebar (not reading bugmail)]

> Well it would be onunload we would switch to, this part is to invalidate the flag when the src has 
> been changed, it tells us to wait for the mozpaint event before capturing the screenshot

pagehide, then.  :)  onunload has basically the same problem as onload in that it never fires twice for a given document -- if a page has an onunload handler, we actually can't put it in the bfcache.

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 20

[Justin Lebar (not reading bugmail)]

Do you need a review on this, or are you still working on it?

Comment 21

[Dale Harvey (:daleharvey)]

Yeh everything from the last review is resolved.

getScreenshot now takes a screenshot as the page is at that moment, so the developer can choose to wait for mozbrowserloadend etc

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 624919 [details] [diff] [review]
Add a getScreenshot call to mozbrowser

Cool; I'll have a look.

(FYI, people usually ignore patches without r?.)

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 624919 [details] [diff] [review]
Add a getScreenshot call to mozbrowser

>     addMessageListener("iconchange", this._fireEventFromMsg);
>     addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
>+    mm.addMessageListener('browser-element-api:got-screenshot',
>+                          this._recvGotScreenshot.bind(this));
>+
>+    function getScreenshot(callback) {
>+      let id = 'req_' + this._screenshotReqCounter++;
>+      let req = Services.DOMRequest
>+        .createRequest(frameElement.ownerDocument.defaultView);
>+      this._screenshotListeners[id] = req;
>+      mm.sendAsyncMessage('browser-element-api:get-screenshot', {id: id});
>+      return req;
>+    }

I think it would be cleaner if you moved getScreenshot() onto BrowserElementParent and passed frameElement as an argument (this._getScreenshot.bind(this, frameElement)).

>+    XPCNativeWrapper.unwrap(frameElement).getScreenshot = getScreenshot.bind(this);
> 
>     mm.loadFrameScript("chrome://global/content/BrowserElementChild.js",
>                        /* allowDelayedLoad = */ true);
>   },
> 
>+  _recvGotScreenshot: function(data) {
>+    var req = this._screenshotListeners[data.json.id];
>+    delete this._screenshotListeners[data.json.id];
>+    Services.DOMRequest.fireSuccess(req, data.json.screenshot);
>+  },

Nit: Please put this (and _getScreenshot) after the event-handling code, so the order things are used in init() matches the order they're declared.

Comment 24

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=4a3137b98a17

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

I also fixed the function ordering in BrowserElementChild

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: Add a getScreenshot call to mozbrowser

Comment 27

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b03d90c2504e

Comment 28

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b03d90c2504e

Comment 29

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getScreenshot