core(entity-classification): classify chrome extensions into separate entities #15017
Conversation
| // Make up an entity only for valid http/https URLs and Chrome extensions. | ||
| if (!isChromeExtension && !parsedUrl.protocol.startsWith('http')) return; | ||
|
|
||
| const rootDomain = isChromeExtension ? 'Chrome Extensions' : Util.getRootDomain(url); |
There was a problem hiding this comment.
we should probably translate this string
There was a problem hiding this comment.
Not sure if translating it here would be the correct approach, since this makes it to the JSON; querying by a specific entity might become difficult. One way we could translate it is to leave it as it is here, or define it as a CHROME_EXTENSIONS string and during reporting run through each and match to an available translation. Wdyt?
There was a problem hiding this comment.
CHROME_EXTENSIONS string and during reporting run through each and match to an available translation. Wdyt?
I think that's how our current translating pipeline works. The untranslated (english) version of this string will be translated by the time it is placed in the LHR, at least that's what I would hope. The report renderer would not do any translation of it's own.
Not sure if this would happen automatically, so if it's too much work I'm fine leaving it alone.
There was a problem hiding this comment.
With the latest change, this string moves to category field. I think we'll approach translations of all 3P categories as a separate PR.
|
| static makupChromeExtensionEntity(entityCache, url, optionalName) { | ||
| const origin = Util.getChromeExtensionOrigin(url); | ||
| const host = new URL(origin).host; | ||
| const name = optionalName || host; |
There was a problem hiding this comment.
I'm defaulting unknown chrome extensions' names to the host part of the extension origin URL, i.e., foobar in chrome-extension://foobar/baz.js. This should help searching on the web with that string to reach the extension.
| * @param {string=} optionalName | ||
| * @return {LH.Artifacts.Entity} | ||
| */ | ||
| static makeupChromeExtensionEntity_(entityCache, url, optionalName) { |
There was a problem hiding this comment.
nit: Can we use this convention. Easier to understand if I know the function is internal from the start.
| static makeupChromeExtensionEntity_(entityCache, url, optionalName) { | |
| static _makeupChromeExtensionEntity(entityCache, url, optionalName) { |
| /** | ||
| * @param {EntityCache} entityCache | ||
| * @param {string} url | ||
| * @param {string=} optionalName |
There was a problem hiding this comment.
nit
| * @param {string=} optionalName | |
| * @param {string=} name |
Fixes #15014.
Level 2 implementation as described on #15014.
URL.originreturns"null"forchrome-extension://so we reconstruct an origin viaprotocol + '://' + hostportion of the URL./cc @brendankenny