Bug 96733

Summary: Web Inspector: Display Named Flows in the Tabbed Pane of the "CSS Named Flows" Drawer
Product: WebKit Reporter: Andrei Poenaru <andreigpoenaru>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 90871    
Bug Blocks: 90786    
Attachments:
Description Flags
Patch
none
Patch
apavlov: review-
Screenshot
none
Patch
none
Patch
none
Patch none

Description Andrei Poenaru 2012-09-14 00:15:07 PDT
Selecting a Named Flow from the sidebar should open a tab that shows the content nodes and the regions associated with that flow.
Comment 1 Andrei Poenaru 2012-09-14 10:37:01 PDT
Created attachment 164188 [details]
Patch
Comment 2 Andrei Poenaru 2012-09-15 03:56:33 PDT
Created attachment 164280 [details]
Patch
Comment 3 Andrei Poenaru 2012-09-15 05:00:10 PDT
Created attachment 164283 [details]
Screenshot
Comment 4 Alexander Pavlov (apavlov) 2012-09-17 02:08:16 PDT
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.
Comment 5 Andrei Poenaru 2012-09-17 04:33:52 PDT
(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).
Comment 6 Alexander Pavlov (apavlov) 2012-09-17 06:03:47 PDT
(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.
Comment 7 Alexander Pavlov (apavlov) 2012-09-17 06:11:40 PDT
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.
Comment 8 Andrei Poenaru 2012-09-17 07:33:13 PDT
Created attachment 164393 [details]
Patch
Comment 9 Alexander Pavlov (apavlov) 2012-09-17 07:58:01 PDT
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
Comment 10 Andrei Poenaru 2012-09-17 08:24:02 PDT
Created attachment 164398 [details]
Patch
Comment 11 Alexander Pavlov (apavlov) 2012-09-17 08:32:33 PDT
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).
Comment 12 Andrei Poenaru 2012-09-17 08:44:55 PDT
Created attachment 164402 [details]
Patch
Comment 13 Alexander Pavlov (apavlov) 2012-09-17 11:04:59 PDT
Comment on attachment 164402 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2012-09-17 11:12:43 PDT
Comment on attachment 164402 [details]
Patch

Clearing flags on attachment: 164402

Committed r128778: <http://trac.webkit.org/changeset/128778>
Comment 15 WebKit Review Bot 2012-09-17 11:12:46 PDT
All reviewed patches have been landed.  Closing bug.