[webkit-reviews] review granted: [Bug 234615] Web Inspector: Sources: cannot copy grouping of blackboxed call frames : [Attachment 448357] Patch

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 234615: Web Inspector: Sources: cannot copy grouping of blackboxed call
frames
https://bugs.webkit.org/show_bug.cgi?id=234615

Attachment 448357: Patch

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




--- Comment #3 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 448357
  --> https://bugs.webkit.org/attachment.cgi?id=448357
Patch

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

r=me

> The only downside I can think of as a result of this is that up/down will now
traverse
> through blackboxed call frame groups, meaning that it's an extra step (or
more) to
> select the previous/next call frame using the arrow keys when there's a
blackboxed
> call frame group in the middle.

I tried this patch locally, and this doesn't appear to be a problem. I can
arrow through the list of call frames as I would expect to be able to do, where
each press of the down key moves me down to the next visible call frame, even
if there are multiple blackboxes call frames represented by the currently
selected frame.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:316
> +	   if (treeElement.toggleOnClick ||
treeElement.isEventWithinDisclosureTriangle(event)) {

Aside: It's kinda strange to me that several subclasses of TreeElement have a
toggleOnClick member set in their constructor, but TreeElement itself
doesn't... I wonder if it makes more sense for there to be a public `get
toggleOnClick()` in TreeElement that returns `false`, and is override by
subclasses that want to return `true`.