From e7e8524e1427c7da0302b43e33b792551a2e9238 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 18 Aug 2020 18:46:56 -0700 Subject: [PATCH] chore: remove screenshot path from the server side (#3519) Also fixes auto-detection of mime type based on path and adds tests. --- src/frames.ts | 32 ++++++--------------------- src/rpc/channels.ts | 2 -- src/rpc/client/elementHandle.ts | 27 +++++++++++++++++++--- src/rpc/client/page.ts | 6 +++-- src/rpc/protocol.yml | 1 - src/rpc/validator.ts | 1 - src/screenshotter.ts | 16 +------------- src/types.ts | 1 - test/elementhandle-screenshot.spec.ts | 12 ++++++++++ test/page-screenshot.spec.ts | 14 ++++++++++++ 10 files changed, 62 insertions(+), 50 deletions(-) diff --git a/src/frames.ts b/src/frames.ts index 54b80ac9c3..d9e8c0111a 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -15,8 +15,6 @@ * limitations under the License. */ -import * as fs from 'fs'; -import * as util from 'util'; import { ConsoleMessage } from './console'; import * as dom from './dom'; import { Events } from './events'; @@ -640,31 +638,23 @@ export class Frame { } async addScriptTag(options: { - url?: string; path?: string; - content?: string; - type?: string; + url?: string, + content?: string, + type?: string, }): Promise { const { url = null, - path = null, content = null, type = '' } = options; - if (!url && !path && !content) + if (!url && !content) throw new Error('Provide an object with a `url`, `path` or `content` property'); const context = await this._mainContext(); return this._raceWithCSPError(async () => { if (url !== null) return (await context.evaluateHandleInternal(addScriptUrl, { url, type })).asElement()!; - let result; - if (path !== null) { - let contents = await util.promisify(fs.readFile)(path, 'utf8'); - contents += '\n//# sourceURL=' + path.replace(/\n/g, ''); - result = (await context.evaluateHandleInternal(addScriptContent, { content: contents, type })).asElement()!; - } else { - result = (await context.evaluateHandleInternal(addScriptContent, { content: content!, type })).asElement()!; - } + const result = (await context.evaluateHandleInternal(addScriptContent, { content: content!, type })).asElement()!; // Another round trip to the browser to ensure that we receive CSP error messages // (if any) logged asynchronously in a separate task on the content main thread. if (this._page._delegate.cspErrorsAsynchronousForInlineScipts) @@ -699,26 +689,18 @@ export class Frame { } } - async addStyleTag(options: { url?: string; path?: string; content?: string; }): Promise { + async addStyleTag(options: { url?: string, content?: string }): Promise { const { url = null, - path = null, content = null } = options; - if (!url && !path && !content) + if (!url && !content) throw new Error('Provide an object with a `url`, `path` or `content` property'); const context = await this._mainContext(); return this._raceWithCSPError(async () => { if (url !== null) return (await context.evaluateHandleInternal(addStyleUrl, url)).asElement()!; - - if (path !== null) { - let contents = await util.promisify(fs.readFile)(path, 'utf8'); - contents += '\n/*# sourceURL=' + path.replace(/\n/g, '') + '*/'; - return (await context.evaluateHandleInternal(addStyleContent, contents)).asElement()!; - } - return (await context.evaluateHandleInternal(addStyleContent, content!)).asElement()!; }); diff --git a/src/rpc/channels.ts b/src/rpc/channels.ts index 79219e244b..72773adf87 100644 --- a/src/rpc/channels.ts +++ b/src/rpc/channels.ts @@ -1822,14 +1822,12 @@ export type ElementHandleQuerySelectorAllResult = { export type ElementHandleScreenshotParams = { timeout?: number, type?: 'png' | 'jpeg', - path?: string, quality?: number, omitBackground?: boolean, }; export type ElementHandleScreenshotOptions = { timeout?: number, type?: 'png' | 'jpeg', - path?: string, quality?: number, omitBackground?: boolean, }; diff --git a/src/rpc/client/elementHandle.ts b/src/rpc/client/elementHandle.ts index f0dbd84109..ba3ced28d5 100644 --- a/src/rpc/client/elementHandle.ts +++ b/src/rpc/client/elementHandle.ts @@ -18,13 +18,15 @@ import { ElementHandleChannel, JSHandleInitializer, ElementHandleScrollIntoViewI import { Frame } from './frame'; import { FuncOn, JSHandle, serializeArgument, parseResult } from './jsHandle'; import { ChannelOwner } from './channelOwner'; -import { helper, assert } from '../../helper'; +import { helper, assert, mkdirIfNeeded } from '../../helper'; import { SelectOption, FilePayload, Rect, SelectOptionOptions } from './types'; import * as fs from 'fs'; import * as mime from 'mime'; import * as path from 'path'; import * as util from 'util'; +const fsWriteFileAsync = util.promisify(fs.writeFile.bind(fs)); + export class ElementHandle extends JSHandle { readonly _elementChannel: ElementHandleChannel; @@ -175,9 +177,16 @@ export class ElementHandle extends JSHandle { }); } - async screenshot(options: ElementHandleScreenshotOptions = {}): Promise { + async screenshot(options: ElementHandleScreenshotOptions & { path?: string } = {}): Promise { return this._wrapApiCall('elementHandle.screenshot', async () => { - return Buffer.from((await this._elementChannel.screenshot(options)).binary, 'base64'); + const type = determineScreenshotType(options); + const result = await this._elementChannel.screenshot({ ...options, type }); + const buffer = Buffer.from(result.binary, 'base64'); + if (options.path) { + await mkdirIfNeeded(options.path); + await fsWriteFileAsync(options.path, buffer); + } + return buffer; }); } @@ -262,3 +271,15 @@ export async function convertInputFiles(files: string | FilePayload | string[] | })); return filePayloads; } + +export function determineScreenshotType(options: { path?: string, type?: 'png' | 'jpeg' }): 'png' | 'jpeg' | undefined { + if (options.path) { + const mimeType = mime.getType(options.path); + if (mimeType === 'image/png') + return 'png'; + else if (mimeType === 'image/jpeg') + return 'jpeg'; + throw new Error(`path: unsupported mime type "${mimeType}"`); + } + return options.type; +} diff --git a/src/rpc/client/page.ts b/src/rpc/client/page.ts index a3ce14147a..5b93bac37a 100644 --- a/src/rpc/client/page.ts +++ b/src/rpc/client/page.ts @@ -27,7 +27,7 @@ import { ChannelOwner } from './channelOwner'; import { ConsoleMessage } from './consoleMessage'; import { Dialog } from './dialog'; import { Download } from './download'; -import { ElementHandle } from './elementHandle'; +import { ElementHandle, determineScreenshotType } from './elementHandle'; import { Worker } from './worker'; import { Frame, FunctionWithSource, verifyLoadState, WaitForNavigationOptions } from './frame'; import { Keyboard, Mouse } from './input'; @@ -425,7 +425,9 @@ export class Page extends ChannelOwner { async screenshot(options: PageScreenshotOptions & { path?: string } = {}): Promise { return this._wrapApiCall('page.screenshot', async () => { - const buffer = Buffer.from((await this._channel.screenshot(options)).binary, 'base64'); + const type = determineScreenshotType(options); + const result = await this._channel.screenshot({ ...options, type }); + const buffer = Buffer.from(result.binary, 'base64'); if (options.path) { await mkdirIfNeeded(options.path); await fsWriteFileAsync(options.path, buffer); diff --git a/src/rpc/protocol.yml b/src/rpc/protocol.yml index 70bb50ee47..59538a62ce 100644 --- a/src/rpc/protocol.yml +++ b/src/rpc/protocol.yml @@ -1513,7 +1513,6 @@ ElementHandle: literals: - png - jpeg - path: string? quality: number? omitBackground: boolean? returns: diff --git a/src/rpc/validator.ts b/src/rpc/validator.ts index f20b41feb3..c5a780d4fd 100644 --- a/src/rpc/validator.ts +++ b/src/rpc/validator.ts @@ -718,7 +718,6 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme { scheme.ElementHandleScreenshotParams = tObject({ timeout: tOptional(tNumber), type: tOptional(tEnum(['png', 'jpeg'])), - path: tOptional(tString), quality: tOptional(tNumber), omitBackground: tOptional(tBoolean), }); diff --git a/src/screenshotter.ts b/src/screenshotter.ts index cf185b83dc..fd33d8a33b 100644 --- a/src/screenshotter.ts +++ b/src/screenshotter.ts @@ -15,11 +15,8 @@ * limitations under the License. */ -import * as fs from 'fs'; -import * as mime from 'mime'; -import * as util from 'util'; import * as dom from './dom'; -import { assert, helper, mkdirIfNeeded } from './helper'; +import { assert, helper } from './helper'; import { Page } from './page'; import * as types from './types'; import { rewriteErrorMessage } from './utils/stackTrace'; @@ -153,10 +150,6 @@ export class Screenshotter { if (shouldSetDefaultBackground) await this._page._delegate.setBackgroundColor(); progress.throwIfAborted(); // Avoid side effects. - if (options.path) { - await mkdirIfNeeded(options.path); - await util.promisify(fs.writeFile)(options.path, buffer); - } if ((options as any).__testHookAfterScreenshot) await (options as any).__testHookAfterScreenshot(); return buffer; @@ -206,13 +199,6 @@ function validateScreenshotOptions(options: types.ScreenshotOptions): 'png' | 'j if (options.type) { assert(options.type === 'png' || options.type === 'jpeg', 'Unknown options.type value: ' + options.type); format = options.type; - } else if (options.path) { - const mimeType = mime.getType(options.path); - if (mimeType === 'image/png') - format = 'png'; - else if (mimeType === 'image/jpeg') - format = 'jpeg'; - assert(format, 'Unsupported screenshot mime type: ' + mimeType); } if (!format) diff --git a/src/types.ts b/src/types.ts index 7c3ed5438b..99f6204e60 100644 --- a/src/types.ts +++ b/src/types.ts @@ -51,7 +51,6 @@ export type WaitForNavigationOptions = TimeoutOptions & { export type ElementScreenshotOptions = TimeoutOptions & { type?: 'png' | 'jpeg', - path?: string, quality?: number, omitBackground?: boolean, }; diff --git a/test/elementhandle-screenshot.spec.ts b/test/elementhandle-screenshot.spec.ts index 71f92c63be..50521363d3 100644 --- a/test/elementhandle-screenshot.spec.ts +++ b/test/elementhandle-screenshot.spec.ts @@ -19,6 +19,8 @@ import './base.fixture'; import utils from './utils'; const {WIRE, HEADLESS} = testOptions; import {PNG} from 'pngjs'; +import path from 'path'; +import fs from 'fs'; // Firefox headful produces a different image. const ffheadful = FFOX && !HEADLESS; @@ -369,3 +371,13 @@ it.skip(ffheadful)('should take screenshot of disabled button', async({page}) => const screenshot = await button.screenshot(); expect(screenshot).toBeInstanceOf(Buffer); }); + +it.skip(ffheadful)('path option should create subdirectories', async({page, server, golden, tmpDir}) => { + await page.setViewportSize({width: 500, height: 500}); + await page.goto(server.PREFIX + '/grid.html'); + await page.evaluate(() => window.scrollBy(50, 100)); + const elementHandle = await page.$('.box:nth-of-type(3)'); + const outputPath = path.join(tmpDir, 'these', 'are', 'directories', 'screenshot.png'); + await elementHandle.screenshot({path: outputPath}); + expect(await fs.promises.readFile(outputPath)).toMatchImage(golden('screenshot-element-bounding-box.png')); +}); diff --git a/test/page-screenshot.spec.ts b/test/page-screenshot.spec.ts index e0c0705706..f65bf89ff2 100644 --- a/test/page-screenshot.spec.ts +++ b/test/page-screenshot.spec.ts @@ -265,3 +265,17 @@ it.skip(ffheadful)('path option should create subdirectories', async({page, serv await page.screenshot({path: outputPath}); expect(await fs.promises.readFile(outputPath)).toMatchImage(golden('screenshot-sanity.png')); }); + +it.skip(ffheadful)('path option should detect jpeg', async({page, server, golden, tmpDir}) => { + await page.setViewportSize({ width: 100, height: 100 }); + await page.goto(server.EMPTY_PAGE); + const outputPath = path.join(tmpDir, 'screenshot.jpg'); + const screenshot = await page.screenshot({omitBackground: true, path: outputPath}); + expect(await fs.promises.readFile(outputPath)).toMatchImage(golden('white.jpg')); + expect(screenshot).toMatchImage(golden('white.jpg')); +}); + +it.skip(ffheadful)('path option should throw for unsupported mime type', async({page, server, golden, tmpDir}) => { + const error = await page.screenshot({ path: 'file.txt' }).catch(e => e); + expect(error.message).toContain('path: unsupported mime type "text/plain"'); +});