Bug 141397 – Web Inspector: Inspecting a page where resources are in folders forces folder organization on every subsequent page

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Jonathan Wells]

Normally opening the inspector on a page with few resources will avoid putting those resources into folders (Stylesheets, Scripts, etc). Pages with many resources will have their resources organized by folder when inspected. However once the inspector uses folders for resources, navigating to any other page, even one with few resources, will folderize the resources even though it shouldn't.

[Radar WebKit Bug Importer]

<rdar://problem/19770326>

[Jonathan Wells]

Offline diagnosis from Joe Peck:

    FrameTreeElement.prototype._mainResourceDidChange should flip this._groupedIntoFolders in FolderizedTreeElement.js

[Jonathan Wells]

Created attachment 246288 [details]
[PATCH] Fix.

[Joseph Pecoraro]

Comment on attachment 246288 [details]
[PATCH] Fix.

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

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:291
> +        this._groupedIntoFolders = false;

We don't want to reach into the superclass' member variables here.

Actually, now that I see this code, it seems "FolderizedTreeElement.prototype.removeChildren" would be appropriate to reset this flag. removeChildren is called here, and if that really does take us to 0 children, it should reset this state.

[Jonathan Wells]

(In reply to comment #4)
> Comment on attachment 246288 [details]
> [PATCH] Fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246288&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:291
> > +        this._groupedIntoFolders = false;
> 
> We don't want to reach into the superclass' member variables here.
> 
> Actually, now that I see this code, it seems
> "FolderizedTreeElement.prototype.removeChildren" would be appropriate to
> reset this flag. removeChildren is called here, and if that really does take
> us to 0 children, it should reset this state.

This actually leads to:
[Error] RangeError: Maximum call stack size exceeded.
	removeChildren (FolderizedTreeElement.js, line 83)
	onpopulate (FrameTreeElement.js, line 235)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	...

Also:

[Error] RangeError: Maximum call stack size exceeded.
	(anonymous function) (FolderizedTreeElement.js, line 166)
	updateParentStatus (FolderizedTreeElement.js, line 166)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 106)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
	addChildForRepresentedObject (FolderizedTreeElement.js, line 117)
	onpopulate (FrameTreeElement.js, line 238)
	expand (TreeOutline.js, line 844)
	shouldRefreshChildren (TreeOutline.js, line 636)
        ...

I think this is perhaps right but it may need to do more than just set that flag.

[Jonathan Wells]

FolderizedTreeElement#_shouldGroupIntoFolders returns true when it shouldn't after a navigation in the main frame. Working to fix.

[Jonathan Wells]

Created attachment 246401 [details]
[PATCH] attempted fix 2.

I noticed that _shouldGroupIntoFolders() returns true permanently after it returns true one time. I changed this, and set this._groupedIntoFolders to false whenever _shouldGroupIntoFolders suddenly returns false. This might not be terribly performant, and I'm up for ideas on how to make this better.

[Jonathan Wells]

Created attachment 246419 [details]
[PATCH] attempted fix 2a.

Oops. Left a debug thing in there. Removed.

[Timothy Hatcher]

Comment on attachment 246419 [details]
[PATCH] attempted fix 2a.

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

On the right track.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:93
> +        if (this._groupedIntoFolders && !this._shouldGroupIntoFolders())
> +            this._groupedIntoFolders = false;

I think you should do this unconditionally. _shouldGroupIntoFolders() will never return true if all children were removed.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:-315
> -        // Already grouped into folders, keep it that way.
> -        if (this._groupedIntoFolders)
> -            return true;

The point of this code was to prevent jumping between folders and no-folders if the page adds then removes resources, then adds more back. Clearing the flag unconditionally in removeChildren is good, but I think this early return should stay.

[Jonathan Wells]

(In reply to comment #9)
> Comment on attachment 246419 [details]
> [PATCH] attempted fix 2a.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246419&action=review
> 
> On the right track.
> 
> > Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:93
> > +        if (this._groupedIntoFolders && !this._shouldGroupIntoFolders())
> > +            this._groupedIntoFolders = false;
> 
> I think you should do this unconditionally. _shouldGroupIntoFolders() will
> never return true if all children were removed.
> 

Ok I just figured out how I can do that without creating an infinite function call cycle. Patch coming.

[Jonathan Wells]

Created attachment 246474 [details]
[PATCH] Attempted fix 3.

This adds a check to prevent setting shouldRefreshChildren unnecessarily, which avoids infinite recursion.

[Timothy Hatcher]

Comment on attachment 246474 [details]
[PATCH] Attempted fix 3.

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

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:118
> +            if (this.children.length)
> +                this.shouldRefreshChildren = true;

I am not sure this is correct. If this.children.length is 0, the frame could still needed refreshed so on populate will be called.

[Jonathan Wells]

Created attachment 246704 [details]
[PATCH] attempted fix 4.

Upon Tim's suggestion, I've prevented this.shouldRefreshChildren from being set back to true while onpopulate takes place.

[Timothy Hatcher]

Comment on attachment 246704 [details]
[PATCH] attempted fix 4.

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

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:108
> +        if (!this.treeOutline && !this._isPopulating) {

We typically don't put "is" in a boolean property. Just _populating or _populatingChildElements.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:119
>          if (!this._groupedIntoFolders && this._shouldGroupIntoFolders()) {
>              // Mark as needing a refresh to rebuild the tree into folders.
>              this._groupedIntoFolders = true;
> -            this.shouldRefreshChildren = true;
> +            if (!this._isPopulating)
> +                this.shouldRefreshChildren = true;
>              return;

I'm not sure _groupedIntoFolders should change while _isPopulating, so the guard should be a level up. (Even thought I doubt this will change once onpopulate starts.)

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:233
> +        this._isPopulating = true;

This should not use a private property. It needs to be public or use a public getter and setter to be used across classes.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:256
> +        this._isPopulating = false;

Ditto.

[Jonathan Wells]

Comment on attachment 246704 [details]
[PATCH] attempted fix 4.

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

>> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:119
>>              return;
> 
> I'm not sure _groupedIntoFolders should change while _isPopulating, so the guard should be a level up. (Even thought I doubt this will change once onpopulate starts.)

We actually need to set _groupedIntoFolders to true here or else it seems based on testing that folderization never occurs.

[Jonathan Wells]

Created attachment 246707 [details]
[PATCH] Attempted fix 5.

[Jonathan Wells]

Created attachment 246786 [details]
[PATCH] Attempted fix 6.

Based on online discussions, this might be the better-organized approach. Tested to work with source maps, sites with many resources, and sites with few resources.

[Timothy Hatcher]

Comment on attachment 246786 [details]
[PATCH] Attempted fix 6.

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

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:113
> +        if (!this.populating) {
> +            if (!this.treeOutline) {
> +                // Just mark as needing to update to avoid doing work that might not be needed.
> +                this.shouldRefreshChildren = true;
> +                return;
> +            }
> +            this._switchToFolderGroupsIfNecessary();
>          }

I don't see any calls to addChildForRepresentedObject other than the two places that now call prepareToPopulate. You should be able to remove this code, this.populating and _switchToFolderGroupsIfNecessary.

> Source/WebInspectorUI/UserInterface/Views/FolderizedTreeElement.js:173
> +        if (this._shouldSwitchToFolderGroups())

If you remove _switchToFolderGroupsIfNecessary you can move _shouldSwitchToFolderGroups into here and get rid of _shouldSwitchToFolderGroups too.

[Jonathan Wells]

Committed r180319: <http://trac.webkit.org/changeset/180319>