Bug 230287

Summary:

Web Inspector: `TreeOutline` should return early when failing to find an ancestor while populating the tree

Product:

WebKit

Reporter:

Patrick Angle <pangle>

Component:

Web Inspector

Assignee:

Patrick Angle <pangle>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch v1.0	none
Patch v1.1	none

Description Patrick Angle 2021-09-14 18:21:47 PDT

Comment 1 Radar WebKit Bug Importer 2021-09-14 18:22:02 PDT

<rdar://problem/83125960>

Comment 2 Patrick Angle 2021-09-14 18:31:56 PDT

Created attachment 438201 [details]
Patch v1.0

Comment 3 Devin Rousso 2021-09-15 11:20:02 PDT

Comment on attachment 438201 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=438201&action=review

r=me, good catch :)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:518
>              item = this.findTreeElement(ancestors[i], isAncestor, getParent);

Aside: I wonder if maybe this should just be `this.getCachedTreeElement(ancestors[i])`, as there's really no point to walking up the `getParent` chain again since we've already done that.  Tho you might need to duplicate the bit that iterates over `this.children`, but conceptually if we have an item inside `this.children` then it should already be cached.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:520
> +                return;

this should be `return null;` to match the other `return`

Comment 4 Devin Rousso 2021-09-15 11:28:10 PDT

Comment on attachment 438201 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=438201&action=review

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:518
>>              item = this.findTreeElement(ancestors[i], isAncestor, getParent);
> 
> Aside: I wonder if maybe this should just be `this.getCachedTreeElement(ancestors[i])`, as there's really no point to walking up the `getParent` chain again since we've already done that.  Tho you might need to duplicate the bit that iterates over `this.children`, but conceptually if we have an item inside `this.children` then it should already be cached.

Aside: we may also wanna add an early-return above the `while` if `getParent` is NOT provided, as right now it's kinda this silent assumption 😅

Comment 5 Patrick Angle 2021-09-15 14:37:01 PDT

Created attachment 438291 [details]
Patch v1.1

Comment 6 EWS 2021-09-15 15:12:08 PDT

Committed r282477 (241725@main): <https://commits.webkit.org/241725@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438291 [details].