[RPP] Show font name in FontDisplay insight

https://i.imgur.com/2xuPgZO.png

Fixed: 369422196
Change-Id: If89a426a427e9521bd1c12ebd88b0b3899b02c7a
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6313665
Commit-Queue: Connor Clark <[email protected]>
Reviewed-by: Paul Irish <[email protected]>
Commit-Queue: Paul Irish <[email protected]>
Auto-Submit: Connor Clark <[email protected]>

Author: Connor Clark

front_end/models/trace/handlers/LayoutShiftsHandler.ts

@@ -53,12 +53,19 @@ interface LayoutShifts {
   renderFrameImplCreateChildFrameEvents: readonly Types.Events.RenderFrameImplCreateChildFrame[];
   domLoadingEvents: readonly Types.Events.DomLoading[];
   layoutImageUnsizedEvents: readonly Types.Events.LayoutImageUnsized[];
-  beginRemoteFontLoadEvents: readonly Types.Events.BeginRemoteFontLoad[];
+  remoteFonts: readonly RemoteFont[];
   scoreRecords: readonly ScoreRecord[];
   // TODO(crbug/41484172): should be readonly
   backendNodeIds: Protocol.DOM.BackendNodeId[];
 }
 
+interface RemoteFont {
+  display: string;
+  url?: string;
+  name?: string;
+  beginRemoteFontLoadEvent: Types.Events.BeginRemoteFontLoad;
+}
+
 // This represents the maximum #time we will allow a cluster to go before we
 // reset it.
 export const MAX_CLUSTER_DURATION = Helpers.Timing.milliToMicro(Types.Timing.Milli(5000));
@@ -84,7 +91,7 @@ const styleRecalcInvalidationEvents: Types.Events.StyleRecalcInvalidationTrackin
 const renderFrameImplCreateChildFrameEvents: Types.Events.RenderFrameImplCreateChildFrame[] = [];
 const domLoadingEvents: Types.Events.DomLoading[] = [];
 const layoutImageUnsizedEvents: Types.Events.LayoutImageUnsized[] = [];
-const beginRemoteFontLoadEvents: Types.Events.BeginRemoteFontLoad[] = [];
+const remoteFonts: RemoteFont[] = [];
 
 const backendNodeIds = new Set<Protocol.DOM.BackendNodeId>();
 
@@ -124,7 +131,7 @@ export function reset(): void {
   renderFrameImplCreateChildFrameEvents.length = 0;
   layoutImageUnsizedEvents.length = 0;
   domLoadingEvents.length = 0;
-  beginRemoteFontLoadEvents.length = 0;
+  remoteFonts.length = 0;
   backendNodeIds.clear();
   clusters.length = 0;
   sessionMaxScore = 0;
@@ -162,7 +169,18 @@ export function handleEvent(event: Types.Events.Event): void {
     layoutImageUnsizedEvents.push(event);
   }
   if (Types.Events.isBeginRemoteFontLoad(event)) {
-    beginRemoteFontLoadEvents.push(event);
+    remoteFonts.push({
+      display: event.args.display,
+      url: event.args.url,
+      beginRemoteFontLoadEvent: event,
+    });
+  }
+  if (Types.Events.isRemoteFontLoaded(event)) {
+    for (const remoteFont of remoteFonts) {
+      if (remoteFont.url === event.args.url) {
+        remoteFont.name = event.args.name;
+      }
+    }
   }
   if (Types.Events.isPaintImage(event)) {
     paintImageEvents.push(event);
@@ -259,7 +277,7 @@ export async function finalize(): Promise<void> {
   renderFrameImplCreateChildFrameEvents.sort((a, b) => a.ts - b.ts);
   domLoadingEvents.sort((a, b) => a.ts - b.ts);
   layoutImageUnsizedEvents.sort((a, b) => a.ts - b.ts);
-  beginRemoteFontLoadEvents.sort((a, b) => a.ts - b.ts);
+  remoteFonts.sort((a, b) => a.beginRemoteFontLoadEvent.ts - b.beginRemoteFontLoadEvent.ts);
   paintImageEvents.sort((a, b) => a.ts - b.ts);
 
   // Each function transforms the data used by the next, as such the invoke order
@@ -514,7 +532,7 @@ export function data(): LayoutShifts {
     renderFrameImplCreateChildFrameEvents,
     domLoadingEvents,
     layoutImageUnsizedEvents,
-    beginRemoteFontLoadEvents,
+    remoteFonts,
     scoreRecords,
     // TODO(crbug/41484172): change the type so no need to clone
     backendNodeIds: [...backendNodeIds],

front_end/models/trace/insights/FontDisplay.test.ts

@@ -4,6 +4,7 @@
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
 import {getFirstOrError, getInsightOrError, processTrace} from '../../../testing/InsightHelpers.js';
+import type * as Types from '../types/types.js';
 
 describeWithEnvironment('FontDisplay', function() {
   it('finds no requests for remote fonts', async () => {
@@ -23,34 +24,40 @@ describeWithEnvironment('FontDisplay', function() {
 
     assert.deepEqual(insight.fonts.map(f => ({...f, request: f.request.args.data.url})), [
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/ptsans/v17/jizaRExUiTo99u79D0KExcOPIDU.woff2',
         display: 'auto',
-        wastedTime: 20,
+        wastedTime: 20 as Types.Timing.Milli,
       },
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/droidsans/v18/SlGVmQWMvZQIdix7AFxXkHNSbRYXags.woff2',
         display: 'auto',
-        wastedTime: 15,
+        wastedTime: 15 as Types.Timing.Milli,
       },
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/ptsans/v17/jizfRExUiTo99u79B_mh0O6tLR8a8zI.woff2',
         display: 'auto',
-        wastedTime: 15,
+        wastedTime: 15 as Types.Timing.Milli,
       },
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/droidsans/v18/SlGWmQWMvZQIdix7AFxXmMh3eDs1ZyHKpWg.woff2',
         display: 'auto',
-        wastedTime: 15,
+        wastedTime: 15 as Types.Timing.Milli,
       },
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/ptserif/v18/EJRVQgYoZZY2vCFuvAFWzr-_dSb_.woff2',
         display: 'auto',
-        wastedTime: 15,
+        wastedTime: 15 as Types.Timing.Milli,
       },
       {
+        name: undefined,
         request: 'https://fonts.gstatic.com/s/lato/v24/S6u9w4BMUTPHh6UVSwiPGQ3q5d0.woff2',
         display: 'auto',
-        wastedTime: 10,
+        wastedTime: 10 as Types.Timing.Milli,
       },
     ]);
   });

front_end/models/trace/insights/FontDisplay.ts

@@ -37,12 +37,15 @@ export function deps(): ['Meta', 'NetworkRequests', 'LayoutShifts'] {
   return ['Meta', 'NetworkRequests', 'LayoutShifts'];
 }
 
+interface RemoteFont {
+  name?: string;
+  request: Types.Events.SyntheticNetworkRequest;
+  display: string;
+  wastedTime: Types.Timing.Milli;
+}
+
 export type FontDisplayInsightModel = InsightModel<typeof UIStrings, {
-  fonts: Array<{
-    request: Types.Events.SyntheticNetworkRequest,
-    display: string,
-    wastedTime: Types.Timing.Milli,
-  }>,
+  fonts: RemoteFont[],
 }>;
 
 function finalize(partialModel: PartialInsightModel<FontDisplayInsightModel>): FontDisplayInsightModel {
@@ -59,8 +62,9 @@ function finalize(partialModel: PartialInsightModel<FontDisplayInsightModel>): F
 
 export function generateInsight(
     parsedTrace: RequiredData<typeof deps>, context: InsightSetContext): FontDisplayInsightModel {
-  const fonts = [];
-  for (const event of parsedTrace.LayoutShifts.beginRemoteFontLoadEvents) {
+  const fonts: RemoteFont[] = [];
+  for (const remoteFont of parsedTrace.LayoutShifts.remoteFonts) {
+    const event = remoteFont.beginRemoteFontLoadEvent;
     if (!Helpers.Timing.eventIsInBounds(event, context.bounds)) {
       continue;
     }
@@ -71,10 +75,9 @@ export function generateInsight(
       continue;
     }
 
-    const display = event.args.display;
     let wastedTime = Types.Timing.Milli(0);
 
-    if (/^(block|fallback|auto)$/.test(display)) {
+    if (/^(block|fallback|auto)$/.test(remoteFont.display)) {
       const wastedTimeMicro = Types.Timing.Micro(
           request.args.data.syntheticData.finishTime - request.args.data.syntheticData.sendStartTime);
       // TODO(crbug.com/352244504): should really end at the time of the next Commit trace event.
@@ -85,8 +88,9 @@ export function generateInsight(
     }
 
     fonts.push({
+      name: remoteFont.name,
       request,
-      display,
+      display: remoteFont.display,
       wastedTime,
     });
   }

front_end/models/trace/types/TraceEvents.ts

@@ -1377,6 +1377,15 @@ export interface BeginRemoteFontLoad extends UserTiming {
   args: Args&{
     display: string,
     id: number,
+    url?: string,
+  };
+}
+
+export interface RemoteFontLoaded extends UserTiming {
+  name: Name.REMOTE_FONT_LOADED;
+  args: Args&{
+    url: string,
+    name: string,
   };
 }
 
@@ -2278,6 +2287,10 @@ export function isBeginRemoteFontLoad(event: Event): event is BeginRemoteFontLoa
   return event.name === Name.BEGIN_REMOTE_FONT_LOAD;
 }
 
+export function isRemoteFontLoaded(event: Event): event is RemoteFontLoaded {
+  return event.name === Name.REMOTE_FONT_LOADED;
+}
+
 export function isPerformanceMeasure(event: Event): event is PerformanceMeasure {
   return isUserTiming(event) && isPhaseAsync(event.ph);
 }
@@ -3012,6 +3025,7 @@ export const enum Name {
 
   DOM_LOADING = 'domLoading',
   BEGIN_REMOTE_FONT_LOAD = 'BeginRemoteFontLoad',
+  REMOTE_FONT_LOADED = 'RemoteFontLoaded',
 
   ANIMATION_FRAME = 'AnimationFrame',
   ANIMATION_FRAME_PRESENTATION = 'AnimationFrame::Presentation',

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

@@ -66,11 +66,13 @@ class EventRef extends HTMLElement {
 
 type EventRefSupportedEvents = Trace.Types.Events.SyntheticNetworkRequest;
 
-export function eventRef(event: EventRefSupportedEvents): Lit.TemplateResult {
-  let title, text;
+export function eventRef(
+    event: EventRefSupportedEvents, options?: {text?: string, title?: string}): Lit.TemplateResult {
+  let title = options?.title;
+  let text = options?.text;
   if (Trace.Types.Events.isSyntheticNetworkRequest(event)) {
-    text = Utils.Helpers.shortenUrl(new URL(event.args.data.url));
-    title = event.args.data.url;
+    text = text ?? Utils.Helpers.shortenUrl(new URL(event.args.data.url));
+    title = title ?? event.args.data.url;
   } else {
     Platform.TypeScriptUtilities.assertNever(
         event, `unsupported event in eventRef: ${(event as Trace.Types.Events.Event).name}`);

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

@@ -60,8 +60,7 @@ export class FontDisplay extends BaseInsightComponent<FontDisplayInsightModel> {
             headers: [i18nString(UIStrings.fontColumn), i18nString(UIStrings.wastedTimeColumn)],
             rows: this.model.fonts.map(font => ({
               values: [
-                // TODO(crbug.com/369422196): the font name would be nicer here.
-                eventRef(font.request),
+                eventRef(font.request, {text: font.name}),
                 i18n.TimeUtilities.millisToString(font.wastedTime),
               ],
               overlays: [this.#overlayForRequest.get(font.request)],

front_end/testing/TraceHelpers.ts

@@ -612,7 +612,7 @@ export function getBaseTraceParseModelData(overrides: Partial<ParsedTrace> = {})
       renderFrameImplCreateChildFrameEvents: [],
       domLoadingEvents: [],
       layoutImageUnsizedEvents: [],
-      beginRemoteFontLoadEvents: [],
+      remoteFonts: [],
       scoreRecords: [],
       backendNodeIds: [],
       paintImageEvents: [],