core(fps): make lhId less dependent on chrome internals #14272
Conversation
| * value (that doesn't expose the actual execution context id) to use for __lighthouseMainExecutionContextId. | ||
| * @type {number[]} | ||
| */ | ||
| this._executionContextIdentifiersUsed = []; |
There was a problem hiding this comment.
Initially I just did contextId - this._executionContextId, but this is problematic due to the existence of clearContextId.
core/lib/page-functions.js
Outdated
| window.__lighthouseExecutionContextId !== undefined ? | ||
| window.__lighthouseExecutionContextId : | ||
| 'page', | ||
| window.__lighthouseExecutionContextUniqueIdentifier ?? '?', |
There was a problem hiding this comment.
'page' wasn't super descriptive but it was something to denote the difference between isolated context elements and page-context. We could pick something else
core/lib/page-functions.js
Outdated
| @@ -498,6 +498,12 @@ function getNodeDetails(element) { | |||
| const selector = getNodeSelector(element); | |||
|
|
|||
| // Create an id that will be unique across all execution contexts. | |||
| // | |||
| // Made up of 3 components: | |||
| // - number unique to specific execution context | |||
There was a problem hiding this comment.
It would be great to document somewhere that we don't need this because we have multiple isolated contexts at the end of things, we just have the one. We only really need this number because some elements may have been saved by other gatherers in other execution contexts, and we need to be able to know they don't match any element saved by FPS because those contexts are long gone by the time FPS rolls around.
Functionally it's the same thing, but we currently make it sound more complicated than it really is (as if we're juggling multiple simultaneous isolated execution contexts).
There was a problem hiding this comment.
But we do have multiple execution contexts - the page context and the optional isolated context.
There was a problem hiding this comment.
Only the one isolated context. Equivalently, we only have one context ID at a time, including at the end of the run.
| const uniqueExecutionContextIdentifier = contextId === undefined ? | ||
| 0 : | ||
| this._executionContextIdentifiersCreated; | ||
|
|
There was a problem hiding this comment.
@brendankenny not sure if this is what you had in mind, but we need to be mindful of the "undefined"/"page" context here.
There was a problem hiding this comment.
Yeah, more or less, though I actually think the previous undefined (instead of 0) here is kind of cleaner--there is no isolated context for that case, it makes it clearer that it's separate from the numbering of the contexts, and it eliminates the need for the '?' case below.
paulirish
changed the title
connorjclark
changed the title
connorjclark
changed the title
connorjclark
changed the title
did you end up trying this? |
|
I did not because the new expectations seem better to me. No need to care about differences b/wn FR and legacy runners. |
|
Wasn't the point of this change that there shouldn't be any differences now? |
|
No-FR is gonna run gatherers in different order, right? Then the ids will be different. point was to just prevent the dependence on chrome doing...whatever it was doing that resulted in multiple execution ids being created before LH starts. |
Which would have been tested by reverting. |
The specific id for the execution context varies as Chrome internals does different work before we even begin Lighthouse gathering, and has already changed twice recently. Instead of using the exact id value, instead log the context ids we've used and use the indexOf from that array as the new execution-context specific position of the lhId.
These values should now only depend on the order in which we run our code (or whatever variance exists on the page that changes what elements our code inspects).