[webkit-reviews] review granted: [Bug 122924] Web Inspector: CSS Regions: Add the list of flows in the FrameTreeElement : [Attachment 214497] Patch V5

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Alexandru Chiculita
<achicu at adobe.com>'s request for review:
Bug 122924: Web Inspector: CSS Regions: Add the list of flows in the
FrameTreeElement
https://bugs.webkit.org/show_bug.cgi?id=122924

Attachment 214497: Patch V5
https://bugs.webkit.org/attachment.cgi?id=214497&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214497&action=review


> Source/WebInspectorUI/UserInterface/DOMTree.js:73
> +    get flows()
> +    {
> +	   return this._flows;
> +    },

I think this would be better named flowMap and _flowMap. Naming it flows
implies an array to me.

> Source/WebInspectorUI/UserInterface/DOMTree.js:78
> +    get flowsCount()
> +    {
> +	   return this._flowsCount;
> +    },

This could be return this._flowMap.keys.length; then you won't need to manage
the number.

> Source/WebInspectorUI/UserInterface/DOMTreeManager.js:566
> +	   var contentFlow = new WebInspector.ContentFlow(flowPayload);

This still pushed the protocol data into the model. Other managers deconstruct
the payload and pass the data to the constructor as separate parameters. But we
do have a desire to move to objects instead of multiple parameters in many
cases. And in the future we can massage the object if the protocol changes. I
just have a deep desire to firewall the protocol as much as possible after the
bad leakage that occurred in the old Inspector.