core(fr): minor renames… cdpSession, defaultSession, requestfinished #14097
Conversation
| @@ -13,10 +13,10 @@ const DEFAULT_PROTOCOL_TIMEOUT = 30000; | |||
| /** @implements {LH.Gatherer.FRProtocolSession} */ | |||
| class ProtocolSession { | |||
| /** | |||
| * @param {LH.Puppeteer.CDPSession} session | |||
| * @param {LH.Puppeteer.CDPSession} cdpSession | |||
There was a problem hiding this comment.
I'd be on board with pptrSession if this was external facing, but constructing these should (hopefully) always be internal. I'm +1 to cdpSession to match the class name in that case.
There was a problem hiding this comment.
Yeah I'm happy with both. Let's change it in the future if we keep referring to it as the pptr session.
| @@ -13,10 +13,10 @@ const DEFAULT_PROTOCOL_TIMEOUT = 30000; | |||
| /** @implements {LH.Gatherer.FRProtocolSession} */ | |||
| class ProtocolSession { | |||
| /** | |||
| * @param {LH.Puppeteer.CDPSession} session | |||
| * @param {LH.Puppeteer.CDPSession} cdpSession | |||
There was a problem hiding this comment.
I'd be on board with pptrSession if this was external facing, but constructing these should (hopefully) always be internal. I'm +1 to cdpSession to match the class name in that case.
Co-authored-by: Brendan Kenny <[email protected]>
I think the plan had something to do with creating multiple sessions, but yeah I'm fine with removing the weird |
|
(tag removed just because tests failing / I merged some PRs touching same files) |
requestloadedbut it fires from all thefinishedhandlers.. (also from the loadingfailed) path. so i'm renaming itrequestfinishedto be more accuratethis._sessionis that pptr instance but in everywhere else its ourFRProtocolSession, so this just clarifies that._sessionAND_defaultSession. I can't really tell why the duplicated_sessionhas existed, but it doesnt need to.none of these should change behavior in any way
the motivation here is improved understandability for #14093 and #14078. i think the wins here are worth the minor churn