[Webkit-unassigned] [Bug 96733] Web Inspector: Display Named Flows in the Tabbed Pane of the "CSS Named Flows" Drawer
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 17 02:07:48 PDT 2012
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.
More information about the webkit-unassigned
mailing list