[webkit-reviews] review denied: [Bug 30520] Enable creation of custom SidebarTreeElements for different ProfileTypes : [Attachment 41433] patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 30520: Enable creation of custom SidebarTreeElements for different
ProfileTypes
https://bugs.webkit.org/show_bug.cgi?id=30520

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

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    sidebarTreeElementForProfile: function(profile)
> +    {
> +	   return new WebInspector.ProfileSidebarTreeElement(profile);
> +    },

Why does this return a new tree element each time? Shouldn't this return the
same one each time? If not, then this function needs a "create" prefix.


> +    createView: function(profile)
> +    {
> +	   return undefined;
> +    },
> +
> +    sidebarTreeElementForProfile: function(profile)
> +    {
> +	   return undefined;
> +    },

It is better to use null in cases like this. But why do these exist?


> +    viewForProfile: function(profile)
> +    {
> +	   if (!profile._profileView)
> +	       profile._profileView = this.createView(profile);
> +	   return profile._profileView;
>      }

I am confused why there are multiple versions of these functions. Also this
clearly calls "this.createView", which just returns undefined. Can you explain
this? I don't think the functions that just return "undefined" are needed or
useful.


> +    _parseKey: function(key)
> +    {
> +	   var match = key.match(/^([^\/]+)\/([^\/]+)$/);
> +	   if (match)
> +	       return [unescape(match[1]), unescape(match[2])];
> +	   return undefined;
> +    },

Better to return null.


> +    _viewForProfile: function(profile)
> +    {
> +	   for (var key in this._profilesIdMap) {
> +	       if (this._profilesIdMap[key] === profile) {
> +		   var parsedKey = this._parseKey(key);
> +		   if (parsedKey)
> +		       return
this.getProfileType(parsedKey[1]).viewForProfile(profile);
> +	       }
> +	   }
> +	   return undefined;
> +    },

Better to return null.


r-. This needs clerified and not have functions that do nothing but return
undefined. (Or explained with good comments.)