-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: remove protocol timeout for Page.navigate #6413
Changes from 4 commits
d798047
9af14cc
273166c
e0fa246
7dd175a
81c00a1
3c58396
0383285
175cc67
6f88aaf
c08cb46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -259,6 +259,8 @@ class Driver { | |
} | ||
|
||
/** | ||
* If > 0, timeout is used for the next call to 'sendCommand'. | ||
* If <= 0, no timeout is used. | ||
* NOTE: This can eventually be replaced when TypeScript | ||
* resolves https://github.com/Microsoft/TypeScript/issues/5453. | ||
* @param {number} timeout | ||
|
@@ -286,18 +288,20 @@ class Driver { | |
} | ||
} | ||
return new Promise(async (resolve, reject) => { | ||
const asyncTimeout = setTimeout((_ => { | ||
const asyncTimeout = timeout > 0 ? setTimeout((_ => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the ternary ends up stretching pretty far. What about just let asyncTimeout;
if (timeout > 0) {
asyncTimeout = setTimeout(...)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should also add a note about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have gone that route first, but it would need to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoops, yeah, The main issue is that you have to look down four lines to see the other half of the conditional, and the syntax makes it harder to parse than normal control flow mechanisms (like an if block). Generally long ternaries just aren't worth it, but at the very least, the null branch should be first |
||
const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); | ||
err.message += ` Method: ${method}`; | ||
reject(err); | ||
}), timeout); | ||
}), timeout) : null; | ||
try { | ||
const result = await this._connection.sendCommand(method, ...params); | ||
clearTimeout(asyncTimeout); | ||
resolve(result); | ||
} catch (err) { | ||
clearTimeout(asyncTimeout); | ||
reject(err); | ||
} finally { | ||
if (asyncTimeout) { | ||
clearTimeout(asyncTimeout); | ||
} | ||
} | ||
}); | ||
} | ||
|
@@ -840,7 +844,7 @@ class Driver { | |
// happen _after_ onload: https://crbug.com/768961 | ||
this.sendCommand('Page.enable'); | ||
this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); | ||
this.setNextProtocolTimeout(30 * 1000); | ||
this.setNextProtocolTimeout(0); // We don't need a timeout for Page.navigate. See #6413. | ||
this.sendCommand('Page.navigate', {url}); | ||
|
||
if (waitForLoad) { | ||
|
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.
The >/<= describe literally what's allowed, but probably aren't worth including. Maybe go with
timeout is used for the next call to 'sendCommand'. If 0, no timeout is used.