RPP: update LargestImagePaintHandler to store by navigation ID

It is much easier if we store the data keyed by navigation ID rather
than the entire object; in particular this is useful for insights that
have the ID attached but not always the entire navigation.

Bug: none
Change-Id: Id63c5c3413897bfbdd290c7a2b5bd1995e0420a9
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6343230
Commit-Queue: Nancy Li <[email protected]>
Reviewed-by: Nancy Li <[email protected]>
Auto-Submit: Jack Franklin <[email protected]>
Commit-Queue: Jack Franklin <[email protected]>

Author: jackfranklin

front_end/models/trace/handlers/LargestImagePaintHandler.test.ts

@@ -18,10 +18,10 @@ describeWithEnvironment('LargestImagePaintHandler', function() {
     // There is only one main frame navigation in this trace.
     assert.lengthOf(mainFrameNavigations, 1);
     const mainNavigation = mainFrameNavigations.at(0);
-    assert.isOk(mainNavigation);
+    assert.isOk(mainNavigation?.args.data?.navigationId);
 
-    const {lcpRequestByNavigation} = parsedTrace.LargestImagePaint;
-    const lcpRequest = lcpRequestByNavigation.get(mainNavigation);
+    const {lcpRequestByNavigationId} = parsedTrace.LargestImagePaint;
+    const lcpRequest = lcpRequestByNavigationId.get(mainNavigation.args.data?.navigationId);
     assert.isOk(lcpRequest);
     assert.strictEqual(lcpRequest.args.data.url, 'https://via.placeholder.com/2000.jpg');
   });
@@ -35,10 +35,10 @@ describeWithEnvironment('LargestImagePaintHandler', function() {
     // There is only one main frame navigation in this trace.
     assert.lengthOf(mainFrameNavigations, 1);
     const mainNavigation = mainFrameNavigations.at(0);
-    assert.isOk(mainNavigation);
+    assert.isOk(mainNavigation?.args.data?.navigationId);
 
-    const {lcpRequestByNavigation} = parsedTrace.LargestImagePaint;
-    const lcpRequest = lcpRequestByNavigation.get(mainNavigation);
+    const {lcpRequestByNavigationId} = parsedTrace.LargestImagePaint;
+    const lcpRequest = lcpRequestByNavigationId.get(mainNavigation.args.data.navigationId);
     assert.isOk(lcpRequest);
     assert.strictEqual(lcpRequest.args.data.url, 'https://placehold.co/1000.jpg');
   });
@@ -50,10 +50,10 @@ describeWithEnvironment('LargestImagePaintHandler', function() {
     // There is only one main frame navigation in this trace.
     assert.lengthOf(mainFrameNavigations, 1);
     const mainNavigation = mainFrameNavigations.at(0);
-    assert.isOk(mainNavigation);
+    assert.isOk(mainNavigation?.args.data?.navigationId);
 
-    const {lcpRequestByNavigation} = parsedTrace.LargestImagePaint;
-    const lcpRequest = lcpRequestByNavigation.get(mainNavigation);
+    const {lcpRequestByNavigationId} = parsedTrace.LargestImagePaint;
+    const lcpRequest = lcpRequestByNavigationId.get(mainNavigation.args.data.navigationId);
 
     // There is a largest image paint event, but it happens after the LCP candidate
     // so it's ignored. The actual LCP is text.

front_end/models/trace/handlers/LargestImagePaintHandler.ts

@@ -32,7 +32,7 @@ import type {HandlerName} from './types.js';
  **/
 const imagePaintsByNodeIdAndProcess =
     new Map<Types.Events.ProcessID, Map<Protocol.DOM.BackendNodeId, Types.Events.LargestImagePaintCandidate>>();
-const lcpRequestByNavigation = new Map<Types.Events.NavigationStart|null, Types.Events.SyntheticNetworkRequest>();
+const lcpRequestByNavigationId = new Map<string, Types.Events.SyntheticNetworkRequest>();
 
 export function reset(): void {
   imagePaintsByNodeIdAndProcess.clear();
@@ -90,17 +90,17 @@ export async function finalize(): Promise<void> {
     }
 
     if (lcpRequest) {
-      lcpRequestByNavigation.set(navigation, lcpRequest);
+      lcpRequestByNavigationId.set(navigationId, lcpRequest);
     }
   }
 }
 
 export interface LargestImagePaintData {
-  lcpRequestByNavigation: Map<Types.Events.NavigationStart|null, Types.Events.SyntheticNetworkRequest>;
+  lcpRequestByNavigationId: Map<string, Types.Events.SyntheticNetworkRequest>;
 }
 
 export function data(): LargestImagePaintData {
-  return {lcpRequestByNavigation};
+  return {lcpRequestByNavigationId};
 }
 
 export function deps(): HandlerName[] {

front_end/models/trace/insights/LCPDiscovery.ts

@@ -121,7 +121,7 @@ export function generateInsight(
     return finalize({warnings: [InsightWarning.NO_DOCUMENT_REQUEST]});
   }
 
-  const lcpRequest = parsedTrace.LargestImagePaint.lcpRequestByNavigation.get(context.navigation);
+  const lcpRequest = parsedTrace.LargestImagePaint.lcpRequestByNavigationId.get(context.navigationId);
   if (!lcpRequest) {
     return finalize({lcpEvent});
   }

front_end/models/trace/insights/LCPPhases.ts

@@ -195,7 +195,7 @@ export function generateInsight(
   const lcpMs = Helpers.Timing.microToMilli(metricScore.timing);
   // This helps position things on the timeline's UI accurately for a trace.
   const lcpTs = metricScore.event?.ts ? Helpers.Timing.microToMilli(metricScore.event?.ts) : undefined;
-  const lcpRequest = parsedTrace.LargestImagePaint.lcpRequestByNavigation.get(context.navigation);
+  const lcpRequest = parsedTrace.LargestImagePaint.lcpRequestByNavigationId.get(context.navigationId);
 
   const docRequest = networkRequests.byTime.find(req => req.args.data.requestId === context.navigationId);
   if (!docRequest) {

front_end/models/trace/insights/RenderBlocking.ts

@@ -114,7 +114,7 @@ function estimateSavingsWithGraphs(deferredIds: Set<string>, lanternContext: Lan
 }
 
 function hasImageLCP(parsedTrace: Handlers.Types.ParsedTrace, context: InsightSetContextWithNavigation): boolean {
-  return parsedTrace.LargestImagePaint.lcpRequestByNavigation.get(context.navigation) !== undefined;
+  return parsedTrace.LargestImagePaint.lcpRequestByNavigationId.has(context.navigationId);
 }
 
 function computeSavings(

front_end/panels/ai_assistance/data_formatters/PerformanceInsightFormatter.ts

@@ -24,20 +24,14 @@ function formatMicro(x: number|undefined): string {
 }
 
 /**
- * For a given frame ID and navigation, returns the LCP Event and the LCP Request, if the resource was an image.
+ * For a given frame ID and navigation ID, returns the LCP Event and the LCP Request, if the resource was an image.
  */
-function getLCPData(
-    parsedTrace: Trace.Handlers.Types.ParsedTrace, frameId: string, navigation: Trace.Types.Events.NavigationStart): {
+function getLCPData(parsedTrace: Trace.Handlers.Types.ParsedTrace, frameId: string, navigationId: string): {
   lcpEvent: Trace.Types.Events.LargestContentfulPaintCandidate,
   metricScore: Trace.Handlers.ModelHandlers.PageLoadMetrics.LCPMetricScore,
   lcpRequest?: Trace.Types.Events.SyntheticNetworkRequest,
 }|null {
-  if (!navigation.args.data?.navigationId) {
-    return null;
-  }
-
-  const navMetrics =
-      parsedTrace.PageLoadMetrics.metricScoresByFrameId.get(frameId)?.get(navigation.args.data?.navigationId);
+  const navMetrics = parsedTrace.PageLoadMetrics.metricScoresByFrameId.get(frameId)?.get(navigationId);
   if (!navMetrics) {
     return null;
   }
@@ -53,7 +47,7 @@ function getLCPData(
 
   return {
     lcpEvent,
-    lcpRequest: parsedTrace.LargestImagePaint.lcpRequestByNavigation.get(navigation),
+    lcpRequest: parsedTrace.LargestImagePaint.lcpRequestByNavigationId.get(navigationId),
     metricScore: metric,
   };
 }
@@ -74,15 +68,11 @@ export class PerformanceInsightFormatter {
       // No navigation ID = no LCP.
       return '';
     }
-    const navigation = this.#parsedTrace.Meta.navigationsByNavigationId.get(this.#insight.navigationId);
-    if (!navigation) {
-      return '';
-    }
-    if (!this.#insight.frameId) {
+    if (!this.#insight.frameId || !this.#insight.navigationId) {
       return '';
     }
 
-    const data = getLCPData(this.#parsedTrace, this.#insight.frameId, navigation);
+    const data = getLCPData(this.#parsedTrace, this.#insight.frameId, this.#insight.navigationId);
     if (!data) {
       return '';
     }

front_end/testing/TraceHelpers.ts

@@ -203,8 +203,8 @@ export function prettyPrint(
     indentation = 2, delimiter = ' ', prefix = '-', newline = '\n', out = ''): string {
   let skipped = false;
   return printNodes(tree.roots);
-  function printNodes(nodes: Set<Trace.Helpers.TreeHelpers.TraceEntryNode>|
-                      Trace.Helpers.TreeHelpers.TraceEntryNode[]): string {
+  function printNodes(nodes: Set<Trace.Helpers.TreeHelpers.TraceEntryNode>|Trace.Helpers.TreeHelpers.TraceEntryNode[]):
+      string {
     for (const node of nodes) {
       const event = node.entry;
       if (!predicate(node, event)) {
@@ -690,7 +690,7 @@ export function getBaseTraceParseModelData(overrides: Partial<ParsedTrace> = {})
       timestampEvents: [],
       measureTraceByTraceId: new Map(),
     },
-    LargestImagePaint: {lcpRequestByNavigation: new Map()},
+    LargestImagePaint: {lcpRequestByNavigationId: new Map()},
     LargestTextPaint: new Map(),
     AuctionWorklets: {
       worklets: new Map(),