getScreenshot property for <iframe mozbrowser>

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla15
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 624381 [details]
Work in Progress
(Assignee)

Updated

5 years ago
Blocks: 693515
(Assignee)

Comment 2

5 years ago
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
> 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.
(Assignee)

Comment 4

5 years ago
> 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
> 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.   :)
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.
(Assignee)

Comment 7

5 years ago
Created attachment 624436 [details]
Work in Progress
Attachment #624381 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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)
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!
(Assignee)

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
Created attachment 624609 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624436 - Attachment is obsolete: true
Attachment #624609 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 12

5 years ago
Created attachment 624611 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624609 - Attachment is obsolete: true
Attachment #624609 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 13

5 years ago
Created attachment 624625 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624611 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 624635 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624625 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Sorry for that multitude of attachments, need to pay more attention, they were all minor changes (renames, forgetting to rebase etc)
Attachment #624635 - Attachment is patch: true
Attachment #624635 - Flags: review?(justin.lebar+bug)
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.
Attachment #624635 - Flags: review?(justin.lebar+bug) → review-
(Assignee)

Comment 17

5 years ago
> 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
> 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.
(Assignee)

Comment 19

5 years ago
Created attachment 624919 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624635 - Attachment is obsolete: true
Do you need a review on this, or are you still working on it?
(Assignee)

Comment 21

5 years ago
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 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?.)
Attachment #624919 - Flags: review?(justin.lebar+bug)
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.
Attachment #624919 - Flags: review?(justin.lebar+bug) → review+
https://tbpl.mozilla.org/?tree=Try&rev=4a3137b98a17
(Assignee)

Comment 25

5 years ago
Created attachment 624973 [details] [diff] [review]
Add a getScreenshot call to mozbrowser

I also fixed the function ordering in BrowserElementChild
Attachment #624919 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Created attachment 625319 [details] [diff] [review]
Add a getScreenshot call to mozbrowser
Attachment #624973 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03d90c2504e
Assignee: nobody → dale
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b03d90c2504e
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 756844
Keywords: dev-doc-needed
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getScreenshot
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.