[Webkit-unassigned] [Bug 138364] Web Inspector: decouple child element folderization logic from FrameTreeElement
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 4 18:19:24 PST 2014
https://bugs.webkit.org/show_bug.cgi?id=138364
Joseph Pecoraro <joepeck at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #240983|review? |review+
Flags| |
--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
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
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.
--
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/03dc3be2/attachment-0002.html>
More information about the webkit-unassigned
mailing list