Skip to content

Commit

Permalink
[LayoutShift] Reduce usage of getNode
Browse files Browse the repository at this point in the history
Lighthouse issue:
GoogleChrome/lighthouse#15869

Bug: None
Change-Id: I72b2a0be71f7f45cb522fc710577fb78b38c7963
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5384002
Reviewed-by: Paul Irish <paulirish@chromium.org>
Reviewed-by: Connor Clark <cjamcl@chromium.org>
Commit-Queue: Paul Irish <paulirish@chromium.org>
  • Loading branch information
Adam Raine authored and Devtools-frontend LUCI CQ committed Mar 22, 2024
1 parent f3659d5 commit 9315458
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 17 deletions.
4 changes: 4 additions & 0 deletions front_end/models/trace/root-causes/LayoutShift.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,10 @@ describeWithMockConnection('LayoutShift root causes', () => {
...unknownLayoutInvalidation,
].sort((a, b) => a.ts - b.ts);

for (const e of layoutInvalidationEvents) {
e.name = TraceEngine.Types.TraceEvents.KnownEventName.LayoutInvalidationTracking;
}

// Map from fake BackendNodeId to fake Protocol.DOM.Node used by the handler to
// resolve the nodeIds in the traces.
const domNodeByBackendIdMapEntries: [Protocol.DOM.BackendNodeId, Protocol.DOM.Node|null][] = [];
Expand Down
42 changes: 26 additions & 16 deletions front_end/models/trace/root-causes/LayoutShift.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,12 @@ export class LayoutShiftRootCauses {
const fontChangeRootCause = this.getFontChangeRootCause(layoutInvalidation, nextPrePaint, modelData);
const renderBlockRootCause = this.getRenderBlockRootCause(layoutInvalidation, nextPrePaint, modelData);
const layoutInvalidationNodeId = nodeIdsByBackendIdMap.get(layoutInvalidation.args.data.nodeId);
const layoutInvalidationNode = layoutInvalidationNodeId !== undefined ?
await this.#protocolInterface.getNode(layoutInvalidationNodeId) :
null;
let unsizedMediaRootCause: UnsizedMedia|null = null;
let iframeRootCause: InjectedIframe|null = null;
if (layoutInvalidationNode && layoutInvalidation.args.data.reason) {
unsizedMediaRootCause =
await this.getUnsizedMediaRootCause(layoutInvalidation.args.data.reason, layoutInvalidationNode);
iframeRootCause = this.getIframeRootCause(layoutInvalidation.args.data.reason, layoutInvalidationNode);
if (layoutInvalidationNodeId !== undefined &&
Types.TraceEvents.isTraceEventLayoutInvalidationTracking(layoutInvalidation)) {
unsizedMediaRootCause = await this.getUnsizedMediaRootCause(layoutInvalidation, layoutInvalidationNodeId);
iframeRootCause = await this.getIframeRootCause(layoutInvalidation, layoutInvalidationNodeId);
}

if (!unsizedMediaRootCause && !iframeRootCause && !fontChangeRootCause && !renderBlockRootCause) {
Expand Down Expand Up @@ -292,12 +289,18 @@ export class LayoutShiftRootCauses {
* because a media element without dimensions was resized.
*/
async getUnsizedMediaRootCause(
reason: Types.TraceEvents.LayoutInvalidationReason,
layoutInvalidationNode: Protocol.DOM.Node): Promise<UnsizedMedia|null> {
layoutInvalidation: Types.TraceEvents.TraceEventLayoutInvalidationTracking,
layoutInvalidationNodeId: Protocol.DOM.NodeId): Promise<UnsizedMedia|null> {
// Filter events to resizes only.
if (reason !== Types.TraceEvents.LayoutInvalidationReason.SIZE_CHANGED) {
if (layoutInvalidation.args.data.reason !== Types.TraceEvents.LayoutInvalidationReason.SIZE_CHANGED) {
return null;
}

const layoutInvalidationNode = await this.#protocolInterface.getNode(layoutInvalidationNodeId);
if (!layoutInvalidationNode) {
return null;
}

const computedStylesList = await this.#protocolInterface.getComputedStyleForNode(layoutInvalidationNode.nodeId);
const computedStyles = new Map(computedStylesList.map(item => [item.name, item.value]));
if (computedStyles && !(await nodeIsUnfixedMedia(layoutInvalidationNode, computedStyles))) {
Expand All @@ -316,14 +319,21 @@ export class LayoutShiftRootCauses {
* Given a LayoutInvalidation trace event, determines if it was dispatched
* because a node, which is an ancestor to an iframe, was injected.
*/
getIframeRootCause(reason: Types.TraceEvents.LayoutInvalidationReason, layoutInvalidationDOMNode: Protocol.DOM.Node):
InjectedIframe|null {
if (layoutInvalidationDOMNode.nodeName !== 'IFRAME' &&
reason !== Types.TraceEvents.LayoutInvalidationReason.STYLE_CHANGED &&
reason !== Types.TraceEvents.LayoutInvalidationReason.ADDED_TO_LAYOUT) {
async getIframeRootCause(
layoutInvalidation: Types.TraceEvents.TraceEventLayoutInvalidationTracking,
layoutInvalidationNodeId: Protocol.DOM.NodeId): Promise<InjectedIframe|null> {
if (layoutInvalidation.args.data.nodeName?.startsWith('IFRAME') &&
layoutInvalidation.args.data.reason !== Types.TraceEvents.LayoutInvalidationReason.STYLE_CHANGED &&
layoutInvalidation.args.data.reason !== Types.TraceEvents.LayoutInvalidationReason.ADDED_TO_LAYOUT) {
return null;
}
const iframe = firstIframeInDOMTree(layoutInvalidationDOMNode);

const layoutInvalidationNode = await this.#protocolInterface.getNode(layoutInvalidationNodeId);
if (!layoutInvalidationNode) {
return null;
}

const iframe = firstIframeInDOMTree(layoutInvalidationNode);
if (!iframe) {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion front_end/models/trace/root-causes/RootCauses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {LayoutShiftRootCauses} from './LayoutShift.js';
export type RootCauseProtocolInterface = {
getInitiatorForRequest(url: string): Protocol.Network.Initiator|null,
pushNodesByBackendIdsToFrontend(backendNodeIds: Protocol.DOM.BackendNodeId[]): Promise<Protocol.DOM.NodeId[]>,
getNode(nodeId: Protocol.DOM.NodeId): Promise<Protocol.DOM.Node>,
getNode(nodeId: Protocol.DOM.NodeId): Promise<Protocol.DOM.Node|null>,
getComputedStyleForNode(nodeId: Protocol.DOM.NodeId): Promise<Protocol.CSS.CSSComputedStyleProperty[]>,
getMatchedStylesForNode(nodeId: Protocol.DOM.NodeId): Promise<Protocol.CSS.GetMatchedStylesForNodeResponse>,
fontFaceForSource(url: string): Protocol.CSS.FontFace|undefined,
Expand Down

0 comments on commit 9315458

Please sign in to comment.