[webkit-reviews] review granted: [Bug 198228] Web Inspector: Debugger: sidebar should always reveal active call frame when hitting a breakpoint : [Attachment 371162] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 2 14:56:46 PDT 2019

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 198228: Web Inspector: Debugger: sidebar should always reveal active call
frame when hitting a breakpoint

Attachment 371162: Patch


--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 371162
  --> https://bugs.webkit.org/attachment.cgi?id=371162

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

r=me, but I haven't been able to apply/test locally because my build is broken

> Source/WebInspectorUI/ChangeLog:23
> +	   (WI.DetailsSection.prototype.get element):

NIT: since you're not really "editing" this function, I'd remove it from the

> Source/WebInspectorUI/ChangeLog:24
> +	   (WI.DetailsSection.prototype.get headerElement):

NIT: I like to add an "Added." suffix to functions that are new :)

> Source/WebInspectorUI/ChangeLog:25
> +	   (WI.DetailsSection.prototype.get identifier):

Ditto (>23).

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:348
> +	       let topOffset = headerRect.bottom -

It's interesting how we have a `totalOffset*` for everything but "bottom". 
Perhaps you could add one (e.g. `Element.prototype.totalOffsetBottom`)?

> +	       let topOffset = headerRect.bottom -

Ditto (>DebuggerSidebarPanel.js:348).

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:493
> +	       this.treeOutline.didRevealTreeElement(this);

It's fine to just inline this function here.  We do similar things elsewhere
for `TreeElement`.
    if (this.treeOutline)
{treeElement: this});

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1169
> +    ElementRevealed: Symbol("element-revealed"),

NIT: I know we used `Symbol`s in the past, but I personally prefer a more
verbose string (e.g. `tree-outline-element-revealed`) so that it's globally

More information about the webkit-reviews mailing list