[webkit-reviews] review granted: [Bug 235924] Web Inspector: [Flexbox] Show flex badge next to flex containers in DOM Tree : [Attachment 451603] Patch

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 235924: Web Inspector: [Flexbox] Show flex badge next to flex containers in
DOM Tree
https://bugs.webkit.org/show_bug.cgi?id=235924

Attachment 451603: Patch

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




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

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

r=me, nice work! I'm glad we were able to do this with a lot less duplicated
code.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:190
> +	  
console.assert(Object.values(WI.DOMNode.LayoutContextType).includes(domNode.lay
outContextType), domNode);

I don't think this assertion will pass if a node becomes something other than
the currently recognized context types of Grid and Flex. Because changing from
one context type to any other context type, or null, is valid I don't think we
need an assertion here any more.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:220
>	   this._nextDefaultGridColorIndex = 0;
> +	   this._nextDefaultFlexColorIndex = 0;

As an aside, I think we might have a pre-existing bug where the order of colors
is possibly dependent on if the Layout sidebar panel is open when loading a
page because only when it is open are all the various layout containers sent to
the frontend. If just the DOM tree is visible, only the DOM nodes that have
made it from the backend to the frontend will get badges, which means deeply
nested child nodes with grid or flex contexts aren't known about until the tree
is expanded, at which point you might have used color 1 before the child
somewhere and color 2 after the child somewhere, which means we might use color
3 for the child, which in the document is declared between the first and second
nodes that happened to already be visible. In practice I don't think this is
too big of a deal, and given it is an existing potential issue with mild
symptoms we certainly don't need to solve it in this patch. But I thought I'd
mention it while it was fresh in my mind.