Skip to content

Commit

Permalink
feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
patrickhulce committed Oct 11, 2017
1 parent 5d34a09 commit 9c446ec
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 23 deletions.
14 changes: 7 additions & 7 deletions lighthouse-cli/test/smokehouse/a11y/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = [
initialUrl: 'http://localhost:10200/a11y/a11y_tester.html',
url: 'http://localhost:10200/a11y/a11y_tester.html',
audits: {
accesskeys: {
'accesskeys': {
score: false,
details: {
items: {
Expand Down Expand Up @@ -77,7 +77,7 @@ module.exports = [
},
},
},
bypass: {
'bypass': {
score: false,
details: {
items: {
Expand All @@ -101,7 +101,7 @@ module.exports = [
},
},
},
dlitem: {
'dlitem': {
score: false,
details: {
items: {
Expand Down Expand Up @@ -157,7 +157,7 @@ module.exports = [
},
},
},
label: {
'label': {
score: false,
details: {
items: {
Expand All @@ -181,15 +181,15 @@ module.exports = [
},
},
},
list: {
'list': {
score: false,
details: {
items: {
length: 1,
},
},
},
listitem: {
'listitem': {
score: false,
details: {
items: {
Expand All @@ -213,7 +213,7 @@ module.exports = [
},
},
},
tabindex: {
'tabindex': {
score: false,
details: {
items: {
Expand Down
43 changes: 29 additions & 14 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,29 @@ class Driver {
}

/**
* Evaluate an expression in the context of the current page.
* Evaluate an expression in the context of the current page. If useIsolation is true, the expression
* will be evaluated in a content script that has access to the page's DOM but whose JavaScript state
* is completely separate.
* Returns a promise that resolves on the expression's value.
* @param {string} expression
* @param {{useIsolation: boolean}=} options
* @return {!Promise<*>}
*/
evaluateAsync(expression, options) {
const {useIsolation = true} = options || {};
evaluateAsync(expression, options = {}) {
const contextIdPromise = options.useIsolation ?
this._getOrCreateIsolatedContextId() :
Promise.resolve(undefined);
return contextIdPromise.then(contextId => this._evaluateInContext(expression, contextId));
}

/**
* Evaluate an expression in the given execution context; an undefined contextId implies the main
* page without isolation.
* @param {string} expression
* @param {number|undefined} contextId
* @return {!Promise<*>}
*/
_evaluateInContext(expression, contextId) {
return new Promise((resolve, reject) => {
// If this gets to 60s and it hasn't been resolved, reject the Promise.
const asyncTimeout = setTimeout(
Expand All @@ -250,12 +265,9 @@ class Driver {
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true,
contextId,
};

if (useIsolation && typeof this._isolatedExecutionContextId === 'number') {
evaluationParams.contextId = this._isolatedExecutionContextId;
}

this.sendCommand('Runtime.evaluate', evaluationParams).then(result => {
clearTimeout(asyncTimeout);
const value = result.result.value;
Expand Down Expand Up @@ -445,7 +457,7 @@ class Driver {
function checkForQuiet(driver, resolve) {
if (cancelled) return;

return driver.evaluateAsync(checkForQuietExpression, {useIsolation: false})
return driver.evaluateAsync(checkForQuietExpression)
.then(timeSinceLongTask => {
if (cancelled) return;

Expand Down Expand Up @@ -607,9 +619,13 @@ class Driver {
}

/**
* @return {!Promise}
* @return {!Promise<number>}
*/
_createIsolatedWorld() {
_getOrCreateIsolatedContextId() {
if (typeof this._isolatedExecutionContextId === 'number') {
return Promise.resolve(this._isolatedExecutionContextId);
}

return this.sendCommand('Page.getResourceTree')
.then(data => {
const frameId = data.frameTree.frame.id;
Expand All @@ -618,7 +634,7 @@ class Driver {
.then(data => this._isolatedExecutionContextId = data.executionContextId);
}

_clearIsolatedWorld() {
_clearIsolatedContextId() {
this._isolatedExecutionContextId = undefined;
}

Expand Down Expand Up @@ -650,7 +666,7 @@ class Driver {
/* eslint-enable max-len */

return this._beginNetworkStatusMonitoring(url)
.then(_ => this._clearIsolatedWorld())
.then(_ => this._clearIsolatedContextId())
.then(_ => {
// These can 'race' and that's OK.
// We don't want to wait for Page.navigate's resolution, as it can now
Expand All @@ -661,7 +677,6 @@ class Driver {
})
.then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs,
networkQuietThresholdMs, cpuQuietThresholdMs, maxWaitMs))
.then(_ => this._createIsolatedWorld())
.then(_ => this._endNetworkStatusMonitoring());
}

Expand Down Expand Up @@ -978,7 +993,7 @@ class Driver {
const globalVarToPopulate = `window['__${funcName}StackTraces']`;
const collectUsage = () => {
return this.evaluateAsync(
`Array.from(${globalVarToPopulate}).map(item => JSON.parse(item))`, {useIsolation: false})
`Array.from(${globalVarToPopulate}).map(item => JSON.parse(item))`)
.then(result => {
if (!Array.isArray(result)) {
throw new Error(
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class Accessibility extends Gatherer {
return (${runA11yChecks.toString()}());
})()`;

return driver.evaluateAsync(expression).then(returnedValue => {
return driver.evaluateAsync(expression, {useIsolation: true}).then(returnedValue => {
if (!returnedValue) {
throw new Error('No axe-core results returned');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class JSLibraries extends Gatherer {
return (${detectLibraries.toString()}());
})()`;

return options.driver.evaluateAsync(expression, {useIsolation: false});
return options.driver.evaluateAsync(expression);
}
}

Expand Down

0 comments on commit 9c446ec

Please sign in to comment.