[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]

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
https://bugs.webkit.org/show_bug.cgi?id=198228

Attachment 371162: Patch

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




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

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
ChangeLog.

> 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 -
treeElement.listItemElement.totalOffsetTop;

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

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:493
> +	       let topOffset = headerRect.bottom -
treeElement.listItemElement.totalOffsetTop;

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)
       
this.treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRevealed,
{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
unique/searchable.