[RPP] Support non-fresh traces in devtools-performance-node-link

This fixes a bug in the Viewport insight that was showing a random
DOM node for a non-fresh trace. It now shows fallback text when the
trace is not fresh.

Fixes a similar bug that likely existed with LayoutShiftDetails, since
the expected frame id is now checked before linkifying a given backend
node id. Though no support for non-fresh traces was added there, as it
requires more work.

Bug: 371620361
Change-Id: Ia3e55b534b31d0ed9e78642c6e88b54c5b113f4d
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6152659
Auto-Submit: Connor Clark <cjamcl@chromium.org>
Commit-Queue: Adriana Ixba <aixba@chromium.org>
Reviewed-by: Adriana Ixba <aixba@chromium.org>
Commit-Queue: Connor Clark <cjamcl@chromium.org>

Author: Connor Clark

front_end/panels/timeline/components/LayoutShiftDetails.ts

@@ -120,13 +120,20 @@ export class LayoutShiftDetails extends HTMLElement {
     `;
   }
 
-  #renderShiftedElements(elementsShifted: Trace.Types.Events.TraceImpactedNode[]|undefined): LitHtml.LitTemplate {
+  #renderShiftedElements(
+      shift: Trace.Types.Events.SyntheticLayoutShift,
+      elementsShifted: Trace.Types.Events.TraceImpactedNode[]|undefined): LitHtml.LitTemplate {
     // clang-format off
     return html`
       ${elementsShifted?.map(el => {
         if (el.node_id !== undefined) {
           return html`
-            <devtools-performance-node-link .data=${{backendNodeId: el.node_id}}>
+            <devtools-performance-node-link
+              .data=${{
+                backendNodeId: el.node_id,
+                frame: shift.args.frame,
+                // TODO(crbug.com/371620361): if ever rendered for non-fresh traces, this needs to set a fallback text value.
+              } as Insights.NodeLink.NodeLinkData}>
             </devtools-performance-node-link>`;
         }
           return LitHtml.nothing;
@@ -188,26 +195,30 @@ export class LayoutShiftDetails extends HTMLElement {
     // clang-format on
   }
 
-  #renderUnsizedImage(node: Protocol.DOM.BackendNodeId): LitHtml.TemplateResult|null {
+  #renderUnsizedImage(frame: string, backendNodeId: Protocol.DOM.BackendNodeId): LitHtml.TemplateResult|null {
     // clang-format off
     const el = html`
       <devtools-performance-node-link
         .data=${{
-          backendNodeId: node,
+          backendNodeId,
+          frame,
+          // TODO(crbug.com/371620361): if ever rendered for non-fresh traces, this needs to set a fallback text value. This requires
+          // `rootCauses.unsizedImages` to have more than just the backend node id.
         } as Insights.NodeLink.NodeLinkData}>
       </devtools-performance-node-link>`;
     return html`
     <span class="culprit"><span class="culprit-type">${i18nString(UIStrings.unsizedImage)}: </span><span class="culprit-value">${el}</span></span>`;
     // clang-format on
   }
 
-  #renderRootCauseValues(rootCauses: Trace.Insights.Models.CLSCulprits.LayoutShiftRootCausesData|
-                         undefined): LitHtml.TemplateResult|null {
+  #renderRootCauseValues(
+      frame: string,
+      rootCauses: Trace.Insights.Models.CLSCulprits.LayoutShiftRootCausesData|undefined): LitHtml.TemplateResult|null {
     return html`
       ${rootCauses?.fontRequests.map(fontReq => this.#renderFontRequest(fontReq))}
       ${rootCauses?.iframeIds.map(iframe => this.#renderIframe(iframe))}
       ${rootCauses?.nonCompositedAnimations.map(failure => this.#renderAnimation(failure))}
-      ${rootCauses?.unsizedImages.map(image => this.#renderUnsizedImage(image))}
+      ${rootCauses?.unsizedImages.map(backendNodeId => this.#renderUnsizedImage(frame, backendNodeId))}
     `;
   }
 
@@ -237,6 +248,8 @@ export class LayoutShiftDetails extends HTMLElement {
         (rootCauses.fontRequests.length || rootCauses.iframeIds.length || rootCauses.nonCompositedAnimations.length ||
          rootCauses.unsizedImages.length));
 
+    // TODO(crbug.com/371620361): Needs to show something for non-fresh recordings.
+
     // clang-format off
     return html`
       <tr class="shift-row" data-ts=${shift.ts}>
@@ -245,12 +258,12 @@ export class LayoutShiftDetails extends HTMLElement {
         ${this.#isFreshRecording ? html`
           <td>
             <div class="elements-shifted">
-              ${this.#renderShiftedElements(elementsShifted)}
+              ${this.#renderShiftedElements(shift, elementsShifted)}
             </div>
           </td>` : LitHtml.nothing}
         ${hasCulprits && this.#isFreshRecording ? html`
           <td class="culprits">
-            ${this.#renderRootCauseValues(rootCauses)}
+            ${this.#renderRootCauseValues(shift.args.frame, rootCauses)}
           </td>` : LitHtml.nothing}
       </tr>`;
     // clang-format on

front_end/panels/timeline/components/insights/NodeLink.ts

@@ -5,57 +5,63 @@
 // TODO: move to ui/components/node_link?
 
 import * as Common from '../../../../core/common/common.js';
-import * as SDK from '../../../../core/sdk/sdk.js';
 import type * as Protocol from '../../../../generated/protocol.js';
+import * as Trace from '../../../../models/trace/trace.js';
 import * as ComponentHelpers from '../../../../ui/components/helpers/helpers.js';
 import * as LitHtml from '../../../../ui/lit-html/lit-html.js';
 
 const {html} = LitHtml;
 
 export interface NodeLinkData {
   backendNodeId: Protocol.DOM.BackendNodeId;
+  frame: string;
   options?: Common.Linkifier.Options;
+  /**
+   * Text to display if backendNodeId cannot be resolved (ie for traces loaded from disk).
+   * Displayed as monospace code. Use this or the next field.
+   */
+  fallbackHtmlSnippet?: string;
+  /**
+   * Text to display if backendNodeId cannot be resolved (ie for traces loaded from disk).
+   * Displayed as plain text. Use this or the previous field.
+   */
+  fallbackText?: string;
 }
 
 export class NodeLink extends HTMLElement {
-
   readonly #shadow = this.attachShadow({mode: 'open'});
   readonly #boundRender = this.#render.bind(this);
   #backendNodeId?: Protocol.DOM.BackendNodeId;
+  #frame?: string;
   #options?: Common.Linkifier.Options;
+  #fallbackHtmlSnippet?: string;
+  #fallbackText?: string;
 
   set data(data: NodeLinkData) {
     this.#backendNodeId = data.backendNodeId;
+    this.#frame = data.frame;
     this.#options = data.options;
+    this.#fallbackHtmlSnippet = data.fallbackHtmlSnippet;
+    this.#fallbackText = data.fallbackText;
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#boundRender);
   }
 
   async #linkify(): Promise<Node|undefined> {
-    // TODO: consider using `Trace.Extras.FetchNodes.extractRelatedDOMNodesFromEvent`, which
-    // requires parsedTrace.
-
     if (this.#backendNodeId === undefined) {
       return;
     }
 
-    const mainTarget = SDK.TargetManager.TargetManager.instance().primaryPageTarget();
-    if (!mainTarget) {
-      return;
-    }
-
-    const domModel = mainTarget.model(SDK.DOMModel.DOMModel);
-    if (!domModel) {
-      return;
-    }
-
-    const backendNodeIds = new Set([this.#backendNodeId]);
-    const domNodesMap = await domModel.pushNodesByBackendIdsToFrontend(backendNodeIds);
-    if (!domNodesMap) {
+    // Users of `NodeLink` do not have a parsed trace so this is a workaround. This
+    // is an abuse of this API, but it's currently alright since the first parameter
+    // is only used as a cache key.
+    const domNodesMap = await Trace.Extras.FetchNodes.domNodesForMultipleBackendNodeIds(
+        this as unknown as Trace.Handlers.Types.ParsedTrace, [this.#backendNodeId]);
+    const node = domNodesMap.get(this.#backendNodeId);
+    if (!node) {
       return;
     }
 
-    const node = domNodesMap.get(this.#backendNodeId);
-    if (!node) {
+    if (node.frameId() !== this.#frame) {
       return;
     }
 
@@ -66,11 +72,20 @@ export class NodeLink extends HTMLElement {
 
   async #render(): Promise<void> {
     const relatedNodeEl = await this.#linkify();
-    LitHtml.render(
-        html`<div class='node-link'>
-        ${relatedNodeEl}
-      </div>`,
-        this.#shadow, {host: this});
+
+    let template;
+    if (relatedNodeEl) {
+      template = html`<div class='node-link'>${relatedNodeEl}</div>`;
+    } else if (this.#fallbackHtmlSnippet) {
+      // TODO: Use CodeHighlighter.
+      template = html`<pre style='text-wrap: auto'>${this.#fallbackHtmlSnippet}</pre>`;
+    } else if (this.#fallbackText) {
+      template = html`<span>${this.#fallbackText}</span>`;
+    } else {
+      template = LitHtml.nothing;
+    }
+
+    LitHtml.render(template, this.#shadow, {host: this});
   }
 }
 

front_end/panels/timeline/components/insights/Viewport.ts

@@ -26,22 +26,27 @@ export class Viewport extends BaseInsightComponent<ViewportInsightModel> {
     return this.model?.metricSavings?.INP ?? null;
   }
 
-  override renderContent(): LitHtml.LitTemplate {
-    if (!this.model) {
+  renderContent(): LitHtml.LitTemplate {
+    if (!this.model || !this.model.viewportEvent) {
       return LitHtml.nothing;
     }
 
-    const backendNodeId = this.model.viewportEvent?.args.data.node_id;
+    const backendNodeId = this.model.viewportEvent.args.data.node_id;
+    if (backendNodeId === undefined) {
+      return LitHtml.nothing;
+    }
 
     // clang-format off
     return html`
       <div>
-        ${backendNodeId !== undefined ? html`<devtools-performance-node-link
+        <devtools-performance-node-link
           .data=${{
             backendNodeId,
-            options: {tooltip: this.model.viewportEvent?.args.data.content},
+            frame: this.model.viewportEvent.args.data.frame ?? '',
+            options: {tooltip: this.model.viewportEvent.args.data.content},
+            fallbackHtmlSnippet: `<meta name=viewport content="${this.model.viewportEvent.args.data.content}">`,
           }}>
-        </devtools-performance-node-link>` : LitHtml.nothing}
+        </devtools-performance-node-link>
       </div>`;
     // clang-format on
   }

front_end/ui/visual_logging/KnownContextValues.ts

@@ -3466,6 +3466,7 @@ export const knownContextValues = new Set([
   'timeline.insights-tab',
   'timeline.insights.cls-culprits',
   'timeline.insights.document-latency',
+  'timeline.insights.dom-size',
   'timeline.insights.font-display',
   'timeline.insights.image-delivery',
   'timeline.insights.inp',
@@ -3532,6 +3533,7 @@ export const knownContextValues = new Set([
   'timeline.timings',
   'timeline.toggle-insight.cls-culprits',
   'timeline.toggle-insight.document-latency',
+  'timeline.toggle-insight.dom-size',
   'timeline.toggle-insight.font-display',
   'timeline.toggle-insight.image-delivery',
   'timeline.toggle-insight.inp',