Created attachment 164188 [details] Patch

Created attachment 164280 [details] Patch

Created attachment 164283 [details] Screenshot

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.

(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).

(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 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.

Created attachment 164393 [details] Patch

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

Created attachment 164398 [details] Patch

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).

Created attachment 164402 [details] Patch

Comment on attachment 164402 [details] Patch r=me

Comment on attachment 164402 [details] Patch Clearing flags on attachment: 164402 Committed r128778: <http://trac.webkit.org/changeset/128778>

All reviewed patches have been landed. Closing bug.