Bug 90871 – Web Inspector: Display Named Flows in the "CSS Named Flows" drawer

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrei Poenaru]

Display Named Flows from a web page in the sidebar pane.

[Timothy Hatcher]

Can you provide more details or a mock up on what you are planning here?

[Vsevolod Vlasov]

(In reply to comment #1)
> Can you provide more details or a mock up on what you are planning here?

See https://bugs.webkit.org/show_bug.cgi?id=90789

[Timothy Hatcher]

Dupe of 90789 then?

[Andrei Poenaru]

Created attachment 158821 [details]
Patch

[Pavel Feldman]

Could you please attach a couple of screenshots?

[Andrei Poenaru]

Created attachment 158827 [details]
Screenshot1

[Andrei Poenaru]

Created attachment 158828 [details]
Screenshot2

[Andrei Poenaru]

Created attachment 158831 [details]
Screenshot3

[Andrei Poenaru]

Comment on attachment 158831 [details]
Screenshot3

This is not the final version of the "Overflows" icon.

[Pavel Feldman]

Comment on attachment 158821 [details]
Patch

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

This looks good overall. I'd suggest that you use TreeOutline instead of your own list view, that should save you some styles / code.

> Source/WebCore/WebCore.gypi:6276
> +            'inspector/front-end/CSSNamedFlowsDrawer.js',

I'd call it a CSSNamedFlowsView just in case it moves from the drawer at some point.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:75445
> +					RelativePath="..\inspector\front-end\cssNamedFlows.css"

It should be also added into the WebKit.qrc. The magic number is 5 files to change for js and 4 files for css / images.

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:7
> +    WebInspector.SplitView.call(this, WebInspector.SplitView.SidebarPosition.Left, undefined);

Last parameter is optional, can be omitted.

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:8
> +    this.minimalSidebarWidth = Preferences.minScriptsSidebarWidth;

I don't think you need this line

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:12
> +    this.registerRequiredCSS("cssNamedFlows.css");

We typically make this line follow the super constructor.

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:34
> +    WebInspector.domAgent.requestDocument(this.setDocument.bind(this));

setDocument should be private: _setDocument.

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:81
> +        this._sidebarContentElement = document.createElement("div");

you could do

this._leftElement.createChild("div", "sidebar-content outline-disclosure")

to save some space.

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:85
> +        this._flowListElement = document.createElement("ol");

also, createChild

> Source/WebCore/inspector/front-end/CSSNamedFlowsDrawer.js:128
> +        var listElement = document.createElement("li");

You probably want to use TreeOutline and TreeElement here (treeoutline.js) - they are used all over the place in the inspector. Creating TreeOutline around "ol" and setting it a "outline-disclosure" class will give you nice expandable arrows. It also support selection, etc.

> Source/WebCore/inspector/front-end/ElementsPanel.js:342
> +        if (WebInspector.experimentsSettings.cssRegions.isEnabled()) {

In order to improve modularity (not depend from elements to named flows module), you should do modify ElementsTreeOutline.prototype.populateContextMenu instead. You should add
contextMenu.appendApplicableItems(domNode);

You module would call WebInspector.ContextMenu.registerProvider in the end of the file and would add your named flows action where applicable on demand (to all elements for now?).

[Pavel Feldman]

Comment on attachment 158821 [details]
Patch

r- as per comments above (primarily context menu and treeoutline).

[Andrei Poenaru]

Created attachment 163781 [details]
Patch

Please see next attachment for a diff of compile-front-end.py output with and without the patch.
Patch also contains an icon.

[Andrei Poenaru]

Created attachment 163782 [details]
compile-front-end.py output diff

[Alexander Pavlov (apavlov)]

Comment on attachment 163781 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:3
> + * @extends {WebInspector.SplitView}

According to the compilation script output:
Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:299: WARNING - actual parameter 1 of WebInspector.ContextMenu.registerProvider does not match formal parameter
found   : function (new:WebInspector.CSSNamedFlowCollectionsView): undefined
required: (WebInspector.ContextMenu.Provider|null)
WebInspector.ContextMenu.registerProvider(WebInspector.CSSNamedFlowCollectionsView);

WebInspector.CSSNamedFlowCollectionsView does not implement WebInspector.ContextMenu.Provider, so you should annotate it here as

@implements {WebInspector.ContextMenu.Provider}

[Andrei Poenaru]

Created attachment 163805 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 163805 [details]
Patch

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

You may want to somehow remove the tEXt ancillary chunks containing "Creation Date" and "Software" from namedFlowOverflow.png in order to reduce its size.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:1
> +/**

License boilerplate header missing

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:72
> +    _sidebarHasContent: function()

This naming convention (same for _sidebarIsEmpty) implies that the methods are accessors return a boolean. You should use something like "_setSidebarHasContent(boolean)" instead.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:93
> +        var flowContainer = {

No need to expand this onto multiple lines, we usually put this onto a single line.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:96
> +        }

Trailing ';' missing

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:194
> +        // FIXME: we only have support for Named Flows in the main document

Even though it is quite arbitrary in the legacy code, the text following FIXME: should be a full sentence (capitalized, with a trailing period), and we try to stick to this convention in the new code. Ditto for the following methods.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:204
> +        if (event.data.documentNodeId !== this._document.id)

In order to avoid compilation errors, declare
var data = /** @type {WebInspector.NamedFlow} */ event.data;
and then handle the |data| var instead.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:219
> +    hashNamedFlow: function(namedFlow)

All of these should be private methods (with a leading underscore)

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:221
> +        return "flow" + namedFlow.documentNodeId + namedFlow.name;

Can a flow name start with a digit? If so, we have a potential collision here.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:224
> +    hashRemovedNamedFlow: function(eventData)

You don't need this method (use hashNamedFlow instead).

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:244
> +    showNamedFlowForNode: function(node)

Should be private

> Source/WebCore/inspector/front-end/cssNamedFlows.css:1
> +.css-named-flow-collections-view .split-view-sidebar-left {

License boilerplate header missing

[Andrei Poenaru]

Created attachment 163837 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 163837 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:68
> +    appendApplicableItems: function(contextMenu, target)

Please annotate the methods that have arguments and/or return values. This will immensely simplify the code maintenance and improve code quality. Sorry I didn't mention this in the original review.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:249
> +    _hashNamedFlow: function(namedFlow, eventData)

Ugh, this is definitely not what I meant... I didn't notice that the methods had different argument types. But to keep things simple, you can provide documentNodeId and flowName as separate arguments to this method (since these are the only things that are actually needed.) The calls will be, respectively,

this._hashNamedFlow(flow.documentNodeId, flow.name);
and
this._hashNamedFlow(event.data.documentNodeId, event.data.flowName);

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:252
> +            return "flow" + namedFlow.documentNodeId + "|" + namedFlow.name;

While we are here, does the "flow" prefix do anything useful? Are you perhaps going to store some other kind of hash in the same "map"?

[Andrei Poenaru]

Created attachment 163906 [details]
Patch

I set the text using WebInspector.UIString late because localizedStrings are not processed when WebInspector.cssNamedFlowCollections object is created.

[Andrei Poenaru]

Created attachment 164069 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164069 [details]
Patch

Much better overall, a few more nits left. Feel free to ask questions online.

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

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:132
> +            var overflowIcon = document.createElement("img");

Sorry for overlooking this earlier. We decorate tree elements simply by setting appropriate classes on them and using images in .class:before. I can help you write the related CSS if you wish.

So, in this case you will just do
flowTreeElement.title.addStyleClass("named-flow-overflow");
and have the respective CSS place the desired image in the :before pseudo-element for you.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:197
> +                var overflowIcon = document.createElement("img");

Ditto

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:223
> +            this._showNamedFlowForNode(WebInspector.panels["elements"].treeOutline.selectedDOMNode());

This is not the correct way to address panels now, since panels are loaded lazily. Use
WebInspector.panel("elements").treeOutline....

> Source/WebCore/inspector/front-end/ElementsPanel.js:356
> +        WebInspector.showViewInDrawer(WebInspector.cssNamedFlowCollectionsView._statusElement, WebInspector.cssNamedFlowCollectionsView);

Since CSSNamedFlowCollectionsView is defined in a different file, you cannot access its private field (_statusElement). Instead, you should have something like
showInDrawer: function()
{
    WebInspector.showViewInDrawer(this._statusElement, this);
}
as a separate method on CSSNamedFlowCollectionsView.prototype, so that instead of this line you can call
WebInspector.cssNamedFlowCollectionsView.showInDrawer();


_showNamedFlowCollections() is only necessary to create the view instance as a field on the ElementsPanel lazily.

As an alternative, you can define showNamedFlowCollections() on WebInspector.CSSNamedFlowCollectionsView (as a "static" field), and create the view singleton as a field on WebInspector.CSSNamedFlowCollectionsView (e.g. WebInspector.CSSNamedFlowCollectionsView._instance). This way, you will not need the said showInDrawer() method at all.

[Andrei Poenaru]

Created attachment 164100 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164100 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 164100 [details]
Patch

Clearing flags on attachment: 164100

Committed r128590: <http://trac.webkit.org/changeset/128590>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.