[Webkit-unassigned] [Bug 138364] Web Inspector: decouple child element folderization logic from FrameTreeElement

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Timothy Hatcher <timothy at apple.com> ---
Comment on attachment 240983
  --> https://bugs.webkit.org/attachment.cgi?id=240983
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240983&action=review

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:32
> +    this._folderSettingsKey = "";

I worry that _folderSettingsKey will be used before the client sets it to something meaningful. Any reason folderSettingsKey can't be set in the constructor?

>> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:287
>> +            folderTreeElement._expandedSetting = new WebInspector.Setting(type + "-folder-expanded-" + this._folderSettingsKey, false);
> 
> Interesting!! Normally when we store a property on some other object like this we use double underscore prefix. I know you didn't add this but could you update it:
> 
>     folderTreeElement.__expandedSetting

Maybe add an assert at the top of this function that _folderSettingsKey is not empty. If it is empty, the client forgot to set folderSettingsKey.

>> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:74
>> +    );
> 
> Just a nit, but I think this might look better rearranging the functions:
> 
>    this.registerFolderizeSettings("frames", WebInspector.UIString("Frames"),
>         function(representedObject) { return representedObject instanceof WebInspector.Frame; },
>         function(representedObject) { return new WebInspector.FrameTreeElement(representedObject); },
>         function() { return this.frame.childFrames.length; }.bind(this)
>     );
> 
>     this.registerFolderizeSettings("flows", WebInspector.UIString("Flows"),
>         function(representedObject) { return representedObject instanceof WebInspector.ContentFlow; },
>         function(representedObject) { return new WebInspector.ContentFlowTreeElement(representedObject); },
>         function() { return this.frame.domTree.flowsCount; }.bind(this)
>     );
> 
> This is purely a style suggestion, you can ignore. But since the bodies are so short, this is nice and sweet.

I agree!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141105/94423b5f/attachment-0002.html>