[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stop the trace when multiple tabs are found. #639

Merged
merged 14 commits into from
Oct 28, 2016
Merged

Stop the trace when multiple tabs are found. #639

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2a9d09b
Updated gatherer to check if multiple service workers are registered
wardpeet Sep 8, 2016
eb0d9a9
Show error messages to the user
wardpeet Sep 8, 2016
e21fd75
Change error message and name of the function to checkForMultipleTabs…
wardpeet Sep 9, 2016
065d327
Updated multiple tabs open logic
wardpeet Sep 10, 2016
5d571f4
Make sw check more readable
wardpeet Sep 15, 2016
cc4b7b4
Fix message variable readability
wardpeet Sep 15, 2016
40919bb
Review changes
wardpeet Sep 17, 2016
5f7f0a6
Latest review changes
wardpeet Sep 22, 2016
acc3c4f
Review changes
wardpeet Oct 4, 2016
fcf2b0b
Merge branch 'master' into sw/warn-tabs
wardpeet Oct 25, 2016
3b4379e
Fixed review @brendankenny
wardpeet Oct 25, 2016
809c565
Fix fetching tabid for cri connection
wardpeet Oct 26, 2016
3d2d822
Fixes latest nits
wardpeet Oct 28, 2016
a6238f2
Merge branch 'master' into sw/warn-tabs
wardpeet Oct 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change error message and name of the function to checkForMultipleTabs… 
…Attached
  • Loading branch information
@wardpeet
wardpeet committed Oct 4, 2016
commit e21fd7572dbd091d5ea398b6c9ce11c8eac8a9f5
6 changes: 3 additions & 3 deletions lighthouse-core/gather/drivers/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,9 @@ class Driver {
/**
* Checks all serviceworkes and see if any duplications are running
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service workers

* @param {string} pageUrl
* @return {Promise}
*/
Copy link
Member
@brendankenny brendankenny Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a @return {!Promise} to be clear

checkForMultipleServiceWorkers(pageUrl) {
checkForMultipleTabsAttached(pageUrl) {
// Get necessary information of serviceWorkers
const getRegistrations = new Promise(resolve => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe pull this out into a getServiceWorkerRegistrations, like getServiceWorkerVersions? That way ServiceWorker.enable/ServiceWorker.disable is also handled

this.once('ServiceWorker.workerRegistrationUpdated', resolve);
Expand All @@ -239,9 +240,8 @@ class Driver {
});

if (activeServiceWorkers.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to do something slightly different.

So far we have all worker versions that are attached to the registration that matches on the scopeURL. Perf. That's what we need... so far.

Our versions look like this: https://chromedevtools.github.io/debugger-protocol-viewer/tot/ServiceWorker/#type-ServiceWorkerVersion

We need all controlledClients of all activeServiceWorkers

if totalControlledClients has > 1 items, then we definitely have multiple tabs currently attached to the SW. if it's just one, then it is probably the open tab that's about to be reloaded.

const activeSW = activeServiceWorkers[0].scriptURL;
throw new Error(
Copy link
Member
@paulirish paulirish Sep 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to call driver.disconnect around now, so we dont keep the debugger attached.

Also, Is there any other cleanup we need to do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will check if anything else needs to be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only needed to disconnect the driver. Couldn't find anything else. I will have a look at what happens when we close the trace in between.. (shouldn't be part of this PR)

`Unable to kill ServiceWorker(${activeSW}).`
'You probably have multiple tabs open.'
);
}

Expand Down
8 changes: 5 additions & 3 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ const path = require('path');
* i. beginEmulation
* ii. cleanAndDisableBrowserCaches
* iii. clearDataForOrigin
* iiii. checkForMultipleServiceWorkers
* iiiii. forceUpdateServiceWorkers
* iiii. checkForMultipleTabsAttached
*
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
Expand Down Expand Up @@ -94,6 +93,9 @@ class GatherRunner {
}).then(_ => {
// Clears storage for origin.
return driver.clearDataForOrigin(options.url);
}).then(_ => {
// Check Service Workers running
return driver.checkForMultipleTabsAttached(options.url);
Copy link
Member
@brendankenny brendankenny Sep 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like setupDriver has a weird order. Feel free to punt to a followup PR, but I feel like the multiple tabs check should come before clearingDataForOrigin, for instance (why bother if the SW clear isn't going to work anyways?). Not sure of optimal order, though.

});
}

Expand Down Expand Up @@ -221,7 +223,7 @@ class GatherRunner {
return driver.connect()
.then(_ => {
// Check Service Workers running
return driver.checkForMultipleServiceWorkers(options.url);
return driver.checkForMultipleTabsAttached(options.url);
})
.then(_ => GatherRunner.setupDriver(driver, options))

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ module.exports = function(url, flags, configJSON) {
// kick off a lighthouse run
resolve(Runner.run(driver, {url, flags, config}));
})).catch(err => {
if (err.message.toLowerCase().startsWith('unable to kill')) {
log.error('Multiple tabs', err.message);
if (err.message.toLowerCase().includes('multiple tabs')) {
log.error('status', err.message);
return Promise.reject('');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear why we reject upwards with an empty message in this case, but I think I'm missing something..

Copy link
Collaborator Author
@wardpeet wardpeet Sep 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason why is not to throw a runtime error... (#639 (comment)) and wanted to show a better error message so i do it in the catch but perhaps i need to do it in the lighthouse-cli instead? (while writing i think that's a better option)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulirish this one i still need to fix (forgot about this one)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good.

}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/fake-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ module.exports = {
return Promise.resolve();
},

checkForMultipleServiceWorkers() {
checkForMultipleTabsAttached() {
return Promise.resolve();
},

Expand Down
6 changes: 2 additions & 4 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,7 @@ describe('GatherRunner', function() {
},
cleanAndDisableBrowserCaches() {},
clearDataForOrigin() {},
checkForMultipleServiceWorkers() {},
forceUpdateServiceWorkers() {}
checkForMultipleTabsAttached() {}
};

return GatherRunner.setupDriver(driver, {
Expand All @@ -106,8 +105,7 @@ describe('GatherRunner', function() {
},
cleanAndDisableBrowserCaches() {},
clearDataForOrigin() {},
checkForMultipleServiceWorkers() {},
forceUpdateServiceWorkers() {}
checkForMultipleTabsAttached() {}
};

return GatherRunner.setupDriver(driver, {
Expand Down
19 changes: 8 additions & 11 deletions lighthouse-core/test/lib/drivers/driver-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,11 @@ describe('Browser Driver', () => {
}
});

const errorMsg = `Unable to kill ServiceWorker(${swUrl}).`;
return driverStub.checkForMultipleServiceWorkers(pageUrl)
.catch(err => {
assert.equal(err.message, errorMsg);
});
return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(false), _ => assert.ok(true));
});

it('will will succeed when old sw are deleted', () => {
it('will succeed when old sw are deleted', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this test still what you want given what @paulirish said about deleted service workers?

const pageUrl = 'https://example.com/';
const swUrl = `${pageUrl}sw.js`;
const registrations = [1, 2].map(i =>
Expand All @@ -125,11 +122,11 @@ describe('Browser Driver', () => {
}
});

const errorMsg = new RegExp(`Unable to kill ServiceWorker(${swUrl}).`);
assert.doesNotThrow(() => driverStub.checkForMultipleServiceWorkers(pageUrl), errorMsg);
return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(true), _ => assert.ok(false));
});

it('will will succeed when only one sw loaded', () => {
it('will succeed when only one sw loaded', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sw/service worker

const pageUrl = 'https://example.com/';
const swUrl = `${pageUrl}sw.js`;
const registrations = [createSWRegistration(1, pageUrl)];
Expand All @@ -144,8 +141,8 @@ describe('Browser Driver', () => {
}
});

const errorMsg = new RegExp(`Unable to kill ServiceWorker(${swUrl}).`);
assert.doesNotThrow(() => driverStub.checkForMultipleServiceWorkers(pageUrl), errorMsg);
return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(true), _ => assert.ok(false));
});
});

Expand Down