[webkit-reviews] review granted: [Bug 109796] Web Inspector: extract DOM counters graph implementation into its own class : [Attachment 188290] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 109796: Web Inspector: extract DOM counters graph implementation into its
own class
https://bugs.webkit.org/show_bug.cgi?id=109796

Attachment 188290: Patch
https://bugs.webkit.org/attachment.cgi?id=188290&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188290&action=review


> Source/WebCore/inspector/front-end/DOMCountersGraph.js:32
> + * @param {WebInspector.TimelinePanel} timelinePanel

@param tags should follow the @constructor and @extends tags

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:48
> +WebInspector.DOMCounterUI = function(memoryCountersPane, title,
currentValueLabel, rgb, valueGetter)

JSDoc for the params, please

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:50
> +    var swatchColor = "rgb(" + rgb.join(",") + ")";

BTW, why don't you use WebInspector.Color? It will ease your life.

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:66
> +WebInspector.DOMCountersGraph.Counter = function(time, documentCount,
nodeCount, listenerCount)

JSDoc for the params, please

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:159
> +	       new WebInspector.DOMCounterUI(this, "Document Count",
"Documents: %d", [100,0,0], getDocumentCount),

spaces between array elements everywhere

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:242
> +	       ctx.arc(x, y, radius, 0, Math.PI*2, true);

whitespace around '*'

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:289
> +	   var currentY = originY + (height -
(valueGetter(this._counters[this._minimumIndex])- minValue) * yFactor);

whitespace before '-'

> Source/WebCore/inspector/front-end/DOMCountersGraph.js:294
> +		currentY = originY + (height - (valueGetter(this._counters[i])-
minValue) * yFactor);

ditto