[webkit-reviews] review denied: [Bug 24604] WebKit profiler reports incorrect total times : [Attachment 29707] Fixes heavy view.

[bugzilla-daemon at webkit.org]

Kevin McCullough <kmccullough at apple.com> has denied Francisco Tolmasky
<tolmasky at gmail.com>'s request for review:
Bug 24604: WebKit profiler reports incorrect total times
https://bugs.webkit.org/show_bug.cgi?id=24604

Attachment 29707: Fixes heavy view.
https://bugs.webkit.org/attachment.cgi?id=29707&action=review

------- Additional Comments from Kevin McCullough <kmccullough at apple.com>
> Index: JavaScriptCore/ChangeLog
> ===================================================================
> --- JavaScriptCore/ChangeLog	(revision 42775)
> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,33 @@
> +	   Made it so that most the functions now match the behavior of Shark.
Most notably, in the heavy view,
> +	   child nodes now represent the statistics of the root node, but in
the callstack originating at that node.

Missing "of" in the first sentence.  I'm not sure what you mean in the second.


> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 42775)
> +++ WebCore/ChangeLog (working copy)
> +	   Made it so that most the functions now match the behavior of Shark.
Most notably, in the heavy view,
> +	   child nodes now represent the statistics of the root node, but in
the callstack originating at that node.

Same comment


> Index: WebCore/inspector/front-end/ProfileView.js
> ===================================================================
> --- WebCore/inspector/front-end/ProfileView.js	(revision 42746)
> +++ WebCore/inspector/front-end/ProfileView.js	(working copy)
> @@ -115,28 +163,34 @@ WebInspector.ProfileView.prototype = {
>	   var selectedProfileNode = this.dataGrid.selectedNode ?
this.dataGrid.selectedNode.profileNode : null;
>  
>	   this.dataGrid.removeChildren();
> +	   
> +	   var children = this.profileDataGridTree.children,
> +	       index = 0,
> +	       count = children.length;

Our style is to declare each variable with "var" on it's own line.  And this
will allow you to declare 'index' in the 'for' statement later.

>  
> -	   var children = this.profile.head.children;
> -	   var childrenLength = children.length;
> -	   for (var i = 0; i < childrenLength; ++i)
> -	       if (children[i].visible)
> -		   this.dataGrid.appendChild(new
WebInspector.ProfileDataGridNode(this, children[i]));
> +	   for (; index < count; ++index)
> +	       this.dataGrid.appendChild(children[index]);
>  

Here is where you can declare the 'index', since it's not used earlier

> @@ -191,71 +245,90 @@ WebInspector.ProfileView.prototype = {
>	   if (!isNaN(queryNumber) && !(greaterThan || lessThan))
>	       equalTo = true;
>  
> -	   function matchesQuery(profileNode)
> +	   function matchesQuery(/*ProfileDataGridNode*/ aProfileDataGridNode)

no need to put 'a' in front of arguments, just name this 'profileDataGridNode'.


> @@ -461,37 +542,21 @@ WebInspector.ProfileView.prototype = {
>	   this._sortProfile(this.profile);
>      },
>  
> -    _sortProfile: function(profile)
> +    _sortProfile: function()
>      {
> -	   if (!profile)
> -	       return;
> -
> -	   var sortOrder = this.dataGrid.sortOrder;
> -	   var sortColumnIdentifier = this.dataGrid.sortColumnIdentifier;
> -
> -	   var sortingFunctionName = "sort";
> -
> -	   if (sortColumnIdentifier === "self")
> -	       sortingFunctionName += "SelfTime";
> -	   else if (sortColumnIdentifier === "total")
> -	       sortingFunctionName += "TotalTime";
> -	   else if (sortColumnIdentifier === "calls")
> -	       sortingFunctionName += "Calls";
> -	   else if (sortColumnIdentifier === "function")
> -	       sortingFunctionName += "FunctionName";
> -
> -	   if (sortOrder === "ascending")
> -	       sortingFunctionName += "Ascending";
> -	   else
> -	       sortingFunctionName += "Descending";
> +	   var sortAscending = this.dataGrid.sortOrder === "ascending",
> +	       sortColumnIdentifier = this.dataGrid.sortColumnIdentifier,
> +	       sortProperty = { 
> +		   "average": "averageTime", 
> +		   "self": "selfTime", 
> +		   "total": "totalTime", 
> +		   "calls": "numberOfCalls", 
> +		   "function": "functionName"
> +	       }[sortColumnIdentifier];
> +	   

Same comment here about declaring each variable on its own line.

> Index: WebCore/inspector/front-end/inspector.html
> ===================================================================
> --- WebCore/inspector/front-end/inspector.html	(revision 42746)
> +++ WebCore/inspector/front-end/inspector.html	(working copy)
> @@ -73,6 +73,9 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE PO
>      <script type="text/javascript" src="DatabaseTableView.js"></script>
>      <script type="text/javascript" src="DatabaseQueryView.js"></script>
>      <script type="text/javascript" src="ScriptView.js"></script>
> +    <script type="text/javascript" src="ProfileDataGridTree.js"></script>
> +    <script type="text/javascript"
src="BottomUpProfileDataGridTree.js"></script>
> +    <script type="text/javascript"
src="TopDownProfileDataGridTree.js"></script>-

I don't think you need the '-' at the end of this line

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 42775)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +2009-04-23  Francisco Tolmasky  <set EMAIL_ADDRESS environment variable>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   BUG 24604: WebKit profiler reports incorrect total times
> +	   <https://bugs.webkit.org/show_bug.cgi?id=24604>
> +
> +	   Changed profile.treeProfile to just profile, since these aren't
generated in C++ anymore.
> +	   Removed heavy-view test since heavy-view isn't an actual tree that
is generated in C++ land anymore,
> +	   but rather just a different display of the normal treeProfile in the
JS data grid.
> +
> +	   * fast/profiler/heavy-view-expected.txt: Removed.
> +	   * fast/profiler/heavy-view.html: Removed.
> +	   * fast/profiler/resources/profiler-test-JS-resources.js:
profiles[i].treeProfile -> profiles[i].treeProfile
> +	   (printProfilesDataWithoutTime):
> +
>  2009-04-23  Brady Eidson  <beidson at apple.com>

If we are going to take this test out we should find a way to make a test so
that we can tell if we regress in the heavy-view.

>  
> Index: WebCore/English.lproj/localizedStrings.js
> ===================================================================
> Cannot display: file marked as a binary type.
> svn:mime-type = application/octet-stream
> 
> //52AGEAcgAgAGwAbwBjAGEAbABpAHoAZQBkAFMAdAByAGkAbgBnAHMAIAA9ACAAbgBlAHcAIABP
> AGIAagBlAGMAdAA7AAoACgBsAG8AYwBhAGwAaQB6AGUAZABTAHQAcgBpAG4AZwBzAFsAIgAgACgA

It looks like this file is marked binary, please make it human readable for
review