New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Capture native resolution while emulating. #72
Conversation
lib/Browser.js
Outdated
@@ -57,7 +57,6 @@ class Browser { | |||
let chromiumRevision = require('../package.json').puppeteer.chromium_revision; | |||
let revisionInfo = Downloader.revisionInfo(Downloader.currentPlatform(), chromiumRevision); | |||
console.assert(revisionInfo, 'Chromium revision is not downloaded. Run npm install'); | |||
this._chromeExecutable = revisionInfo.executablePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still needed
lib/EmulatedDevice.js
Outdated
@@ -0,0 +1,199 @@ | |||
// Copyright 2014 The Chromium Authors. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should go under /third_party/chromium
lib/Page.js
Outdated
@@ -260,51 +257,45 @@ class Page extends EventEmitter { | |||
/** | |||
* @param {string} html | |||
* @param {!Object=} options | |||
* @return {!Promise<!Response>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep returning the response here
lib/Page.js
Outdated
@@ -362,7 +421,7 @@ class Page extends EventEmitter { | |||
console.assert(Number.isInteger(options.quality), 'Expected options.quality to be an integer'); | |||
console.assert(options.quality >= 0 && options.quality <= 100, 'Expected options.quality to be between 0 and 100 (inclusive), got ' + options.quality); | |||
} | |||
console.assert(!options.clip || !options.fullPage, 'options.clip and options.fullPage are exclusive'); | |||
console.assert(!options.clip || !options['fullPage'], 'options.clip and options.fullPage are exclusive'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's be consistent here and use .fullPage
lib/Page.js
Outdated
|
||
// Await for a single raf rountrip to ensure basic rasterization is complete. | ||
await this.evaluate(() => { | ||
let callback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return new Promise(fulfill => requestAnimationFrame(fulfill));
/** | ||
* @return {!Promise<!Response>} | ||
*/ | ||
async reload() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have reload which just navigates to the same url?
async reload(options) {
await this.navigate(this.url(), options);
}
lib/Page.js
Outdated
@@ -509,36 +550,31 @@ class Page extends EventEmitter { | |||
* @param {!Promise} | |||
*/ | |||
async click(selector) { | |||
let center = await this.evaluate(selector => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this version of the method is better since it doesn't use DOM domain
lib/Page.js
Outdated
this._viewport = viewport; | ||
const needsReload = await EmulationManager.emulateViewport(this._client, viewport); | ||
if (needsReload) | ||
return this.reload(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await this.reload()
lib/Page.js
Outdated
this._viewport = viewport; | ||
const needsReload = await EmulationManager.emulateViewport(this._client, viewport); | ||
if (needsReload) | ||
return this.reload(); | ||
} | ||
|
||
/** | ||
* @return {!{width: number, height: number}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns Page.Viewport
lib/EmulationManager.js
Outdated
|
||
/** | ||
* @param {string} name | ||
* @return {string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove options
lib/EmulationManager.js
Outdated
]); | ||
|
||
let reloadNeeded = false; | ||
if (viewport.hasTouch && !EmulationManager._emulatingTouch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop _emulatingTouch so that it works in multitab world
lib/EmulationManager.js
Outdated
} | ||
|
||
if (!viewport.hasTouch && EmulationManager._emulatingTouch) { | ||
await client.send('Runtime.removeScriptToEvaluateOnNewDocument', EmulationManager._emulatingTouch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmulationManager._emulatingTouch = false;
No description provided.