Bug 90871 - Web Inspector: Display Named Flows in the "CSS Named Flows" drawer
Summary: Web Inspector: Display Named Flows in the "CSS Named Flows" drawer
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on: 93443
Blocks: 90786 96733
  Show dependency treegraph
Reported: 2012-07-10 05:00 PDT by Andrei Poenaru
Modified: 2012-09-14 05:05 PDT (History)
12 users (show)

See Also:

Patch (18.85 KB, patch)
2012-08-16 07:54 PDT, Andrei Poenaru
pfeldman: review-
Details | Formatted Diff | Diff
Screenshot1 (58.27 KB, image/png)
2012-08-16 08:18 PDT, Andrei Poenaru
no flags Details
Screenshot2 (54.33 KB, image/png)
2012-08-16 08:19 PDT, Andrei Poenaru
no flags Details
Screenshot3 (34.57 KB, image/gif)
2012-08-16 08:29 PDT, Andrei Poenaru
no flags Details
Patch (21.77 KB, patch)
2012-09-12 22:59 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
compile-front-end.py output diff (1.89 KB, text/plain)
2012-09-12 23:00 PDT, Andrei Poenaru
no flags Details
Patch (23.98 KB, patch)
2012-09-13 00:55 PDT, Andrei Poenaru
apavlov: review-
Details | Formatted Diff | Diff
Patch (26.72 KB, patch)
2012-09-13 04:28 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
Patch (28.11 KB, patch)
2012-09-13 10:50 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
Patch (27.30 KB, patch)
2012-09-14 01:35 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff
Patch (26.94 KB, patch)
2012-09-14 04:29 PDT, Andrei Poenaru
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Poenaru 2012-07-10 05:00:54 PDT
Display Named Flows from a web page in the sidebar pane.
Comment 1 Timothy Hatcher 2012-07-10 09:54:47 PDT
Can you provide more details or a mock up on what you are planning here?
Comment 2 Vsevolod Vlasov 2012-07-10 09:56:30 PDT
(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
Comment 3 Timothy Hatcher 2012-07-10 09:57:05 PDT
Dupe of 90789 then?
Comment 4 Andrei Poenaru 2012-08-16 07:54:20 PDT
Created attachment 158821 [details]
Comment 5 Pavel Feldman 2012-08-16 08:11:45 PDT
Could you please attach a couple of screenshots?
Comment 6 Andrei Poenaru 2012-08-16 08:18:55 PDT
Created attachment 158827 [details]
Comment 7 Andrei Poenaru 2012-08-16 08:19:36 PDT
Created attachment 158828 [details]
Comment 8 Andrei Poenaru 2012-08-16 08:29:45 PDT
Created attachment 158831 [details]
Comment 9 Andrei Poenaru 2012-08-16 08:30:58 PDT
Comment on attachment 158831 [details]

This is not the final version of the "Overflows" icon.
Comment 10 Pavel Feldman 2012-08-16 08:46:24 PDT
Comment on attachment 158821 [details]

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

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?).
Comment 11 Pavel Feldman 2012-08-16 08:47:00 PDT
Comment on attachment 158821 [details]

r- as per comments above (primarily context menu and treeoutline).
Comment 12 Andrei Poenaru 2012-09-12 22:59:19 PDT
Created attachment 163781 [details]

Please see next attachment for a diff of compile-front-end.py output with and without the patch.
Patch also contains an icon.
Comment 13 Andrei Poenaru 2012-09-12 23:00:01 PDT
Created attachment 163782 [details]
compile-front-end.py output diff
Comment 14 Alexander Pavlov (apavlov) 2012-09-12 23:12:30 PDT
Comment on attachment 163781 [details]

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.CSSNamedFlowCollectionsView does not implement WebInspector.ContextMenu.Provider, so you should annotate it here as

@implements {WebInspector.ContextMenu.Provider}
Comment 15 Andrei Poenaru 2012-09-13 00:55:18 PDT
Created attachment 163805 [details]
Comment 16 Alexander Pavlov (apavlov) 2012-09-13 02:47:41 PDT
Comment on attachment 163805 [details]

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
Comment 17 Andrei Poenaru 2012-09-13 04:28:03 PDT
Created attachment 163837 [details]
Comment 18 Alexander Pavlov (apavlov) 2012-09-13 08:11:32 PDT
Comment on attachment 163837 [details]

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);
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"?
Comment 19 Andrei Poenaru 2012-09-13 10:50:33 PDT
Created attachment 163906 [details]

I set the text using WebInspector.UIString late because localizedStrings are not processed when WebInspector.cssNamedFlowCollections object is created.
Comment 20 Andrei Poenaru 2012-09-14 01:35:04 PDT
Created attachment 164069 [details]
Comment 21 Alexander Pavlov (apavlov) 2012-09-14 02:28:07 PDT
Comment on attachment 164069 [details]

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
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");


> 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

> 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

_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.
Comment 22 Andrei Poenaru 2012-09-14 04:29:39 PDT
Created attachment 164100 [details]
Comment 23 Alexander Pavlov (apavlov) 2012-09-14 04:56:22 PDT
Comment on attachment 164100 [details]

Comment 24 WebKit Review Bot 2012-09-14 05:05:24 PDT
Comment on attachment 164100 [details]

Clearing flags on attachment: 164100

Committed r128590: <http://trac.webkit.org/changeset/128590>
Comment 25 WebKit Review Bot 2012-09-14 05:05:29 PDT
All reviewed patches have been landed.  Closing bug.