[Webkit-unassigned] [Bug 24604] WebKit profiler reports incorrect total times

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24604





------- Comment #3 from timothy at hatcher.name  2009-04-23 11:06 PDT -------
(From update of attachment 29707)
+2009-04-23  Francisco Tolmasky  <set EMAIL_ADDRESS environment variable>

Make sure to include your email address in the ChangeLogs.

+         { "callUID", getCallUID, 0, kJSPropertyAttributeNone },

I think "identifier" or "callIdentifier" would be better names.

+    var profileNodeUIDs = 0,
+        profileNodeGroupIndex = 0,
+        profileNodeGroups = [[], [aProfileNode]],
+        visitedProfileNodesForCallUID = {};

I think this would look better having var on each line and not chained.

+    for (; profileNodeGroupIndex < profileNodeGroups.length;
++profileNodeGroupIndex)
+    {

Our coding style puts the "{" on the same line with the for.

+            if (profileNode.head && profileNode !== profileNode.head)
+            {

Same for if statements.

+                }
+                else {

The "}" should be on the same line as the "else {".

+                    var parentIndex = 0,
+                        parentCount = parentProfileNodes.length;

Same comment about the var from earlier.

+                    for (; parentIndex < parentCount; ++parentIndex)
+                        if (visitedNodes[parentProfileNodes[parentIndex].UID])
{
+                            totalTimeAccountedFor = true;
+                            break;
+                        }

The for loop should have braces since is is multiline, even if it isn't needed.

+            if (children.length)
+            {

Put the "{" on the same line with the if.

+        while (currentNode.parent && (currentNode instanceof
WebInspector.ProfileDataGridNode))
+        {

Same.

+            }
+            
+            // If not, add it as a true ancestor.
+            else {

The "else {" should be on the same line as "}" with the command inside the else
block.

+                if (ancestor !== focusNode)
+                {                    

Put the "{" on the same line with the if.

+            // only

What does this comment mean?

+            if (parent && parent.parent)
+            {

Same.

+        if (this.children.length <= 0)
+            this.hasChildren = false;

The concept for hasChildren was to allow it to be expandable and empty at the
same time. But I'm not sure we ever used that feature in DataGrid, but we do in
TreeOutline.

+        if (this._hasChildren)
+        {

Put the "{" on the same line with the if.

+        }
+        else
+        {

Should all be on the same line.

+++ WebCore/inspector/front-end/ProfileDataGridNode.js  (revision 0)

This file is missing the license.

+WebInspector.XProfileDataGridNode = function(aProfileView, aProfileNode,
aTree, hasChildren)

XProfileDataGridNode should be named something better.

+    if (aProfileNode)
+    {

Put the "{" on the same line with the if.

+    }
+    else
+    {

Should all be on the same line.

+    appendChild: function(/*ProfileDataGridNode*/ aProfileDataGridNode)
+    findChild: function(/*Node*/ aNode)

We don't use this naming technique for parameters. Just drop the "a".

+WebInspector.ProfileDataGridNode = function(aProfileView, aProfileNode, aTree,
hasChildren)

ProfileDataGridNode and XProfileDataGridNode should share code. Why did
XProfileDataGridNode get it's own file and ProfileDataGridNode stay with
ProfileDataGridTree? They could both live in ProfileDataGridNode.js and share
code.

I've given up on the style review at this point. Please correct all the style
formatting according to our style guidelines (including separate var statements
and no "a" prefix for parameters.)

+//        this.children.forEach(function(node) {
node._exclude(excludedCallUID); });

We don't commit commented out code. We also like to avoid forEach, since it is
pretty expensive compared to a normal for loop.

You also need to add all the new files to WebKit.qrc and WebCore.vcproj.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.