Skip to content
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

report: use entity classification to filter third-parties #14697

Merged
merged 143 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
143 commits
Select commit Hold shift + click to select a range
7833354
First cut of entity classification computed artifact.
alexnj Oct 20, 2022
0a154eb
some cleanup on the types
alexnj Oct 20, 2022
3212aa6
Refactor resource-summary audit with third party classification
alexnj Oct 20, 2022
c71a8ab
Refactor unused-javascript to output entity and is-3p flag
alexnj Oct 21, 2022
14163bd
Refactor 3p audits to depend on computed entity classification.
alexnj Nov 1, 2022
46d706c
Expose entity classification to LH report via a hidden audit.
alexnj Nov 1, 2022
b5caa4f
Refine groupBy feature to be more concise.
alexnj Nov 1, 2022
8689903
Replace domains with homepage
alexnj Nov 8, 2022
5b67f2e
Revert the LHR grouping changes, after discussing the design with team.
alexnj Nov 8, 2022
0abc053
Add name based lookup to entity classification audit result.
alexnj Nov 11, 2022
170adcb
Refactor third-party filter to base on entity-classification
alexnj Nov 14, 2022
5e18f75
Attach entity classification to ByteEfficiencyAudit.
alexnj Nov 14, 2022
f0674c7
Classify bootup-time (Reduce JavaScript execution time) audit
alexnj Nov 14, 2022
69725b7
Classify long-tasks (Avoid long main-thread tasks) audit
alexnj Nov 14, 2022
ab5d847
Classify uses-long-cache-ttl audit.
alexnj Nov 15, 2022
04bfff8
Mark sub-items with entity as well
alexnj Nov 15, 2022
e8c1c7e
Fix the regression caused to third-party-summary audit
alexnj Nov 15, 2022
0a87d78
Classify uses-rel-preconnect audit.
alexnj Nov 15, 2022
d953829
Classify all ViolationAudit derived audits.
alexnj Nov 15, 2022
aea71bf
Classify no-unload-listeners audit
alexnj Nov 15, 2022
cb16f02
Classify total-byte-weight audit
alexnj Nov 15, 2022
1e73c13
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Nov 16, 2022
e79f44b
Updated components/CSS
alexnj Nov 16, 2022
6f80513
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Nov 16, 2022
d299ae8
Some cleanup
alexnj Nov 16, 2022
f462dc8
Classify valid-source-maps audit
alexnj Nov 16, 2022
9189494
Classify legacy-javascript audit
alexnj Nov 16, 2022
f1d9893
Classify all audits that depend on makeOpportunityDetails call
alexnj Nov 17, 2022
1ad67ac
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Dec 5, 2022
0230896
Explicitly name the lookup tables.
alexnj Dec 8, 2022
abf81f9
Update types on entity classification
alexnj Dec 9, 2022
73cc72d
Update unit tests of ViolationAudit based tests.
alexnj Dec 9, 2022
1daccab
Unit tests: Legacy Javascript and Bootup Time.
alexnj Dec 9, 2022
8cd7347
Fix the bug introduced into firstPartyHostname calculation.
alexnj Dec 9, 2022
4a44dc6
Bugfix: Rely on Artifacts.URL.mainDocumentUrl for 1P calc.
alexnj Dec 9, 2022
60c609a
Classify and update unit tests of 3P facades audit.
alexnj Dec 9, 2022
39753fe
Unit tests for 3p facades test.
alexnj Dec 9, 2022
0459e38
First party detection should fall back to finalDisplayedUrl.
alexnj Dec 10, 2022
a9e5d6a
Update third-party-summary tests.
alexnj Dec 12, 2022
f7adfe3
Allow audits to override entity on items.
alexnj Dec 13, 2022
21a53ba
update tests for valid-source-maps
alexnj Dec 13, 2022
da02b4f
Update snapshots + flow
alexnj Dec 13, 2022
8780384
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Dec 13, 2022
880c09e
Make third-party filtering backward compatible to old LHRs.
alexnj Dec 13, 2022
86fc747
Remove extra space
alexnj Dec 13, 2022
122486e
Unit tests for Audit.makeOpportunityDetails
alexnj Dec 13, 2022
d14d703
Updated tests cases set.
alexnj Dec 14, 2022
ac3f59e
Updated tests set.
alexnj Dec 14, 2022
d373c45
Remove redundant entity marker.
alexnj Dec 14, 2022
171d563
some minor fixups.
alexnj Dec 14, 2022
1059e6a
Entity classification unit tests.
alexnj Dec 15, 2022
ad43021
Computed entity-classification artifact unit tests.
alexnj Dec 15, 2022
02f212a
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Dec 15, 2022
99a9a67
Fix navigation and timespan E2E audit counts.
alexnj Dec 15, 2022
793b1ae
Explicitly handle entity-classification details.type
alexnj Dec 15, 2022
6eef166
Shorter explanation for tsc-expect-error
alexnj Dec 15, 2022
2667564
apparently i cant see squiggly lines
alexnj Dec 15, 2022
36bff06
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Dec 15, 2022
2f50be6
Review fixes
alexnj Dec 16, 2022
093d60c
Refactor entity-classification to shed LUT/lookup tables.
alexnj Dec 16, 2022
9e29828
Merge remote-tracking branch 'origin/main' into entity-based-3p
alexnj Dec 16, 2022
c3efb66
Reverse all but entity-classification auditis change.
alexnj Dec 16, 2022
49bce13
third-party-facades depend on third-party-summary so it needs ec.
alexnj Dec 16, 2022
388e557
Bring branch in sync and tests passing.
alexnj Dec 16, 2022
42cb558
Now that we need to lookup on the report, bring back LUTs.
alexnj Dec 17, 2022
932b029
Compute entities during reporting, first pass.
alexnj Dec 17, 2022
8ebe3d9
Sync review changes from entity-based-3p branch: 2022-12-21 11:04
alexnj Dec 21, 2022
4c9d8be
Move entity-classification from being an audit to global.
alexnj Jan 10, 2023
b57fb8e
Final review change sync from entity-based-3p at 2023-01-09 20:47
alexnj Jan 10, 2023
730705a
Merge branch 'main' into entity-based-3p-reportonly
alexnj Jan 10, 2023
673162a
Move item.entity resolution to prepareReportResult
alexnj Jan 10, 2023
d3a68c7
Cant have esm fast enough
alexnj Jan 10, 2023
38bf2f7
Update tests.
alexnj Jan 10, 2023
6cba854
Protobuf changes to support entityClassification.
alexnj Jan 10, 2023
a839ecc
Some cleanup and comments.
alexnj Jan 10, 2023
0224365
Make valid-source-maps audit depend on EntityClassification.
alexnj Jan 10, 2023
5304284
Drop entity classification from valid source maps audit
alexnj Jan 10, 2023
e328f66
Add a test case to prevent unrecognized 3ps becoming 1ps
alexnj Jan 10, 2023
c31aca9
legacy-javsacript needs to depend on ComputedEntityClassification
alexnj Jan 10, 2023
f54852d
uses-http2 to depend on entity-classification
alexnj Jan 10, 2023
2934c84
Depend on entity classification adjust condition for correctness
alexnj Jan 10, 2023
737fdd0
Less hurting to the eyes.
alexnj Jan 10, 2023
a3ca3e0
Revert uses-http2 logic change.
alexnj Jan 10, 2023
608fe9e
Special case handling: source-location type
alexnj Jan 11, 2023
aa0e54f
Use source-location instead of hardcoding names
alexnj Jan 11, 2023
f70b5d3
Fix up report-ui-features test
alexnj Jan 11, 2023
39c441d
missed the core/util
alexnj Jan 11, 2023
35ebe83
Update url protocol check
alexnj Jan 12, 2023
0b82a99
Merge branch 'main' into entity-based-3p-reportonly
alexnj Jan 12, 2023
dcabac8
Merge branch 'main' into entity-based-3p-reportonly
alexnj Jan 12, 2023
399a92a
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 12, 2023
145b9d5
Apply suggestions from code review
alexnj Jan 13, 2023
56d36ce
Review changes.
alexnj Jan 13, 2023
d11f3bb
Merge branch 'entity-based-3p-reportonly' of github.com:GoogleChrome/…
alexnj Jan 13, 2023
f532bf7
Update core/util
alexnj Jan 13, 2023
811f103
Minor comment change
alexnj Jan 13, 2023
7971218
Review changes, deal with audit instead of params
alexnj Jan 14, 2023
47553d0
copy util
alexnj Jan 14, 2023
ca82fe3
Exclusion for some audits
alexnj Jan 17, 2023
7568caf
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 17, 2023
831fa7c
Proto changes
alexnj Jan 17, 2023
1ef0970
Fix a bad merge
alexnj Jan 17, 2023
8b1a3a4
copy util
alexnj Jan 17, 2023
8a87ec2
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 17, 2023
ebfa00d
Unit test assertions for convenience fns.
alexnj Jan 17, 2023
dba8d79
exclude core/util.js from code coverage
alexnj Jan 18, 2023
1fb6ad7
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 18, 2023
45b63b4
Apply suggestions from code review
alexnj Jan 18, 2023
a1a293d
Add back the correct type for Entity.
alexnj Jan 19, 2023
c08fb30
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 19, 2023
157e3b5
always missing core/util
alexnj Jan 19, 2023
c6876d6
Rename lookup tables to LH naming convention
alexnj Jan 19, 2023
8d97490
Apply suggestions from code review
alexnj Jan 19, 2023
c142ed6
add back legacy filter tests.
alexnj Jan 19, 2023
ded03b6
Review changes: variable names
alexnj Jan 19, 2023
3b56d0b
who likes commented tests?
alexnj Jan 19, 2023
656287c
reverse patch all report changes.
alexnj Jan 19, 2023
0a0136f
it's me vs. util.js time again
alexnj Jan 19, 2023
118f97d
review changes
alexnj Jan 20, 2023
250d4f6
Merge branch 'main' into entity-based-3p-reportonly
alexnj Jan 20, 2023
9c80a3e
Relax checks on firstparty
alexnj Jan 20, 2023
aec61da
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 20, 2023
11ec2b1
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 23, 2023
e8760bb
Collect all report related changes from #14622
alexnj Jan 19, 2023
eeb341a
me vs. utiljs: utiljs:1, alex:0
alexnj Jan 19, 2023
53d77a3
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 25, 2023
a42dd62
Cleanup after merge
alexnj Jan 25, 2023
edbff41
util js is still leading!, alex: 0, coreutil: 2
alexnj Jan 25, 2023
2d3c251
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 25, 2023
c858df8
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 25, 2023
31c41ae
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 25, 2023
5e363d0
Apply suggestions from code review
alexnj Jan 25, 2023
dd8771c
Review changes.
alexnj Jan 25, 2023
2cc6b44
Update/cleanup comments
alexnj Jan 25, 2023
6f392b8
Review changes + Refactor of third-party filter + tests
alexnj Jan 26, 2023
2466ec6
delete core/util.cjs
alexnj Jan 26, 2023
8a6c2cc
Missed entity classification
alexnj Jan 26, 2023
c2625a4
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 26, 2023
f3a16d3
Merge remote-tracking branch 'origin/main' into entity-based-3p-repor…
alexnj Jan 27, 2023
41aa773
Update report/renderer/report-ui-features.js
alexnj Jan 27, 2023
e850f7e
attempt bugfix: unclassified row items aren't 3ps
alexnj Jan 27, 2023
1265edb
add testcase: non-classifable urls belong in 1st party.
alexnj Jan 27, 2023
c69cb3d
Update report/renderer/report-ui-features.js
alexnj Jan 27, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions report/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ export class DetailsRenderer {
return null;

default: {
// @ts-expect-error tsc thinks this is unreachable, but be forward compatible
// with new unexpected detail types.
// @ts-expect-error - all detail types need to be handled above so tsc thinks this is unreachable.
// Call _renderUnknown() to be forward compatible with new, unexpected detail types.
return this._renderUnknown(details.type, details);
}
}
Expand Down Expand Up @@ -398,9 +398,22 @@ export class DetailsRenderer {
let even = true;
for (const item of details.items) {
const rowsFragment = this._renderTableRowsFromItem(item, details.headings);

// The attribute item.entity could be a string (entity-classification), or
// a LinkValue for ThirdPartySummary audit.
let entityName;
if (typeof item.entity === 'object' && item.entity.type === 'link') {
entityName = item.entity.text;
} else if (typeof item.entity === 'string') {
entityName = item.entity;
}

for (const rowEl of this._dom.findAll('tr', rowsFragment)) {
// For zebra styling.
rowEl.classList.add(even ? 'lh-row--even' : 'lh-row--odd');
if (entityName && !rowEl.classList.contains('lh-sub-item-row')) {
rowEl.dataset.entity = entityName;
}
}
even = !even;
tbodyElem.append(rowsFragment);
Expand Down
44 changes: 25 additions & 19 deletions report/renderer/report-ui-features.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ export class ReportUIFeatures {

tablesWithUrls.forEach((tableEl) => {
const rowEls = getTableRows(tableEl);
const thirdPartyRows = this._getThirdPartyRows(rowEls, Util.getFinalDisplayedUrl(this.json));
const primaryRowEls = rowEls.filter(rowEl => !rowEl.classList.contains('lh-sub-item-row'));
const thirdPartyRowEls = this._getThirdPartyRows(
primaryRowEls, Util.getFinalDisplayedUrl(this.json));

// create input box
const filterTemplate = this._dom.createComponent('3pFilter');
Expand All @@ -253,9 +255,9 @@ export class ReportUIFeatures {
filterInput.addEventListener('change', e => {
const shouldHideThirdParty = e.target instanceof HTMLInputElement && !e.target.checked;
let even = true;
let rowEl = rowEls[0];
let rowEl = primaryRowEls[0];
while (rowEl) {
const shouldHide = shouldHideThirdParty && thirdPartyRows.includes(rowEl);
const shouldHide = shouldHideThirdParty && thirdPartyRowEls.includes(rowEl);

// Iterate subsequent associated sub item rows.
do {
Expand All @@ -272,12 +274,12 @@ export class ReportUIFeatures {
});

this._dom.find('.lh-3p-filter-count', filterTemplate).textContent =
`${thirdPartyRows.length}`;
`${thirdPartyRowEls.length}`;
this._dom.find('.lh-3p-ui-string', filterTemplate).textContent =
Globals.strings.thirdPartyResourcesLabel;

const allThirdParty = thirdPartyRows.length === rowEls.length;
const allFirstParty = !thirdPartyRows.length;
const allThirdParty = thirdPartyRowEls.length === primaryRowEls.length;
const allFirstParty = !thirdPartyRowEls.length;

// If all or none of the rows are 3rd party, hide the control.
if (allThirdParty || allFirstParty) {
Expand Down Expand Up @@ -319,25 +321,29 @@ export class ReportUIFeatures {
* @return {Array<HTMLElement>}
*/
_getThirdPartyRows(rowEls, finalDisplayedUrl) {
/** @type {Array<HTMLElement>} */
const thirdPartyRows = [];
const finalDisplayedUrlRootDomain = Util.getRootDomain(finalDisplayedUrl);
const firstPartyEntityName = this.json.entities?.firstParty;

/** @type {Array<HTMLElement>} */
const thirdPartyRowEls = [];
for (const rowEl of rowEls) {
if (rowEl.classList.contains('lh-sub-item-row')) continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was removing this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've introduced a regression in third-party entity count shown on the filter, with that line removed. We don't have a unit test to check if the count displayed is correct, so I'm going to add one for that as well.

Longer version:

The legacy filter checkbox visibility works by matching rows that have a lh-text__url and a dataset.url attached to it. The filter checkbox is hidden if count(such rows) === count(all table rows) in which case all rows are third-parties, or if count(such rows) equals to zero, in which case it assumes all rows are 1st parties.

For an audit like third-party-summary, this still works when the third-party count mismatches and fails the all-third-party check. The 1st party check (incorrectly) succeeds. I think we can rewrite this section with both comparisons basing on primary rows only.

Copy link
Member Author

@alexnj alexnj Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the code; I think it should be good for a re-review. We're now passing only first-level/primary rows into this function.


const urlItem = rowEl.querySelector('div.lh-text__url');
if (!urlItem) continue;

const datasetUrl = urlItem.dataset.url;
if (!datasetUrl) continue;
const isThirdParty = Util.getRootDomain(datasetUrl) !== finalDisplayedUrlRootDomain;
if (!isThirdParty) continue;
if (firstPartyEntityName) {
// We rely on entity-classification for new LHRs that support it.
if (!rowEl.dataset.entity || rowEl.dataset.entity === firstPartyEntityName) continue;
} else {
// Without 10.0's entity classification, fallback to the older root domain-based filtering.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was already discussed, but maybe after a certain amount of time (circa 11.0?) we should drop this code? We want to render old LHRs, but I'm not sure 3p filtering of old LHRs is that important after a while

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't discussed that. Should we have a tracker issue labeled 11.0?

const urlItem = rowEl.querySelector('div.lh-text__url');
if (!urlItem) continue;
const datasetUrl = urlItem.dataset.url;
if (!datasetUrl) continue;
const isThirdParty = Util.getRootDomain(datasetUrl) !== finalDisplayedUrlRootDomain;
if (!isThirdParty) continue;
}

thirdPartyRows.push(rowEl);
thirdPartyRowEls.push(rowEl);
}

return thirdPartyRows;
return thirdPartyRowEls;
}

/**
Expand Down
72 changes: 72 additions & 0 deletions report/renderer/report-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ class ReportUtils {
}
}

// Attach table/opportunity items with entity information.
ReportUtils.classifyEntities(result.entities, audit);

// TODO: convert printf-style displayValue.
// Added: #5099, v3
// Removed: #6767, v4
Expand Down Expand Up @@ -187,6 +190,75 @@ class ReportUtils {
return clone;
}

/**
* Given an audit's details, identify and return a URL locator function that
* can be called later with an `item` to extract the URL of it.
* @param {LH.FormattedIcu<LH.Audit.Details.TableColumnHeading[]>} headings
* @return {{(item: LH.FormattedIcu<LH.Audit.Details.TableItem>): string|undefined}=}
alexnj marked this conversation as resolved.
Show resolved Hide resolved
*/
static getUrlLocatorFn(headings) {
// The most common type, valueType=url.
const urlKey = headings.find(heading => heading.valueType === 'url')?.key;
if (urlKey && typeof urlKey === 'string') {
// Return a function that extracts item.url.
return (item) => {
const url = item[urlKey];
if (typeof url === 'string') return url;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically table items can override their valueType themselves, so this would just opt them out even if they could still be classified from item[urlKey].value. Maybe a corner case worth dealing with after #14655, though (not sure if any audit actually does this with url)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I've seen in any of our audits. Why would we allow that, vs. introducing a new valueType?

};
}

// The second common type, valueType=source-location.
const srcLocationKey = headings.find(heading => heading.valueType === 'source-location')?.key;
if (srcLocationKey) {
// Return a function that extracts item.source.url.
return (item) => {
const sourceLocation = item[srcLocationKey];
if (typeof sourceLocation === 'object' && sourceLocation.type === 'source-location') {
return sourceLocation.url;
}
};
}

// More specific tests go here, as we need to identify URLs in more audits.
}

/**
* Mark TableItems/OpportunityItems with entity names.
* @param {LH.Result.Entities|undefined} entityClassification
* @param {import('../../types/lhr/audit-result').Result} audit
*/
static classifyEntities(entityClassification, audit) {
if (!entityClassification) return;
if (audit.details?.type !== 'opportunity' && audit.details?.type !== 'table') {
Comment on lines +231 to +232
Copy link
Member

@brendankenny brendankenny Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent might be clearer if these checks are moved out into prepareReportResult, so as you're reading that method you know you only need to step into classifyEntities if it's a table or opportunity (right now you have to trust the comment isn't out of date).

In here, the params could then be narrowed, dropping the undefined on entityClassification, and audit could become details of LH.Audit.Details.Opportunity|LH.Audit.Details.Table

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are to keep TS happy in the rest of the function, as we're passing the full audit here. Is there a better way to do it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean something like

diff --git a/report/renderer/report-utils.js b/report/renderer/report-utils.js
index a38b52d2b..739cc6210 100644
--- a/report/renderer/report-utils.js
+++ b/report/renderer/report-utils.js
@@ -91,7 +91,11 @@ class ReportUtils {
         }
 
         // Attach table/opportunity items with entity information.
-        ReportUtils.classifyEntities(result.entities, audit);
+        if (result.entities && audit.details) {
+          if (audit.details.type === 'opportunity' || audit.details.type === 'table') {
+            ReportUtils.classifyEntities(result.entities, audit.details);
+          }
+        }
 
         // TODO: convert printf-style displayValue.
         // Added:   #5099, v3
@@ -224,17 +228,12 @@ class ReportUtils {
 
   /**
    * Mark TableItems/OpportunityItems with entity names.
-   * @param {LH.Result.Entities|undefined} entityClassification
-   * @param {import('../../types/lhr/audit-result').Result} audit
+   * @param {LH.Result.Entities} entityClassification
+   * @param {LH.FormattedIcu<LH.Audit.Details.Opportunity|LH.Audit.Details.Table>} details
    */
-  static classifyEntities(entityClassification, audit) {
-    if (!entityClassification) return;
-    if (audit.details?.type !== 'opportunity' && audit.details?.type !== 'table') {
-      return;
-    }
-
+  static classifyEntities(entityClassification, details) {
     // If details.items are already marked with entity attribute during an audit, nothing to do here.
-    const {items, headings} = audit.details;
+    const {items, headings} = details;
     if (!items.length || items.some(item => item.entity)) return;
 
     // Identify a URL-locator function that we could call against each item to get its URL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was passing audit.details or more specific params earlier, I think. But I now think it would be beneficial to pass the full audit to get the id as well, in case if any audit needs special handling in future.

return;
}

// If details.items are already marked with entity attribute during an audit, nothing to do here.
const {items, headings} = audit.details;
if (!items.length || items.some(item => item.entity)) return;

// Identify a URL-locator function that we could call against each item to get its URL.
const urlLocatorFn = ReportUtils.getUrlLocatorFn(headings);
if (!urlLocatorFn) return;

for (const item of items) {
const url = urlLocatorFn(item);
if (!url) continue;

let origin;
try {
// Non-URLs can appear in valueType: url columns, like 'Unattributable'
origin = Util.parseURL(url).origin;
} catch {}
if (!origin) continue;

const entityIndex = entityClassification.entityIndexByOrigin[origin];
if (entityIndex === undefined) return;
const entity = entityClassification.list[entityIndex];
item.entity = entity.name;
}
}

/**
* @param {LH.Result['configSettings']} settings
* @return {!{deviceEmulation: string, screenEmulation?: string, networkThrottling: string, cpuThrottling: string, summary: string}}
Expand Down
Loading