From e54195ccfb947969ec76bb474af3ee675257d276 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Tue, 18 Aug 2020 18:48:44 -0700
Subject: [PATCH] chore: align page.pdf options to the rpc protocol (#3521)

Drive-by: remove unused devices from playwright impl.
---
 src/chromium/crPdf.ts                  | 40 ++++++++++----------------
 src/rpc/server/playwrightDispatcher.ts |  3 +-
 src/server/playwright.ts               |  5 ----
 src/types.ts                           |  7 ++---
 4 files changed, 20 insertions(+), 35 deletions(-)

diff --git a/src/chromium/crPdf.ts b/src/chromium/crPdf.ts
index e49937ad84..aa1f9adf6d 100644
--- a/src/chromium/crPdf.ts
+++ b/src/chromium/crPdf.ts
@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { assert, helper } from '../helper';
+import { assert } from '../helper';
 import * as types from '../types';
 import { CRSession } from './crConnection';
 import { readProtocolStream } from './crProtocolHelper';
@@ -41,31 +41,22 @@ const unitToPixels: { [key: string]: number } = {
   'mm': 3.78
 };
 
-function convertPrintParameterToInches(parameter: (string | number | undefined)): (number | undefined) {
-  if (typeof parameter === 'undefined')
+function convertPrintParameterToInches(text: string | undefined): number | undefined {
+  if (text === undefined)
     return undefined;
-  let pixels: number;
-  if (helper.isNumber(parameter)) {
-    // Treat numbers as pixel values to be aligned with phantom's paperSize.
-    pixels = parameter;
-  } else if (helper.isString(parameter)) {
-    const text: string = parameter;
-    let unit = text.substring(text.length - 2).toLowerCase();
-    let valueText = '';
-    if (unitToPixels.hasOwnProperty(unit)) {
-      valueText = text.substring(0, text.length - 2);
-    } else {
-      // In case of unknown unit try to parse the whole parameter as number of pixels.
-      // This is consistent with phantom's paperSize behavior.
-      unit = 'px';
-      valueText = text;
-    }
-    const value = Number(valueText);
-    assert(!isNaN(value), 'Failed to parse parameter value: ' + text);
-    pixels = value * unitToPixels[unit];
+  let unit = text.substring(text.length - 2).toLowerCase();
+  let valueText = '';
+  if (unitToPixels.hasOwnProperty(unit)) {
+    valueText = text.substring(0, text.length - 2);
   } else {
-    throw new Error('page.pdf() Cannot handle parameter type: ' + (typeof parameter));
+    // In case of unknown unit try to parse the whole parameter as number of pixels.
+    // This is consistent with phantom's paperSize behavior.
+    unit = 'px';
+    valueText = text;
   }
+  const value = Number(valueText);
+  assert(!isNaN(value), 'Failed to parse parameter value: ' + text);
+  const pixels = value * unitToPixels[unit];
   return pixels / 96;
 }
 
@@ -87,7 +78,6 @@ export class CRPDF {
       pageRanges = '',
       preferCSSPageSize = false,
       margin = {},
-      path = null
     } = options;
 
     let paperWidth = 8.5;
@@ -124,6 +114,6 @@ export class CRPDF {
       pageRanges,
       preferCSSPageSize
     });
-    return await readProtocolStream(this._client, result.stream!, path);
+    return await readProtocolStream(this._client, result.stream!, null);
   }
 }
diff --git a/src/rpc/server/playwrightDispatcher.ts b/src/rpc/server/playwrightDispatcher.ts
index 1c7efdfacd..4b9b52b6f9 100644
--- a/src/rpc/server/playwrightDispatcher.ts
+++ b/src/rpc/server/playwrightDispatcher.ts
@@ -21,11 +21,12 @@ import { Dispatcher, DispatcherScope } from './dispatcher';
 import { SelectorsDispatcher } from './selectorsDispatcher';
 import { Electron } from '../../server/electron';
 import { ElectronDispatcher } from './electronDispatcher';
+import { DeviceDescriptors } from '../../deviceDescriptors';
 
 export class PlaywrightDispatcher extends Dispatcher<Playwright, PlaywrightInitializer> implements PlaywrightChannel {
   constructor(scope: DispatcherScope, playwright: Playwright) {
     const electron = (playwright as any).electron as (Electron | undefined);
-    const deviceDescriptors = Object.entries(playwright.devices)
+    const deviceDescriptors = Object.entries(DeviceDescriptors)
         .map(([name, descriptor]) => ({ name, descriptor }));
     super(scope, playwright, 'Playwright', {
       chromium: new BrowserTypeDispatcher(scope, playwright.chromium),
diff --git a/src/server/playwright.ts b/src/server/playwright.ts
index f885329b9c..e5c20c0ce5 100644
--- a/src/server/playwright.ts
+++ b/src/server/playwright.ts
@@ -14,8 +14,6 @@
  * limitations under the License.
  */
 
-import * as types from '../types';
-import { DeviceDescriptors } from '../deviceDescriptors';
 import { Chromium } from './chromium';
 import { WebKit } from './webkit';
 import { Firefox } from './firefox';
@@ -24,14 +22,11 @@ import * as browserPaths from '../install/browserPaths';
 
 export class Playwright {
   readonly selectors = selectors;
-  readonly devices: types.Devices;
   readonly chromium: Chromium;
   readonly firefox: Firefox;
   readonly webkit: WebKit;
 
   constructor(packagePath: string, browsers: browserPaths.BrowserDescriptor[]) {
-    this.devices = DeviceDescriptors;
-
     const chromium = browsers.find(browser => browser.name === 'chromium');
     this.chromium = new Chromium(packagePath, chromium!);
 
diff --git a/src/types.ts b/src/types.ts
index 99f6204e60..fd075ca7e2 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -116,11 +116,10 @@ export type PDFOptions = {
   landscape?: boolean,
   pageRanges?: string,
   format?: string,
-  width?: string|number,
-  height?: string|number,
+  width?: string,
+  height?: string,
   preferCSSPageSize?: boolean,
-  margin?: {top?: string|number, bottom?: string|number, left?: string|number, right?: string|number},
-  path?: string,
+  margin?: {top?: string, bottom?: string, left?: string, right?: string},
 }
 
 export type CSSCoverageOptions = {