Bug 122924 – Web Inspector: CSS Regions: Add the list of flows in the FrameTreeElement

• Component: Web Inspector
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Add the list of flows in the resources panel on the left. Every frame should have it's own list of flows.

[Radar WebKit Bug Importer]

<rdar://problem/15246973>

[Alexandru Chiculita]

Created attachment 214404 [details]
Patch V1

[Alexandru Chiculita]

Created attachment 214407 [details]
Patch V2

Rebased.

[Joseph Pecoraro]

Comment on attachment 214404 [details]
Patch V1

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

This is purely a style pass. I briefly looked over the real code changes and they looked good, but I'd like to see a style update to start.

> Source/WebInspectorUI/UserInterface/ContentFlow.js:2
> + * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.

NOTE: Since this is the first time there is an Adobe Systems Copyright you should add Adobe to the list of Copyrights in Source/WebInspectorUI/Scripts/copy-user-interface-resources.sh in the LICENSE text appended to the top of minified sources!

> Source/WebInspectorUI/UserInterface/ContentFlow.js:32
> +    this._loadFromJSON(data);

This class extends WebInspector.Object, so it should call the super constructor:

    WebInspector.Object.call(this);

> Source/WebInspectorUI/UserInterface/ContentFlow.js:48
> +

The first property of any WebInspector.Foo class should be the constructor property:

    constructor: WebInspector.ContentFlow

> Source/WebInspectorUI/UserInterface/ContentFlow.js:73
> +                if (currentRegion.nodeId !== newRegion.nodeId
> +                    || currentRegion.regionOverset !== newRegion.regionOverset) {

No need for the line break here. We are not shy about long lines.

> Source/WebInspectorUI/UserInterface/ContentFlow.js:91
> +    update: function(data)

The typical style of a WebInspector.Foo class is:

    WebInspector.Foo = function(foo)
    {
        /* constructor */
        this._foo = foo; /* Member variables are private and underscore prefixed */
    };

    WebInspector.Foo.prototype = {
        constructor: WebInspector.Foo,

        // Public

        /* getters and setters */

        get foo()
        {
            return this._foo; /* access to private ivars through public getters */
        },

        set foo(foo)
        {
            this._foo = foo; /* mutators to private ivars through public setters */
        },

        publicMethod: function()
        {
            /* public methods called outside the class */
        },

        // Private

        _privateMethod: function()
        {
            /* private methods are underscore prefixed */
        }
    };

A great example of this is FilterBar.js:
<http://trac.webkit.org/browser/trunk/Source/WebInspectorUI/UserInterface/FilterBar.js>

Review comments would be:
1. Make _private member variables and getters/setters for public accessors.
2. Add // Public and // Private comments, and reorder the functions.

> Source/WebInspectorUI/UserInterface/ContentFlowTreeElement.js:30
> +WebInspector.ContentFlowTreeElement = function(representedObject)

Nice, this class looks prefect.

> Source/WebInspectorUI/UserInterface/DOMTree.js:262
> +        function updateFlowList(rootNode)
> +        {
> +            // Let the backend know we are interested about the named flow events for this document.
> +            WebInspector.domTreeManager.getNamedFlowCollection(rootNode.id);
> +        }
> +        this.requestRootDOMNode(updateFlowList);

We have been moving toward just inlining anonymous function like this, especially if they don't need a .bind(). I find it much easier to read:

    this.requestRootDOMNode(function(rootNode) {
        ...
    });

> Source/WebInspectorUI/UserInterface/DOMTree.js:313
> +        for (var flowKey in deletedFlows)
> +            this.dispatchEventToListeners(WebInspector.DOMTree.Event.FlowWasRemoved, { flow: deletedFlows[flowKey] });
> +
> +        for (var i = 0; i < newFlows.length; ++i)
> +            this.dispatchEventToListeners(WebInspector.DOMTree.Event.FlowWasAdded, { flow: newFlows[i] });

Style for JavaScript Object Literals is: {key1: value1, key2: value2}. So you should remove the excess whitespace in the literals in this patch.

> Source/WebInspectorUI/UserInterface/DOMTree.js:333
> +    _flowWasRemovd: function(event)

Typo: _flowWasRemovd => _flowWasRemoved

[Timothy Hatcher]

Comment on attachment 214404 [details]
Patch V1

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

A quick pass for me as well until style is tweaked.

> Source/WebInspectorUI/UserInterface/ContentFlow.js:45
> +WebInspector.ContentFlow.hashKey = function(flow)
> +{
> +    // Use the flow node id, to avoid collisions when we change main document id.
> +    return flow.documentNodeId + ":" + flow.name;
> +};

I think this would be better as a member. ContentFlow.prototype.id to match other classes.

> Source/WebInspectorUI/UserInterface/DOMTree.js:59
> +    FlowWasAdded: "dom-tree-flow-was-added",
> +    FlowWasRemoved: "dom-tree-flow-was-removed"

You use the name ContentFlow for the classes. I think the events should also have ContentFlow in the name.

> Source/WebInspectorUI/UserInterface/DOMTree.js:267
> +        return this._rootDOMNode && (flow.documentNodeId === this._rootDOMNode.id);

No need for the ( ).

> Source/WebInspectorUI/UserInterface/FrameTreeElement.js:435
> +        return aIsResource ? 1 : -1;

This works for now, but a FIXME would be good to mention grouping items by tree element subclass if a and b are different.

> Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">

It would be better to have this be intrinsically 16 x 16. I can help make an icon that fits the look and feel of the existing icons. I think this icon is good, it just needs a color treatment.

> Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:9
> +	<path fill="#707070" d="M81.25,6.25h-62.5c-6.885,0-12.5,5.615-12.5,12.5v62.5c0,6.885,5.615,12.5,12.5,12.5h62.5
> +		c6.885,0,12.5-5.615,12.5-12.5v-62.5C93.75,11.865,88.135,6.25,81.25,6.25 M81.25,12.5c3.442,0,6.25,2.808,6.25,6.25v62.5
> +		c0,3.442-2.808,6.25-6.25,6.25h-62.5c-3.442,0-6.25-2.808-6.25-6.25v-62.5c0-3.442,2.808-6.25,6.25-6.25H81.25"/>
> +	<rect x="17.875" y="19.625" fill="#717171" width="29.292" height="24.708"/>
> +	<rect x="17.875" y="53" fill="#717171" width="16.625" height="26.25"/>
> +	<rect x="55.375" y="19.625" fill="#717171" width="26.625" height="61"/>

We prefer using rgb() over hex colors. We also prefer not hard wrapping <path> and using spaces for all delimiters. Like: d="M 81.25 6.25 h -62.5 c -6.885 0 -12.5 5.615 -12.5…"

[Alexandru Chiculita]

Created attachment 214472 [details]
Patch V3

Thanks for the review. Sorry about the style, every JS project is so different about coding patterns and styles.

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 214404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214404&action=review
> 
> A quick pass for me as well until style is tweaked.
> 
> > Source/WebInspectorUI/UserInterface/ContentFlow.js:45
> > +WebInspector.ContentFlow.hashKey = function(flow)
> > +{
> > +    // Use the flow node id, to avoid collisions when we change main document id.
> > +    return flow.documentNodeId + ":" + flow.name;
> > +};
> 
> I think this would be better as a member. ContentFlow.prototype.id to match other classes.
> 
> > Source/WebInspectorUI/UserInterface/DOMTree.js:59
> > +    FlowWasAdded: "dom-tree-flow-was-added",
> > +    FlowWasRemoved: "dom-tree-flow-was-removed"
> 
> You use the name ContentFlow for the classes. I think the events should also have ContentFlow in the name.
> 
> > Source/WebInspectorUI/UserInterface/DOMTree.js:267
> > +        return this._rootDOMNode && (flow.documentNodeId === this._rootDOMNode.id);
> 
> No need for the ( ).
> 
> > Source/WebInspectorUI/UserInterface/FrameTreeElement.js:435
> > +        return aIsResource ? 1 : -1;
> 
> This works for now, but a FIXME would be good to mention grouping items by tree element subclass if a and b are different.
> 
> > Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:3
> > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
> 
> It would be better to have this be intrinsically 16 x 16. I can help make an icon that fits the look and feel of the existing icons. I think this icon is good, it just needs a color treatment.
> 
> > Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:9
> > +	<path fill="#707070" d="M81.25,6.25h-62.5c-6.885,0-12.5,5.615-12.5,12.5v62.5c0,6.885,5.615,12.5,12.5,12.5h62.5
> > +		c6.885,0,12.5-5.615,12.5-12.5v-62.5C93.75,11.865,88.135,6.25,81.25,6.25 M81.25,12.5c3.442,0,6.25,2.808,6.25,6.25v62.5
> > +		c0,3.442-2.808,6.25-6.25,6.25h-62.5c-3.442,0-6.25-2.808-6.25-6.25v-62.5c0-3.442,2.808-6.25,6.25-6.25H81.25"/>
> > +	<rect x="17.875" y="19.625" fill="#717171" width="29.292" height="24.708"/>
> > +	<rect x="17.875" y="53" fill="#717171" width="16.625" height="26.25"/>
> > +	<rect x="55.375" y="19.625" fill="#717171" width="26.625" height="61"/>
> 
> We prefer using rgb() over hex colors. We also prefer not hard wrapping <path> and using spaces for all delimiters. Like: d="M 81.25 6.25 h -62.5 c -6.885 0 -12.5 5.615 -12.5…"

I didn't see your comment when I've uploaded the patch. Please ignore it until I have a chance to go through the second review.

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 214404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214404&action=review
> 
> A quick pass for me as well until style is tweaked.
Thanks!

> 
> > Source/WebInspectorUI/UserInterface/ContentFlow.js:45
> > +WebInspector.ContentFlow.hashKey = function(flow)
> > +{
> > +    // Use the flow node id, to avoid collisions when we change main document id.
> > +    return flow.documentNodeId + ":" + flow.name;
> > +};
> 
> I think this would be better as a member. ContentFlow.prototype.id to match other classes.
I use this on JSON structures that I receive from the backend. Maybe it is the wrong place to have it and would better stay in the DOMTree file instead. I like how it can work on both pure JSON and ContentFlows as they use the same names, but so far I only needed it for JSON objects.

> 
> > Source/WebInspectorUI/UserInterface/DOMTree.js:59
> > +    FlowWasAdded: "dom-tree-flow-was-added",
> > +    FlowWasRemoved: "dom-tree-flow-was-removed"
> 
> You use the name ContentFlow for the classes. I think the events should also have ContentFlow in the name.
> 

Is this what you had in mind? I will update all the other events to match.

ContentFlowWasAdded: "dom-tree-content-flow-was-added",
ContentFlowWasRemoved: "dom-tree-content-flow-was-removed"

> > Source/WebInspectorUI/UserInterface/DOMTree.js:267
> > +        return this._rootDOMNode && (flow.documentNodeId === this._rootDOMNode.id);
> 
> No need for the ( ).
Ok.

> 
> > Source/WebInspectorUI/UserInterface/FrameTreeElement.js:435
> > +        return aIsResource ? 1 : -1;
> 
> This works for now, but a FIXME would be good to mention grouping items by tree element subclass if a and b are different.

Ok.

> > Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:3
> > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
> 
> It would be better to have this be intrinsically 16 x 16. I can help make an icon that fits the look and feel of the existing icons. I think this icon is good, it just needs a color treatment.
> 
Ok, I can fix the 16 x 16. I created the icon myself, so I was hoping it is just temporary until someone with better color taste/skills would fix it :) Please help!

> > Source/WebInspectorUI/UserInterface/Images/ContentFlow.svg:9
> > +	<path fill="#707070" d="M81.25,6.25h-62.5c-6.885,0-12.5,5.615-12.5,12.5v62.5c0,6.885,5.615,12.5,12.5,12.5h62.5
> > +		c6.885,0,12.5-5.615,12.5-12.5v-62.5C93.75,11.865,88.135,6.25,81.25,6.25 M81.25,12.5c3.442,0,6.25,2.808,6.25,6.25v62.5
> > +		c0,3.442-2.808,6.25-6.25,6.25h-62.5c-3.442,0-6.25-2.808-6.25-6.25v-62.5c0-3.442,2.808-6.25,6.25-6.25H81.25"/>
> > +	<rect x="17.875" y="19.625" fill="#717171" width="29.292" height="24.708"/>
> > +	<rect x="17.875" y="53" fill="#717171" width="16.625" height="26.25"/>
> > +	<rect x="55.375" y="19.625" fill="#717171" width="26.625" height="61"/>
> 
> We prefer using rgb() over hex colors. We also prefer not hard wrapping <path> and using spaces for all delimiters. Like: d="M 81.25 6.25 h -62.5 c -6.885 0 -12.5 5.615 -12.5…"

That file was generated by Illustrator and I just cleaned it up. I will fix the style and reupload.

[Alexandru Chiculita]

Created attachment 214476 [details]
Patch V4

[Timothy Hatcher]

Comment on attachment 214404 [details]
Patch V1

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

>>>> Source/WebInspectorUI/UserInterface/ContentFlow.js:45
>>>> +};
>>> 
>>> I think this would be better as a member. ContentFlow.prototype.id to match other classes.
>> 
>> I didn't see your comment when I've uploaded the patch. Please ignore it until I have a chance to go through the second review.
> 
> I use this on JSON structures that I receive from the backend. Maybe it is the wrong place to have it and would better stay in the DOMTree file instead. I like how it can work on both pure JSON and ContentFlows as they use the same names, but so far I only needed it for JSON objects.

We try to keep the protocol objects in the Observer and Manager classes. Exposing the protocol to other classes causes a compatibility nightmare in the future if things change.

> Source/WebInspectorUI/UserInterface/DOMTree.js:342
> +        var flow = event.data.flow;
> +        if (!this._isFlowInCurrentDocument(flow))
> +            return;
> +
> +        var flowKey = WebInspector.ContentFlow.hashKey(flow);
> +        console.assert(this._flows.hasOwnProperty(flowKey));
> +        flow = this._flows[flowKey];
> +        delete this._flows[flowKey];

Yeah, I didn't notice this before how it is JSON once then ContentFlow later. Using a different variable and keeping the key generation in the manager would be preferred. We use "payload" when labeling protocol objects. So flowPayload, then later flow would be the ContentFlow object.

> Source/WebInspectorUI/UserInterface/DOMTreeManager.js:547
> +        this.dispatchEventToListeners(WebInspector.DOMTreeManager.Event.FlowWasAdded, { flow: flow });

These flow objects are what I would expect to be WebInspector.ContentFlow already. Users of the events should not need to convert or manage them, that is the job of the manager class. It also allows future protocol changes to be handled here in one spot before involving other classes.

[Alexandru Chiculita]

Created attachment 214497 [details]
Patch V5

[Timothy Hatcher]

Comment on attachment 214497 [details]
Patch V5

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.

[WebKit Commit Bot]

Comment on attachment 214497 [details]
Patch V5

Rejecting attachment 214497 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 214497, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
mothy Hatcher']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Parsed 13 diffs from patch file(s).
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 279.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5248018

[Alexandru Chiculita]

Created attachment 214600 [details]
Patch V6

[Timothy Hatcher]

Comment on attachment 214600 [details]
Patch V6

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

> Source/WebInspectorUI/UserInterface/ContentFlow.js:49
> +    /* getters and setters */

No need for this comment.

[Alexandru Chiculita]

Created attachment 214601 [details]
Patch V7

Thanks! Rebased patch and fixed the last comment.

[WebKit Commit Bot]

Comment on attachment 214601 [details]
Patch V7

Clearing flags on attachment: 214601

Committed r157649: <http://trac.webkit.org/changeset/157649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.