-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
core(entity-classification): classify chrome extensions into separate entities #15017
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example report that you can share?
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably translate this string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
* @param {string=} optionalName | |
* @param {string=} name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good pending nits
Fixes #15014.
Level 2 implementation as described on #15014.
URL.origin
returns"null"
forchrome-extension://
so we reconstruct an origin viaprotocol + '://' + host
portion of the URL./cc @brendankenny