Closed Bug 1262594 Opened 8 years ago Closed 8 years ago

Rebuild screenshot capturing after iframe mozbrowser

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox48 fixed, firefox49 verified)

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: jryans, Assigned: zer0)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(1 file)

Once we land the switch to <iframe mozbrowser>, the new screenshot button will now only capture blank images.  We need to change how we're capturing the picture.

We're already using the frame script from old RDM with <iframe mozbrowser>, so we can call the screenshot API it offers.
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [multiviewport][triage] → [multiviewport] [mvp-rdm]
QA Contact: mihai.boldan
Assignee: nobody → zer0
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
var s = iframe.getScreenshot(280, 280);
s.onsuccess = (shot) => console.log(shot.target.blob);
(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #1)
> var s = iframe.getScreenshot(280, 280);
> s.onsuccess = (shot) => console.log(shot.target.blob);

Thanks Tim! I believe that this would be the right approach. However, I found – at least on OS X – a bug (bug 1265450) that makes it currently unusable. We'll probably needs a "rebuild" after such bug will be fixed – if I didn't misunderstood the behavior of `postIdleTask` –; in the meantime I used the approach we have with the current RDM.
Comment on attachment 8742622 [details]
MozReview Request: Bug 1262594 - Rebuild screenshot capturing after iframe mozbrowser; r=jryans

https://reviewboard.mozilla.org/r/47365/#review44249

Looks good overall, thanks for working on this!

::: devtools/client/responsive.html/components/browser.js:73
(Diff revision 1)
>  
>    componentWillUnmount() {
>      let { onContentResize } = this;
>      let browser = this.refs.browserContainer.querySelector("iframe.browser");
>      let mm = browser.frameLoader.messageManager;
>      mm.removeMessageListener("ResponsiveMode:OnContentResize", onContentResize);

Should we expose more methods in the `e10s` module so we don't use the "ResponsiveMode:" message prefix anywhere in here anymore?  If you add these, please re-request review.

::: devtools/client/responsive.html/utils/e10s.js:12
(Diff revision 1)
> -const promise = require("promise");
> +const { defer } = require("promise");
>  
> -module.exports = {
> +// The prefix used for RDM messages in content.
> +// see: devtools/client/responsivedesign/responsivedesign-child.js
> +const MESSAGE_PREFIX = "ResponsiveMode:";
> +const MESSAGE_SUFFIX = ":Done";

Nit: Maybe `REQUEST_DONE_SUFFIX` since it's only for the completion reply?

::: devtools/client/responsive.html/utils/e10s.js:15
(Diff revision 1)
> +// see: devtools/client/responsivedesign/responsivedesign-child.js
> +const MESSAGE_PREFIX = "ResponsiveMode:";
> +const MESSAGE_SUFFIX = ":Done";
> +
> +/**
> + * Resolves a promise the next time the specified `message` is send over the

Nit: send -> sent

::: devtools/client/responsive.html/utils/e10s.js:18
(Diff revision 1)
> +
> +/**
> + * Resolves a promise the next time the specified `message` is send over the
> + * given message manager.
> + * @param {nsIMessageListenerManager} mm
> + *    The Message Menager

Nit: Menager -> Manager

::: devtools/client/responsive.html/utils/e10s.js:43
(Diff revision 1)
> +/**
> + * Send a "request" over the given message manager, and returns a promise that
> + * is resolved when the request is complete.
> + *
> + * @param {nsIMessageListenerManager} mm
> + *    The Message Menager

Nit: Menager -> Manager

::: devtools/client/responsive.html/utils/e10s.js:46
(Diff revision 1)
> + *
> + * @param {nsIMessageListenerManager} mm
> + *    The Message Menager
> + * @param {String} message
> + *    The message. It will be prefixed with the constant `MESSAGE_PREFIX`, and
> + *    also suffixed with `MESSAGE_SUFFIX` for the completition message.

Nit: completition -> completion (or just "reply")

::: devtools/client/responsivedesign/responsivedesign-child.js:153
(Diff revision 1)
>      docShell.contentViewer.sticky = isSticky;
>    }
>  
>    function screenshot() {
>      let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> -    let width = content.innerWidth;
> +    let ratio = content.devicePixelRatio;

Ah, good catch, I forgot this was missing here.
Attachment #8742622 - Flags: review?(jryans) → review+
Comment on attachment 8742622 [details]
MozReview Request: Bug 1262594 - Rebuild screenshot capturing after iframe mozbrowser; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47365/diff/1-2/
Comment on attachment 8742622 [details]
MozReview Request: Bug 1262594 - Rebuild screenshot capturing after iframe mozbrowser; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47365/diff/2-3/
Comment on attachment 8742622 [details]
MozReview Request: Bug 1262594 - Rebuild screenshot capturing after iframe mozbrowser; r=jryans

My apologies, the 2nd revision seems to be an empty commit, this one should be fine!
Attachment #8742622 - Flags: review+ → review?(jryans)
Comment on attachment 8742622 [details]
MozReview Request: Bug 1262594 - Rebuild screenshot capturing after iframe mozbrowser; r=jryans

https://reviewboard.mozilla.org/r/47365/#review44475

Thanks, this looks great to me!
Attachment #8742622 - Flags: review?(jryans) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/849d0f0165a6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I confirm that the screenshots are correctly performed on Firefox 49.0a1 (2016-05-04) and on Windows 10 x64, Ubuntu 14.04 x86 and on Mac OS X 10.10.5.
I am marking this issue Verified-Fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.