[Webkit-unassigned] [Bug 96733] Web Inspector: Display Named Flows in the Tabbed Pane of the "CSS Named Flows" Drawer

[bugzilla-daemon at webkit.org]

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


Alexander Pavlov (apavlov) <apavlov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #164280|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #4 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2012-09-17 02:08:16 PST ---
(From update of attachment 164280)
View in context: https://bugs.webkit.org/attachment.cgi?id=164280&action=review

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:135
>          var flowTreeElement = new TreeElement(container, flowContainer);

This has slipped through already, but I just checked the existing code and confirmed that we typically use the suffix "Element" for DOM elements. E.g.
var overviewTreeElement = document.createElement("ol");
is a DOM element for which a TreeOutline will be constructed. TreeElement instances usually have the suffix "Item" or "TreeItem" (e.g. "flowTreeItem"). Fix at your discretion.

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:199
> +            if (flow.overset) {

I'd suggest that you extend TreeElement and extract this "overset" property modification code into a separate method on the extending object's prototype (setOverset: function(boolean) or something). Then you could reuse it when constructing flowTreeElement above (e.g. by passing "flow" or "flow.overset" into the constructor).

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:306
> +                flowContainer.flowView =  new WebInspector.CSSNamedFlowView(flowContainer.flow);

extra whitespace after '='

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:43
> +    this._contentElement = new TreeElement("content", null, true);

Avoid "Element" suffix. E.g., this._contentTreeItem
Also, "content" should be set using WebInspector.UIString()

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:46
> +    this._regionsElement = new TreeElement("region chain", null, true);

Avoid "Element" suffix. E.g., this._regionsTreeItem
Also, "region chain" should be set using WebInspector.UIString()

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:63
> +     * @param {WebInspector.DOMNode|undefined} rootDOMNode

In parameter types, this is expressed as
@param {WebInspector.DOMNode=} rootDOMNode (see https://developers.google.com/closure/compiler/docs/js-for-compiler#types, "Optional parameter in a @param annotation")

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:64
> +     * @return {WebInspector.ElementsTreeOutline|null}

"|null" is not necessary, since all object types are nullable by default, but if you feel like emphasizing this, use
@return {?WebInspector.ElementsTreeOutline}

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:76
> +        WebInspector.domAgent.removeEventListener(WebInspector.DOMAgent.Events.DocumentUpdated, treeOutline._elementsTreeUpdater._documentUpdated, treeOutline._elementsTreeUpdater);

Why is this necessary? The corresponding entry will stick around even after a page navigation?

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:139
> +        regionTreeElement.tooltip = WebInspector.UIString("Region is " + newRegionOverset + ".");

Perhaps having className suffix be a word in the message text is not a good idea. It's better to have a choice, something like
var oversetText = newRegionOverset === "foo" ? WebInspector.UIString("bar") : WebInspector.UIString("baz"); // or something more complicated, but you get the idea,
and then do:
regionTreeElement.tooltip = WebInspector.UIString("Region is %s.", oversetText);

This way, you will have separate messages for "Region is %s.", "bar", and "baz" in localizedStrings.js, and it will be possible to easily build the target message in any natural language.

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:147
> +        var i, j, k;

Declare variables as late as possible (i.e. along with their initialization). And "for" should use its own counter var: "for (var i = ...)".
optional: These vars perhaps need more verbose names (oldContentIndex, newContentIndex, etc.), or the code should perhaps be accompanied by some specification reference URL if this algorithm is described somewhere online.

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:159
> +        /** Merge content nodes */

/** ... */-style comment is a JSDoc comment, which is not appropriate here. Also, rather than have this comment, you should extract this into a separate method with a self-describing name (e.g. _mergeContentNodes). If I get it right, everything from the beginning of the method down to the end of this loop should go there.

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:188
> +        var oldRegions = this._flow.regions;

Ditto.

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