From 0d3a7b32b4b6f4b14971679657c3f83736324d33 Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Mon, 8 Feb 2021 07:02:02 +0000 Subject: [PATCH] Bug 1568831 - [devtools] Make eyedropper fission compatible. r=ladybenko. The current implementation of the eyedropper was using drawWindow, which does not handle remote frames. To make the tool fission compatible, we re-use the screenshot-content actor which do support remote frames. We had to do a few modification on it though so the eyedropper would work the same as it does at the moment: - add a `disableFlash` property to the screenshot options, so we don't play the flash animation for the eyedropper - retrieve the page current zoom level from the screenshot-content actor so we can adjust the size of the resulting image - split the `dpr` screenshot option into 2 distinct ones: - `snapshotScale`, which will be the scale passed to `drawSnapshot`. By default, this will be the dpr of the window, so the screenshot is as crisp as it can be. - `fileScale`, which will be the scale of the resulting file. - we control the fileScale that is sent to the server with the `ignoreDprForFileScale` on the client. This is needed by the eyedropper so it doesn't draw images that are too big when the dpr is > 1. With all those changes, browser_inspector_highlighter-eyedropper-frames.js is passing with fission enabled, so we can remove the `fail-if` annotation it had. Differential Revision: https://phabricator.services.mozilla.com/D103879 --- devtools/client/fronts/inspector.js | 40 +++++- devtools/client/inspector/test/browser.ini | 1 - devtools/client/shared/screenshot.js | 131 +++++++++++------- .../server/actors/highlighters/eye-dropper.js | 49 +++++-- devtools/server/actors/screenshot-content.js | 11 +- .../server/actors/utils/capture-screenshot.js | 28 ++-- 6 files changed, 180 insertions(+), 80 deletions(-) diff --git a/devtools/client/fronts/inspector.js b/devtools/client/fronts/inspector.js index d55f83bd673b..429d18411c12 100644 --- a/devtools/client/fronts/inspector.js +++ b/devtools/client/fronts/inspector.js @@ -12,6 +12,13 @@ const { } = require("devtools/shared/protocol.js"); const { inspectorSpec } = require("devtools/shared/specs/inspector"); +loader.lazyRequireGetter( + this, + "captureScreenshot", + "devtools/client/shared/screenshot", + true +); + const TELEMETRY_EYEDROPPER_OPENED = "DEVTOOLS_EYEDROPPER_OPENED_COUNT"; const TELEMETRY_EYEDROPPER_OPENED_MENU = "DEVTOOLS_MENU_EYEDROPPER_OPENED_COUNT"; @@ -156,7 +163,38 @@ class InspectorFront extends FrontClassWithSpec(inspectorSpec) { } async pickColorFromPage(options) { - await super.pickColorFromPage(options); + let screenshot = null; + + // @backward-compat { version 87 } ScreenshotContentActor was only added in 87. + // When connecting to older server, the eyedropper will use drawWindow + // to retrieve the screenshot of the page (that's a decent fallback, + // even if it doesn't handle remote frames). + if (this.targetFront.hasActor("screenshotContent")) { + try { + // We use the screenshot actors as it can retrieve an image of the current viewport, + // handling remote frame if need be. + const { data } = await captureScreenshot(this.targetFront, { + browsingContextID: this.targetFront.browsingContextID, + disableFlash: true, + ignoreDprForFileScale: true, + }); + screenshot = data; + } catch (e) { + // We simply log the error and still call pickColorFromPage as it will default to + // use drawWindow in order to get the screenshot of the page (that's a decent + // fallback, even if it doesn't handle remote frames). + console.error( + "Error occured when taking a screenshot for the eyedropper", + e + ); + } + } + + await super.pickColorFromPage({ + ...options, + screenshot, + }); + if (options?.fromMenu) { telemetry.getHistogramById(TELEMETRY_EYEDROPPER_OPENED_MENU).add(true); } else { diff --git a/devtools/client/inspector/test/browser.ini b/devtools/client/inspector/test/browser.ini index 6d3369756222..250d3dec0a25 100644 --- a/devtools/client/inspector/test/browser.ini +++ b/devtools/client/inspector/test/browser.ini @@ -120,7 +120,6 @@ skip-if = (os == 'linux' && bits == 32 && debug) # bug 1328915, disable linux32 [browser_inspector_highlighter-eyedropper-events.js] skip-if = os == 'win' # bug 1413442 [browser_inspector_highlighter-eyedropper-frames.js] -fail-if = fission [browser_inspector_highlighter-eyedropper-image.js] [browser_inspector_highlighter-eyedropper-label.js] [browser_inspector_highlighter-eyedropper-show-hide.js] diff --git a/devtools/client/shared/screenshot.js b/devtools/client/shared/screenshot.js index fdfbda370c69..1a31d339128c 100644 --- a/devtools/client/shared/screenshot.js +++ b/devtools/client/shared/screenshot.js @@ -32,6 +32,12 @@ const L10N = new LocalizationHelper(STRINGS_URI); * @param {Number} args.delay: Number of seconds to wait before taking the screenshot * @param {Boolean} args.help: Set to true to receive a message with the screenshot command * documentation. + * @param {Boolean} args.disableFlash: Set to true to disable the flash animation when the + * screenshot is taken. + * @param {Boolean} args.ignoreDprForFileScale: Set to true to if the resulting screenshot + * file size shouldn't be impacted by the dpr. Note that the dpr will still + * be taken into account when taking the screenshot, only the size of the + * file will be different. * @returns {Array} An array of object representing the different * messages emitted throught the process, that should be displayed to the user. */ @@ -41,58 +47,7 @@ async function captureAndSaveScreenshot(targetFront, window, args = {}) { return [{ text: getFormattedHelpData() }]; } - // @backward-compat { version 87 } The screenshot-content actor was introduced in 87, - // so we can always use it once 87 reaches release. - const supportsContentScreenshot = targetFront.hasActor("screenshotContent"); - - let captureResponse; - if (supportsContentScreenshot) { - if (args.delay > 0) { - await new Promise(res => setTimeout(res, args.delay * 1000)); - } - - const screenshotContentFront = await targetFront.getFront( - "screenshot-content" - ); - - // Call the content-process on the server to retrieve informations that will be needed - // by the parent process. - const { - rect, - windowDpr, - messages, - error, - } = await screenshotContentFront.prepareCapture(args); - - if (error) { - return messages; - } - - if (rect) { - args.rect = rect; - } - - if (!args.dpr) { - args.dpr = windowDpr; - } - - args.browsingContextID = targetFront.browsingContextID; - - // We can now call the parent process which will take the screenshot via - // the drawSnapshot API - const rootFront = targetFront.client.mainRoot; - const parentProcessScreenshotFront = await rootFront.getFront("screenshot"); - captureResponse = await parentProcessScreenshotFront.capture(args); - messages.push(...(captureResponse.messages || [])); - - // Reset the page to the state it was in before taking the screenshot (e.g. set the - // scroll position as it was before). This is a oneway method so there's nothing to - // wait for. - screenshotContentFront.captureDone(); - } else { - const screenshotFront = await targetFront.getFront("screenshot"); - captureResponse = await screenshotFront.capture(args); - } + const captureResponse = await captureScreenshot(targetFront, args); if (captureResponse.error) { return captureResponse.messages || []; @@ -102,6 +57,72 @@ async function captureAndSaveScreenshot(targetFront, window, args = {}) { return (captureResponse.messages || []).concat(saveMessages); } +/** + * Take a screenshot of a browser element matching the passed target + * @param {TargetFront} targetFront: The targetFront of the frame we want to take a screenshot of. + * @param {Object} args: See args param in captureAndSaveScreenshot + */ +async function captureScreenshot(targetFront, args) { + // @backward-compat { version 87 } The screenshot-content actor was introduced in 87, + // so we can always use it once 87 reaches release. + const supportsContentScreenshot = targetFront.hasActor("screenshotContent"); + if (!supportsContentScreenshot) { + const screenshotFront = await targetFront.getFront("screenshot"); + return screenshotFront.capture(args); + } + + if (args.delay > 0) { + await new Promise(res => setTimeout(res, args.delay * 1000)); + } + + const screenshotContentFront = await targetFront.getFront( + "screenshot-content" + ); + + // Call the content-process on the server to retrieve informations that will be needed + // by the parent process. + const { + rect, + windowDpr, + windowZoom, + messages, + error, + } = await screenshotContentFront.prepareCapture(args); + + if (error) { + return { error, messages }; + } + + if (rect) { + args.rect = rect; + } + + args.dpr ||= windowDpr; + + args.snapshotScale = args.dpr * windowZoom; + if (args.ignoreDprForFileScale) { + args.fileScale = windowZoom; + } + + args.browsingContextID = targetFront.browsingContextID; + + // We can now call the parent process which will take the screenshot via + // the drawSnapshot API + const rootFront = targetFront.client.mainRoot; + const parentProcessScreenshotFront = await rootFront.getFront("screenshot"); + const captureResponse = await parentProcessScreenshotFront.capture(args); + + // Reset the page to the state it was in before taking the screenshot (e.g. set the + // scroll position as it was before). This is a oneway method so there's nothing to + // wait for. + screenshotContentFront.captureDone(); + + return { + ...captureResponse, + messages: (messages || []).concat(captureResponse.messages || []), + }; +} + const screenshotDescription = L10N.getStr("screenshotDesc"); const screenshotGroupOptions = L10N.getStr("screenshotGroupOptions"); const screenshotCommandParams = [ @@ -362,4 +383,8 @@ async function saveToFile(image) { } } -module.exports = { captureAndSaveScreenshot, saveScreenshot }; +module.exports = { + captureAndSaveScreenshot, + captureScreenshot, + saveScreenshot, +}; diff --git a/devtools/server/actors/highlighters/eye-dropper.js b/devtools/server/actors/highlighters/eye-dropper.js index a8d3869ae44a..b0f3e8b8e97d 100644 --- a/devtools/server/actors/highlighters/eye-dropper.js +++ b/devtools/server/actors/highlighters/eye-dropper.js @@ -134,9 +134,13 @@ class EyeDropper { /** * Show the eye-dropper highlighter. + * * @param {DOMNode} node The node which document the highlighter should be inserted in. * @param {Object} options The options object may contain the following properties: - * - {Boolean} copyOnSelect Whether selecting a color should copy it to the clipboard. + * - {Boolean} copyOnSelect: Whether selecting a color should copy it to the clipboard. + * - {String|null} screenshot: a dataURL representation of the page screenshot. If null, + * the eyedropper will use `drawWindow` to get the the screenshot + * (⚠️ but it won't handle remote frames). */ show(node, options = {}) { if (this.highlighterEnv.isXUL) { @@ -152,7 +156,7 @@ class EyeDropper { // eyedropper UI will appear in the screenshot itself (since the UI is injected as // native anonymous content in the page). // Once the screenshot is ready, the magnified area will be drawn. - this.prepareImageCapture(); + this.prepareImageCapture(options.screenshot); // Start listening for user events. const { pageListenerTarget } = this.highlighterEnv; @@ -216,22 +220,41 @@ class EyeDropper { this.win.document.setSuppressedEventListener(null); } - prepareImageCapture() { - // Get the image data from the content window. - const imageData = getWindowAsImageData(this.win); + /** + * Create an image bitmap from the page screenshot, draw the eyedropper and set the + * "drawn" attribute on the "root" element once it's done. + * + * @params {String|null} screenshot: a dataURL representation of the page screenshot. + * If null, we'll use `drawWindow` to get the the page screenshot + * (⚠️ but it won't handle remote frames). + */ + async prepareImageCapture(screenshot) { + let imgData; + if (screenshot) { + // If a screenshot data URL was passed, we create an image tag with it which we + // use to create an image bitmap. + imgData = this.win.document.createElement("img"); + const onImgLoaded = new Promise(resolve => + imgData.addEventListener("load", resolve, { once: true }) + ); + imgData.src = screenshot; + await onImgLoaded; + } else { + imgData = getWindowAsImageData(this.win); + } // We need to transform imageData to something drawWindow will consume. An ImageBitmap // works well. We could have used an Image, but doing so results in errors if the page // defines CSP headers. - this.win.createImageBitmap(imageData).then(image => { - this.pageImage = image; - // We likely haven't drawn anything yet (no mousemove events yet), so start now. - this.draw(); + const image = await this.win.createImageBitmap(imgData); - // Set an attribute on the root element to be able to run tests after the first draw - // was done. - this.getElement("root").setAttribute("drawn", "true"); - }); + this.pageImage = image; + // We likely haven't drawn anything yet (no mousemove events yet), so start now. + this.draw(); + + // Set an attribute on the root element to be able to run tests after the first draw + // was done. + this.getElement("root").setAttribute("drawn", "true"); } /** diff --git a/devtools/server/actors/screenshot-content.js b/devtools/server/actors/screenshot-content.js index 413e32ad5dc5..fb766c6c5d63 100644 --- a/devtools/server/actors/screenshot-content.js +++ b/devtools/server/actors/screenshot-content.js @@ -11,7 +11,12 @@ const { const { LocalizationHelper } = require("devtools/shared/l10n"); const STRINGS_URI = "devtools/shared/locales/screenshot.properties"; const L10N = new LocalizationHelper(STRINGS_URI); -loader.lazyRequireGetter(this, "getRect", "devtools/shared/layout/utils", true); +loader.lazyRequireGetter( + this, + ["getCurrentZoom", "getRect"], + "devtools/shared/layout/utils", + true +); exports.ScreenshotContentActor = ActorClassWithSpec(screenshotContentSpec, { initialize: function(conn, targetActor) { @@ -41,6 +46,7 @@ exports.ScreenshotContentActor = ActorClassWithSpec(screenshotContentSpec, { * - messages {Array}: An array of objects representing * the messages emitted throught the process and their level. * - windowDpr {Number}: Value of window.devicePixelRatio + * - windowZoom {Number}: The page current zoom level * - rect {Object}: Object with left, top, width and height properties * representing the rect **inside the browser element** that should be rendered. * For screenshot of the current viewport, we return null, as expected by the @@ -51,6 +57,7 @@ exports.ScreenshotContentActor = ActorClassWithSpec(screenshotContentSpec, { const { window } = this.targetActor; const windowDpr = window.devicePixelRatio; + const windowZoom = getCurrentZoom(window); const messages = []; // If we're going to take the current view of the page, we don't need to compute a rect, @@ -60,6 +67,7 @@ exports.ScreenshotContentActor = ActorClassWithSpec(screenshotContentSpec, { rect: null, messages, windowDpr, + windowZoom, }; } @@ -127,6 +135,7 @@ exports.ScreenshotContentActor = ActorClassWithSpec(screenshotContentSpec, { return { windowDpr, + windowZoom, rect: { left, top, width, height }, messages, }; diff --git a/devtools/server/actors/utils/capture-screenshot.js b/devtools/server/actors/utils/capture-screenshot.js index 70b49df7f395..f148f84630a0 100644 --- a/devtools/server/actors/utils/capture-screenshot.js +++ b/devtools/server/actors/utils/capture-screenshot.js @@ -57,9 +57,13 @@ function simulateCameraFlash(browsingContext) { * be rendered. If null, the current viewport of the element will be rendered. * @param {Boolean} args.fullpage: Should the screenshot be the height of the whole page * @param {String} args.filename: Expected filename for the screenshot - * @param {Number} args.dpr: Scale of the screenshot. ⚠️ Note that the scale might be - * decreased if the resulting image would be too big to draw safely. - * Warning message will be returned if that's the case. + * @param {Number} args.snapshotScale: Scale that will be used by `drawSnapshot` to take the screenshot. + * ⚠️ Note that the scale might be decreased if the resulting image would + * be too big to draw safely. A warning message will be returned if that's + * the case. + * @param {Number} args.fileScale: Scale of the exported file. Defaults to args.snapshotScale. + * @param {Boolean} args.disableFlash: Set to true to disable the flash animation when the + * screenshot is taken. * @param {BrowsingContext} browsingContext * @returns {Object} object with the following properties: * - data {String}: The dataURL representing the screenshot @@ -100,8 +104,6 @@ async function captureScreenshot(args, browsingContext) { ); } - const ratio = args.dpr ? args.dpr : 1; - const document = browsingContext.topChromeWindow.document; const canvas = document.createElementNS( "http://www.w3.org/1999/xhtml", @@ -119,12 +121,15 @@ async function captureScreenshot(args, browsingContext) { "rgb(255,255,255)" ); - canvas.width = snapshot.width; - canvas.height = snapshot.height; - width = snapshot.width; - height = snapshot.height; + const fileScale = args.fileScale || actualRatio; + const renderingWidth = (snapshot.width / actualRatio) * fileScale; + const renderingHeight = (snapshot.height / actualRatio) * fileScale; + canvas.width = renderingWidth; + canvas.height = renderingHeight; + width = renderingWidth; + height = renderingHeight; const ctx = canvas.getContext("2d"); - ctx.drawImage(snapshot, 0, 0); + ctx.drawImage(snapshot, 0, 0, renderingWidth, renderingHeight); // Bug 1574935 - Huge dimensions can trigger an OOM because multiple copies // of the bitmap will exist in memory. Force the removal of the snapshot @@ -137,6 +142,7 @@ async function captureScreenshot(args, browsingContext) { } }; + const ratio = args.snapshotScale; let data = await drawToCanvas(ratio); if (!data && ratio > 1.0) { // If the user provided DPR or the window.devicePixelRatio was higher than 1, @@ -154,7 +160,7 @@ async function captureScreenshot(args, browsingContext) { }); } - if (data) { + if (data && args.disableFlash !== true) { simulateCameraFlash(browsingContext); }