Bug 96733 – Web Inspector: Display Named Flows in the Tabbed Pane of the "CSS Named Flows" Drawer

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

[Andrei Poenaru]

Selecting a Named Flow from the sidebar should open a tab that shows the content nodes and the regions associated with that flow.

[Andrei Poenaru]

Created attachment 164188 [details]
Patch

[Andrei Poenaru]

Created attachment 164280 [details]
Patch

[Andrei Poenaru]

Created attachment 164283 [details]
Screenshot

[Alexander Pavlov (apavlov)]

Comment on attachment 164280 [details]
Patch

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.

[Andrei Poenaru]

(In reply to comment #4)
> (From update of attachment 164280 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164280&action=review
> > 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 + ".");
>

I removed the DocumentUpdated event listener so that the ElementsTreeOutline does not change the rootDOMNode to the root of a new document. On DocumentUpdated I reset all the flows.

> 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.
I don't exactly understand what you meant here (the use cases).

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 164280 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164280&action=review
> > > 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 + ".");
> >
> 
> I removed the DocumentUpdated event listener so that the ElementsTreeOutline does not change the rootDOMNode to the root of a new document. On DocumentUpdated I reset all the flows.

OK, get it.

> > 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.
> I don't exactly understand what you meant here (the use cases).

As discussed on IRC, using _the same_ string as a protocol field enum value, CSS class, and a user message is _very_ brittle. You should have a map from the enum values into user-readable overset-type strings.

[Alexander Pavlov (apavlov)]

Comment on attachment 164280 [details]
Patch

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

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:108
> +        treeElement.tooltip = WebInspector.UIString("Region is " + region.regionOverset + ".");

I seem to have missed this occurrence of the same enum string-UI string issue in my previous review.

[Andrei Poenaru]

Created attachment 164393 [details]
Patch

[Alexander Pavlov (apavlov)]

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

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:383
> +WebInspector.FlowTreeElement = function(flowContainer)

Nice

> Source/WebCore/inspector/front-end/CSSNamedFlowCollectionsView.js:396
> +    get overset()

If possible, transform these into accessor and modifier methods (something like overset() and setOverset(newOverset)), since our latest decision is to transition from getters/setters to methods (we've had a lot of confusion about these, where sometimes they are methods, sometimes they are getters/setters, and accessing a property in a get-tish manner when its getter is actually a method results in a bug, which is not detectable by the compiler, alas). The code inside is fine.

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:222
> +                    this._updateRegionOverset(this._regionsTreeItem.children[regionsTreeChildIndex], newRegions[newRegionsIndex].regionOverset, oldRegions[oldRegionsIndex].regionOverset)

trailing semicolon lost

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:223
> +                ++oldRegionsIndex, ++newRegionsIndex, ++regionsTreeChildIndex;

We don't use this approach to update multiple variables - please make a separate statement for each of these, since this code is hardly readable for someone not familiar with what's happening here.

> Source/WebCore/inspector/front-end/CSSNamedFlowView.js:229
> +                ++newRegionsIndex, ++regionsTreeChildIndex;

Ditto

[Andrei Poenaru]

Created attachment 164398 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164398 [details]
Patch

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

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

This method seems unused - please remove, as we don't commit unused code (unfortunately, the compiler does not strip it from the shipped result).

[Andrei Poenaru]

Created attachment 164402 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 164402 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 164402 [details]
Patch

Clearing flags on attachment: 164402

Committed r128778: <http://trac.webkit.org/changeset/128778>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.