Closed
Bug 1262594
Opened 8 years ago
Closed 8 years ago
Rebuild screenshot capturing after iframe mozbrowser
Categories
(DevTools :: Responsive Design Mode, defect, P1)
DevTools
Responsive Design Mode
Tracking
(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+
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [multiviewport][triage] → [multiviewport] [mvp-rdm]
Updated•8 years ago
|
QA Contact: mihai.boldan
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → zer0
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
Comment 1•8 years ago
|
||
var s = iframe.getScreenshot(280, 280); s.onsuccess = (shot) => console.log(shot.target.blob);
Assignee | ||
Comment 2•8 years ago
|
||
(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.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47365/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47365/
Attachment #8742622 -
Flags: review?(jryans)
Reporter | ||
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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)
Reporter | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/849d0f0165a6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 11•8 years ago
|
||
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]
status-firefox49:
--- → verified
Flags: qe-verify+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•