[Webkit-unassigned] [Bug 90871] Web Inspector: Display Named Flows in the "CSS Named Flows" drawer

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Pavel Feldman <pfeldman at chromium.org>  2012-08-16 08:46:24 PST ---
(From update of attachment 158821)
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?).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.