chore: align some server-side methods with rpc calls (#3504)

- Never write to console on the server side - we use stdout for
  communication. This includes logPolitely and deprecate.
- Pass undefined instead of null in some BrowserContext methods.
- Use explicit _setFileChooserIntercepted instead of on/off magic.

src/browserContext.ts

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { isUnderTest, helper, deprecate} from './helper';
+import { helper } from './helper';
 import * as network from './network';
 import { Page, PageBinding } from './page';
 import { TimeoutSettings } from './timeoutSettings';
@@ -38,10 +38,10 @@ export interface BrowserContext extends EventEmitter {
   clearCookies(): Promise<void>;
   grantPermissions(permissions: string[], options?: { origin?: string }): Promise<void>;
   clearPermissions(): Promise<void>;
-  setGeolocation(geolocation: types.Geolocation | null): Promise<void>;
+  setGeolocation(geolocation?: types.Geolocation): Promise<void>;
   setExtraHTTPHeaders(headers: types.Headers): Promise<void>;
   setOffline(offline: boolean): Promise<void>;
-  setHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void>;
+  setHTTPCredentials(httpCredentials?: types.Credentials): Promise<void>;
   addInitScript(script: Function | string | { path?: string, content?: string }, arg?: any): Promise<void>;
   exposeBinding(name: string, playwrightBinding: frames.FunctionWithSource): Promise<void>;
   route(url: types.URLMatch, handler: network.RouteHandler): Promise<void>;
@@ -101,8 +101,8 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
   abstract clearCookies(): Promise<void>;
   abstract _doGrantPermissions(origin: string, permissions: string[]): Promise<void>;
   abstract _doClearPermissions(): Promise<void>;
-  abstract setGeolocation(geolocation: types.Geolocation | null): Promise<void>;
-  abstract _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void>;
+  abstract setGeolocation(geolocation?: types.Geolocation): Promise<void>;
+  abstract _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise<void>;
   abstract setExtraHTTPHeaders(headers: types.Headers): Promise<void>;
   abstract setOffline(offline: boolean): Promise<void>;
   abstract _doAddInitScript(expression: string): Promise<void>;
@@ -117,9 +117,7 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser
     return await this._doCookies(urls as string[]);
   }
 
-  setHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void> {
-    if (!isUnderTest())
-      deprecate(`context.setHTTPCredentials`, `warning: method |context.setHTTPCredentials()| is deprecated. Instead of changing credentials, create another browser context with new credentials.`);
+  setHTTPCredentials(httpCredentials?: types.Credentials): Promise<void> {
     return this._doSetHTTPCredentials(httpCredentials);
   }
 

src/chromium/crBrowser.ts

@@ -380,10 +380,10 @@ export class CRBrowserContext extends BrowserContextBase {
     await this._browser._session.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined });
   }
 
-  async setGeolocation(geolocation: types.Geolocation | null): Promise<void> {
+  async setGeolocation(geolocation?: types.Geolocation): Promise<void> {
     if (geolocation)
       geolocation = verifyGeolocation(geolocation);
-    this._options.geolocation = geolocation || undefined;
+    this._options.geolocation = geolocation;
     for (const page of this.pages())
       await (page._delegate as CRPage).updateGeolocation();
   }
@@ -400,8 +400,8 @@ export class CRBrowserContext extends BrowserContextBase {
       await (page._delegate as CRPage).updateOffline();
   }
 
-  async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void> {
-    this._options.httpCredentials = httpCredentials || undefined;
+  async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise<void> {
+    this._options.httpCredentials = httpCredentials;
     for (const page of this.pages())
       await (page._delegate as CRPage).updateHttpCredentials();
   }

src/firefox/ffBrowser.ts

@@ -288,11 +288,11 @@ export class FFBrowserContext extends BrowserContextBase {
     await this._browser._connection.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined });
   }
 
-  async setGeolocation(geolocation: types.Geolocation | null): Promise<void> {
+  async setGeolocation(geolocation?: types.Geolocation): Promise<void> {
     if (geolocation)
       geolocation = verifyGeolocation(geolocation);
-    this._options.geolocation = geolocation || undefined;
-    await this._browser._connection.send('Browser.setGeolocationOverride', { browserContextId: this._browserContextId || undefined, geolocation });
+    this._options.geolocation = geolocation;
+    await this._browser._connection.send('Browser.setGeolocationOverride', { browserContextId: this._browserContextId || undefined, geolocation: geolocation || null });
   }
 
   async setExtraHTTPHeaders(headers: types.Headers): Promise<void> {
@@ -308,9 +308,9 @@ export class FFBrowserContext extends BrowserContextBase {
     await this._browser._connection.send('Browser.setOnlineOverride', { browserContextId: this._browserContextId || undefined, override: offline ? 'offline' : 'online' });
   }
 
-  async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void> {
-    this._options.httpCredentials = httpCredentials || undefined;
-    await this._browser._connection.send('Browser.setHTTPCredentials', { browserContextId: this._browserContextId || undefined, credentials: httpCredentials });
+  async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise<void> {
+    this._options.httpCredentials = httpCredentials;
+    await this._browser._connection.send('Browser.setHTTPCredentials', { browserContextId: this._browserContextId || undefined, credentials: httpCredentials || null });
   }
 
   async _doAddInitScript(source: string) {

src/helper.ts

@@ -38,14 +38,6 @@ export type Listener = (...args: any[]) => void;
 
 const isInDebugMode = !!getFromENV('PWDEBUG');
 
-const deprecatedHits = new Set();
-export function deprecate(methodName: string, message: string) {
-  if (deprecatedHits.has(methodName))
-    return;
-  deprecatedHits.add(methodName);
-  console.warn(message);
-}
-
 class Helper {
 
   static evaluationString(fun: Function | string, ...args: any[]): string {
@@ -361,14 +353,6 @@ export function getFromENV(name: string) {
   return value;
 }
 
-export function logPolitely(toBeLogged: string) {
-  const logLevel = process.env.npm_config_loglevel;
-  const logLevelDisplay = ['silent', 'error', 'warn'].indexOf(logLevel || '') > -1;
-
-  if (!logLevelDisplay)
-    console.log(toBeLogged);  // eslint-disable-line no-console
-}
-
 const escapeGlobChars = new Set(['/', '$', '^', '+', '.', '(', ')', '=', '!', '|']);
 
 export const helper = Helper;

src/install/browserFetcher.ts

@@ -23,7 +23,7 @@ import * as ProgressBar from 'progress';
 import { getProxyForUrl } from 'proxy-from-env';
 import * as URL from 'url';
 import * as util from 'util';
-import { assert, logPolitely, getFromENV } from '../helper';
+import { assert, getFromENV } from '../helper';
 import * as browserPaths from './browserPaths';
 import { BrowserName, BrowserPlatform, BrowserDescriptor } from './browserPaths';
 
@@ -250,3 +250,11 @@ function httpRequest(url: string, method: string, response: (r: any) => void) {
   request.end();
   return request;
 }
+
+export function logPolitely(toBeLogged: string) {
+  const logLevel = process.env.npm_config_loglevel;
+  const logLevelDisplay = ['silent', 'error', 'warn'].indexOf(logLevel || '') > -1;
+
+  if (!logLevelDisplay)
+    console.log(toBeLogged);  // eslint-disable-line no-console
+}

src/install/installer.ts

@@ -15,7 +15,7 @@
  */
 
 import * as crypto from 'crypto';
-import { getFromENV, logPolitely } from '../helper';
+import { getFromENV } from '../helper';
 import * as fs from 'fs';
 import * as path from 'path';
 import * as util from 'util';
@@ -36,7 +36,7 @@ export async function installBrowsersWithProgressBar(packagePath: string) {
   const linksDir = path.join(browsersPath, '.links');
 
   if (getFromENV('PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD')) {
-    logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set');
+    browserFetcher.logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set');
     return false;
   }
   await fsMkdirAsync(linksDir,  { recursive: true });
@@ -65,7 +65,7 @@ async function validateCache(packagePath: string, browsersPath: string, linksDir
       }
     } catch (e) {
       if (linkTarget)
-        logPolitely('Failed to process descriptor at ' + linkTarget);
+        browserFetcher.logPolitely('Failed to process descriptor at ' + linkTarget);
       await fsUnlinkAsync(linkPath).catch(e => {});
     }
   }
@@ -77,7 +77,7 @@ async function validateCache(packagePath: string, browsersPath: string, linksDir
   for (const browserPath of usedBrowserPaths)
     directories.delete(browserPath);
   for (const directory of directories) {
-    logPolitely('Removing unused browser at ' + directory);
+    browserFetcher.logPolitely('Removing unused browser at ' + directory);
     await removeFolderAsync(directory).catch(e => {});
   }
 

src/page.ts

@@ -17,7 +17,7 @@
 
 import * as dom from './dom';
 import * as frames from './frames';
-import { assert, helper, Listener, debugLogger } from './helper';
+import { assert, helper, debugLogger } from './helper';
 import * as input from './input';
 import * as js from './javascript';
 import * as network from './network';
@@ -386,20 +386,8 @@ export class Page extends EventEmitter {
     }
   }
 
-  on(event: string | symbol, listener: Listener): this {
-    if (event === Events.Page.FileChooser) {
-      if (!this.listenerCount(event))
-        this._delegate.setFileChooserIntercepted(true);
-    }
-    super.on(event, listener);
-    return this;
-  }
-
-  removeListener(event: string | symbol, listener: Listener): this {
-    super.removeListener(event, listener);
-    if (event === Events.Page.FileChooser && !this.listenerCount(event))
-      this._delegate.setFileChooserIntercepted(false);
-    return this;
+  async _setFileChooserIntercepted(enabled: boolean): Promise<void> {
+    await this._delegate.setFileChooserIntercepted(enabled);
   }
 }
 

src/rpc/client/browserContext.ts

@@ -21,6 +21,7 @@ import * as network from './network';
 import { BrowserContextChannel, BrowserContextInitializer } from '../channels';
 import { ChannelOwner } from './channelOwner';
 import { helper } from '../../helper';
+import { isUnderTest, deprecate } from './clientHelper';
 import { Browser } from './browser';
 import { Events } from './events';
 import { TimeoutSettings } from '../../timeoutSettings';
@@ -157,6 +158,8 @@ export class BrowserContext extends ChannelOwner<BrowserContextChannel, BrowserC
   }
 
   async setHTTPCredentials(httpCredentials: { username: string, password: string } | null): Promise<void> {
+    if (!isUnderTest())
+      deprecate(`context.setHTTPCredentials`, `warning: method |context.setHTTPCredentials()| is deprecated. Instead of changing credentials, create another browser context with new credentials.`);
     return this._wrapApiCall('browserContext.setHTTPCredentials', async () => {
       await this._channel.setHTTPCredentials({ httpCredentials: httpCredentials || undefined });
     });

src/rpc/client/clientHelper.ts

@@ -0,0 +1,30 @@
+/**
+ * Copyright 2017 Google Inc. All rights reserved.
+ * Modifications copyright (c) Microsoft Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { isUnderTest as commonIsUnderTest } from '../../helper';
+
+const deprecatedHits = new Set();
+export function deprecate(methodName: string, message: string) {
+  if (deprecatedHits.has(methodName))
+    return;
+  deprecatedHits.add(methodName);
+  console.warn(message);
+}
+
+export function isUnderTest() {
+  return commonIsUnderTest();
+}

src/rpc/server/browserContextDispatcher.ts

@@ -92,7 +92,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, Browser
   }
 
   async setGeolocation(params: BrowserContextSetGeolocationParams): Promise<void> {
-    await this._context.setGeolocation(params.geolocation || null);
+    await this._context.setGeolocation(params.geolocation);
   }
 
   async setExtraHTTPHeaders(params: { headers: types.HeadersArray }): Promise<void> {
@@ -104,7 +104,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, Browser
   }
 
   async setHTTPCredentials(params: BrowserContextSetHTTPCredentialsParams): Promise<void> {
-    await this._context.setHTTPCredentials(params.httpCredentials || null);
+    await this._context.setHTTPCredentials(params.httpCredentials);
   }
 
   async addInitScript(params: { source: string }): Promise<void> {

src/rpc/server/pageDispatcher.ts

@@ -36,7 +36,6 @@ import { CRCoverage } from '../../chromium/crCoverage';
 
 export class PageDispatcher extends Dispatcher<Page, PageInitializer> implements PageChannel {
   private _page: Page;
-  private _onFileChooser: (fileChooser: FileChooser) => void;
 
   constructor(scope: DispatcherScope, page: Page) {
     // TODO: theoretically, there could be more than one frame already.
@@ -53,11 +52,10 @@ export class PageDispatcher extends Dispatcher<Page, PageInitializer> implements
     page.on(Events.Page.DOMContentLoaded, () => this._dispatchEvent('domcontentloaded'));
     page.on(Events.Page.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) }));
     page.on(Events.Page.Download, dialog => this._dispatchEvent('download', { download: new DownloadDispatcher(this._scope, dialog) }));
-    // We add this listener lazily, to avoid intercepting file chooser when noone listens.
-    this._onFileChooser = fileChooser => this._dispatchEvent('fileChooser', {
+    this._page.on(Events.Page.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', {
       element: new ElementHandleDispatcher(this._scope, fileChooser.element()),
       isMultiple: fileChooser.isMultiple()
-    });
+    }));
     page.on(Events.Page.FrameAttached, frame => this._onFrameAttached(frame));
     page.on(Events.Page.FrameDetached, frame => this._onFrameDetached(frame));
     page.on(Events.Page.Load, () => this._dispatchEvent('load'));
@@ -143,10 +141,7 @@ export class PageDispatcher extends Dispatcher<Page, PageInitializer> implements
   }
 
   async setFileChooserInterceptedNoReply(params: { intercepted: boolean }) {
-    if (params.intercepted)
-      this._page.on(Events.Page.FileChooser, this._onFileChooser);
-    else
-      this._page.removeListener(Events.Page.FileChooser, this._onFileChooser);
+    await this._page._setFileChooserIntercepted(params.intercepted);
   }
 
   async keyboardDown(params: { key: string }): Promise<void> {

src/server/chromium.ts

@@ -17,7 +17,7 @@
 
 import * as path from 'path';
 import * as os from 'os';
-import { getFromENV, logPolitely, helper } from '../helper';
+import { getFromENV, helper } from '../helper';
 import { CRBrowser } from '../chromium/crBrowser';
 import { Env } from './processLauncher';
 import { kBrowserCloseMessageId } from '../chromium/crConnection';
@@ -39,7 +39,6 @@ export class Chromium extends BrowserTypeBase {
     if (debugPort !== undefined) {
       if (Number.isNaN(debugPort))
         throw new Error(`PLAYWRIGHT_CHROMIUM_DEBUG_PORT must be a number, but is set to "${debugPortStr}"`);
-      logPolitely(`NOTE: Chromium will be launched in debug mode on port ${debugPort}`);
     }
 
     super(packagePath, browser, debugPort ? { webSocketRegex: /^DevTools listening on (ws:\/\/.*)$/, stream: 'stderr' } : null);

src/server/electron.ts

@@ -106,7 +106,7 @@ export class ElectronApplication extends EventEmitter {
         return page;
     }
 
-    return await this.waitForEvent(ElectronEvents.ElectronApplication.Window, (page: ElectronPage) => page._browserWindowId === windowId);
+    return await this._waitForEvent(ElectronEvents.ElectronApplication.Window, (page: ElectronPage) => page._browserWindowId === windowId);
   }
 
   context(): BrowserContext {
@@ -114,13 +114,13 @@ export class ElectronApplication extends EventEmitter {
   }
 
   async close() {
-    const closed = this.waitForEvent(ElectronEvents.ElectronApplication.Close);
+    const closed = this._waitForEvent(ElectronEvents.ElectronApplication.Close);
     await this._nodeElectronHandle!.evaluate(({ app }) => app.quit());
     this._nodeConnection.close();
     await closed;
   }
 
-  async waitForEvent(event: string, predicate?: Function): Promise<any> {
+  private async _waitForEvent(event: string, predicate?: Function): Promise<any> {
     const progressController = new ProgressController(this._timeoutSettings.timeout({}));
     if (event !== ElectronEvents.ElectronApplication.Close)
       this._browserContext._closePromise.then(error => progressController.abort(error));

src/webkit/wkBrowser.ts

@@ -286,10 +286,10 @@ export class WKBrowserContext extends BrowserContextBase {
     await Promise.all(this.pages().map(page => (page._delegate as WKPage)._clearPermissions()));
   }
 
-  async setGeolocation(geolocation: types.Geolocation | null): Promise<void> {
+  async setGeolocation(geolocation?: types.Geolocation): Promise<void> {
     if (geolocation)
       geolocation = verifyGeolocation(geolocation);
-    this._options.geolocation = geolocation || undefined;
+    this._options.geolocation = geolocation;
     const payload: any = geolocation ? { ...geolocation, timestamp: Date.now() } : undefined;
     await this._browser._browserSession.send('Playwright.setGeolocationOverride', { browserContextId: this._browserContextId, geolocation: payload });
   }
@@ -306,8 +306,8 @@ export class WKBrowserContext extends BrowserContextBase {
       await (page._delegate as WKPage).updateOffline();
   }
 
-  async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise<void> {
-    this._options.httpCredentials = httpCredentials || undefined;
+  async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise<void> {
+    this._options.httpCredentials = httpCredentials;
     for (const page of this.pages())
       await (page._delegate as WKPage).updateHttpCredentials();
   }