remove tab targetId dependency from multiple tab check #852
Conversation
brendankenny
force-pushed
the
notabid
branch
2 times, most recently
from
ca3beee
to
ffd8836
Compare
| @@ -229,7 +229,7 @@ function handleError(err: LightHouseError) { | |||
| if (err.code === 'ECONNREFUSED') { | |||
| showConnectionError(); | |||
| } else if (err.message.toLowerCase().includes('multiple tabs')) { | |||
There was a problem hiding this comment.
no need for these 2 lines anymore :) it will just go inside the else. (thanks for cleaning up)
There was a problem hiding this comment.
good call. Just needed the process.exit(_RUNTIME_ERROR_CODE) when it fails on that, didn't notice it was the same thing now.
But yeah, sorry I didn't realize that raw.js wouldn't work with what you were up to in #639. We could have made that PR go on even longer :) Does the simplified checkForMultipleTabsAttachedhere seem like it will still work now that we go to about:blank before starting? Testing it with multiple tabs in the CLI/extension seems OK.
brendankenny
force-pushed
the
notabid
branch
2 times, most recently
from
a673252
to
067dcd8
Compare
| swHasMoreThanOneClient = !!versions.find(ver => { | ||
| // Check if any of the service workers are the same (registration id) | ||
| versions.forEach(ver => { | ||
| // Ignore non-matching service worker version information. |
There was a problem hiding this comment.
"Ignore workers unaffiliated with this registration"
| @@ -198,56 +190,44 @@ class Driver { | |||
|
|
|||
| /** | |||
| * Rejects if any open tabs would share a service worker with the target URL. | |||
| * This includes the target tab, so navigation to something like about:blank | |||
| * should be done before calling. | |||
| * @param {!string} pageUrl | |||
| * @return {!Promise} | |||
| */ | |||
| checkForMultipleTabsAttached(pageUrl) { | |||
There was a problem hiding this comment.
name-wise this is not testing "multiple" anymore. wanna update?
There was a problem hiding this comment.
assertNoSameOriginServiceWorkerClients()? :)
Followup to #639. Unfortunately we don't necessarily have a tab ID (particularly in the case of the
rawconnection where Lighthouse doesn't establish the debugger connection), so we can't use the id to check againstcontrolledClientsof any active service workers.However, with the change to first navigate to
about:blankin #850, we know the current tab can't be controlled by a service worker for the target URL, so we can simplify the multiple tab check to just find any service worker for the target URL with any controlled clients at all.