Bug 138364 – Web Inspector: decouple child element folderization logic from FrameTreeElement

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

The logic in FrameTreeElement to organize child elements into folders should be broken out into a new base class, FolderizedTreeElement. This will reduce the responsibilities of FrameTreeElement, and allow other tree item types to group children into folders.

[Radar WebKit Bug Importer]

<rdar://problem/18870277>

[Matt Baker]

Created attachment 240983 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 240983 [details]
Patch

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

Great! Nice and clean too!

A lot of my review comments about the old code you didn't change, lets use this opportunity to fix it up a bit.

There is at least one question I raised that I'd like to see a response for, but if it checks out feel free to land.

> Source/WebInspectorUI/ChangeLog:80
> +        * UserInterface/Main.html:
> +        * UserInterface/Views/FolderizedTreeElement.js: Added.

Nit: Looks like this ChangeLog has everything duplicated.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:100
> +            console.error("No settings for represented object: " + representedObject);

Nit: Simpler to just do:

    console.error("Error message", obj);

Which will actually include the entire representedObject instead of stringifying it to something like [Object]/

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:190
> +        for (var child of this._newChildQueue)
> +            this.addChildForRepresentedObject(child);

This change worries me just a bit, the change is going from the old loop, to the new loop.

Before:

     for (var i = 0; i < this._newChildQueue.length; ++i)
         this._addChildForRepresentedObject(this._newChildQueue[i]);

After:

     for (var child of this._newChildQueue)
         this.addChildForRepresentedObject(child);

I recall during a debugging session previously that during reload of a page we saw something like:

    - _addChildForRepresentedObject loops over child queue
      -> appends the first tree element
        -> caused onpopulate
          -> onpopulate clears existing state (this.removeChildren(), this._clearNewChildQueue())
          -> onpopulate then goes through its model objects and adds all children
      -> back out again to continue to loop over child queue, breaks immediately cause i > cleared queue length of 0

Do we still do something like this with the new for loop? It looks to me like it wouldn't:

    > arr = [1,2,3]; for (var i = 0; i < arr.length; ++i) { console.log(arr[i]); arr = []; } // before
    [Log] 1

    > arr = [1,2,3]; for (var i of arr) { console.log(i); arr = []; } // after
    [Log] 1
    [Log] 2
    [Log] 3

For example, when reloading / opening the inspector for bogojoker.com are there any unexpected duplicated resources?

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:232
> +    _compareChildTreeElements: function (a, b)

Style: "function (" => "function("

> 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

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:297
> +            console.error("Unknown representedObject: ", representedObject);

Nit: update this as well. No need for the ": ".

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:304
> +        if (!this._folderTypeMap.has(settings.type))
> +            this._folderTypeMap.set(settings.type, createFolderTreeElement.call(this, settings.type, settings.folderDisplayName));
> +
> +        return this._folderTypeMap.get(settings.type)

Nit: missing semicolon
Nit: If we create, we can return the folder immediately without having to do a map lookup. Maybe this would be better as:

    var folder = this._folderTypeMap.get(settings.type);
    if (folder)
        return folder;

    var folder = createFolderTreeElement.call(this, settings.type, settings.folderDisplayName);
    this._folderTypeMap.set(folder);
    return folder;

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:74
> +
> +    this.registerFolderizeSettings("frames", WebInspector.UIString("Frames"),
> +        function(representedObject) {
> +            return representedObject instanceof WebInspector.Frame;
> +        },
> +        function() {
> +            return this.frame.childFrames.length;
> +        }.bind(this),
> +        function(representedObject) {
> +            return new WebInspector.FrameTreeElement(representedObject);
> +        }
> +    );
> +
> +    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);
> +        }
> +    );

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.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:84
> +        // There are some other properties on WebInspector.Resource.Type that we need to skip, like private data and functions
> +        if (type.charAt(0) === "_")
> +            continue;
> +
> +        // Only care about the values that are strings, not functions, etc.
> +        var typeValue = WebInspector.Resource.Type[type];
> +        if (typeof typeValue !== "string")
> +            continue;

> Object.keys(WebInspector.Resource.Type)
< ["Document", "Stylesheet", "Image", "Font", "Script", "XHR", "WebSocket", "Other", "_mimeTypeMap", "fromMIMEType", "displayName"]

We should really have a pure enum type, and just move those 3 functions elsewhere. These sound reasonable:

WebInspector.Resource.Type._mimeTypeMap => WebInspector.Resource.Type.MimeTypeMap
WebInspector.Resource.Type.fromMIMEType => WebInspector.Resource.typeFromMIMEType
WebInspector.Resource.Type.displayName => WebInspector.Resource.Type.displayNameForType

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:86
> +        function makeValidateCallback(resourceType) {

Move these two "make*" functions outside of the for loop.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:316
> +    _canvasWasAdded: function(event)
>      {
> -        this._addRepresentedObjectToNewChildQueue(event.data.flow);
> +        var canvas = event.data.canvas;
> +        if (canvas.parentFrame == this._frame)
> +            this.addRepresentedObjectToNewChildQueue(canvas);
>      },

Oops, this seems like a `git add -p` slip. The _canvasWasAdded/Removed stuff would be for a later patch.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:335
> +    _rootDOMNodeInvalidated: function() {

Style: Opening brace on its own line.

[Timothy Hatcher]

Comment on attachment 240983 [details]
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!

[Matt Baker]

Comment on attachment 240983 [details]
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?

Making folderSettingsKey a constructor argument would be awkward due to the inheritance chain: FolderizedTreeElement <- SourceCodeTreeElement <- ResourceTreeElement <- FrameTreeElement. Since it isn't used until child items are added, I don't think there is a risk of it being used before it is initialized.

[Matt Baker]

Created attachment 241151 [details]
Patch

[Timothy Hatcher]

Comment on attachment 241151 [details]
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.

[Matt Baker]

Created attachment 241214 [details]
Patch

[Timothy Hatcher]

Comment on attachment 241214 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:43
> +    for (var key of Object.keys(WebInspector.LayoutTimelineRecord.EventType)) {

for (var key in WebInspector.LayoutTimelineRecord.EventType) does the same thing and does not make an intermediate array.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:46
> +    for (var key of Object.keys(WebInspector.Resource.Type)) {

Ditto.

[Matt Baker]

Created attachment 241218 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241218 [details]
Patch

Clearing flags on attachment: 241218

Committed r175784: <http://trac.webkit.org/changeset/175784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.