Bug 109796 - Web Inspector: extract DOM counters graph implementation into its own class
Summary: Web Inspector: extract DOM counters graph implementation into its own class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 01:36 PST by Yury Semikhatsky
Modified: 2013-02-14 02:24 PST (History)
8 users (show)

See Also:

Attachments
Patch (30.53 KB, patch)
2013-02-14 01:43 PST, Yury Semikhatsky 		apavlov: review+
Details | Formatted Diff | Diff
Patch for landing (31.11 KB, patch)
2013-02-14 02:16 PST, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2013-02-14 01:36:15 PST 
MemoryStatistics.js now implements DOM counters graph and also is inherited by NativeMemoryGraph.js. DOM counters specific should go into its own file leaving only common bits in MemoryStatistics.js.
Comment 1 Yury Semikhatsky 2013-02-14 01:43:05 PST 
Created attachment 188290 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-02-14 01:58:32 PST 
Comment on attachment 188290 [details]
Patch

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
Comment 3 Yury Semikhatsky 2013-02-14 02:14:28 PST 
(In reply to comment #2)
> (From update of attachment 188290 [details])
> 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
> 
Done.

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

> > 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.
> 
I only need to format string value here.

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

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

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

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

> > Source/WebCore/inspector/front-end/DOMCountersGraph.js:294
> > +             currentY = originY + (height - (valueGetter(this._counters[i])- minValue) * yFactor);
> 
> ditto
Done.
Comment 4 Yury Semikhatsky 2013-02-14 02:16:53 PST 
Created attachment 188294 [details]
Patch for landing
Comment 5 Yury Semikhatsky 2013-02-14 02:24:04 PST 
Committed r142862: <http://trac.webkit.org/changeset/142862>