[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

Timothy Hatcher <timothy at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #241151|review?                     |review-
              Flags|                            |

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

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

Almost! Looking good.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:241
> +        var aIsResource = a instanceof WebInspector.ResourceTreeElement;
> +        var bIsResource = b instanceof WebInspector.ResourceTreeElement;
> +
> +        if (aIsResource && bIsResource)
> +            return WebInspector.ResourceTreeElement.compareResourceTreeElements(a, b);

It is odd that this generic class still knows about ResourceTreeElement. This compare method should move up to FrameTreeElement and only have a basic version in this baseclass that does the a.mainTitle.localeCompare(b.mainTitle);

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:62
> +    this.registerFolderizeSettings("flows", WebInspector.UIString("Flows"),
> +        function(representedObject) { return representedObject instanceof WebInspector.ContentFlow; },
> +        function() { return this.frame.domTree.flowsCount; }.bind(this),
> +        function(representedObject) { return new WebInspector.ContentFlowTreeElement(representedObject); }
> +    );

This can be simplified further. I think you only need one callback — countChildrenCallback.

validateRepresentedObjectCallback and createTreeElementCallback can be eliminated by passing in constructors instead. You can pass WebInspector.Frame or WebInspector.ContentFlow that FolderizedTreeElement uses for its own instanceof check. And you can pass WebInspector.FrameTreeElement or WebInspector.ContentFlowTreeElement what gets used with new by FolderizedTreeElement. Those constructors can be stored in your settings objects.

The only reason you would need functions is if the check or construction is not following the existing patterns.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:68
> +    function makeValidateCallback(resourceType) {
> +        return function(representedObject) {
> +            return representedObject instanceof WebInspector.Resource && representedObject.type === resourceType;
> +        };
> +    }

Sigh, this answers why you need a function now for validateRepresentedObjectCallback. Hmm.. Still createTreeElementCallback can be changes to just a constructor.

> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:46
> +    Object.keys(WebInspector.LayoutTimelineRecord.EventType).map(function(key) {
> +        var value = WebInspector.LayoutTimelineRecord.EventType[key];
> +        typeToLabelMap.set(value, WebInspector.LayoutTimelineRecord.displayNameForEventType(value));
> +    });

.map is not the right thing here since you don't use the result (it still creates a unused array with undefined).

This can be:

for (var key in WebInspector.LayoutTimelineRecord.EventType) {
   var value = WebInspector.LayoutTimelineRecord.EventType[key];
   typeToLabelMap.set(value, WebInspector.LayoutTimelineRecord.displayNameForEventType(value));
}

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:49
> +    var typeToLabelMap = new Map;
> +    Object.keys(WebInspector.Resource.Type).map(function(key) {
> +        var value = WebInspector.Resource.Type[key];
> +        typeToLabelMap.set(value, WebInspector.Resource.displayNameForType(value, true));
> +    });

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:81
> +    var keys = Object.keys(dictionary);
>      var scopeBarItems = keys.map(function(key) {

Could be one line. map is correct here since the map function returns an item and the rest of map is used.

-- 
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/20141107/e4037199/attachment-0002.html>