Bug 143111 – Web Inspector: Convert TreeElement classes to ES6

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

[Timothy Hatcher]

Convert TreeOutline and TreeElement to ES6 classes.

[Radar WebKit Bug Importer]

<rdar://problem/20313226>

[Timothy Hatcher]

Created attachment 249506 [details]
Patch

[Timothy Hatcher]

Created attachment 249507 [details]
Patch (Ignore Space)

[Timothy Hatcher]

Created attachment 249510 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 249507 [details]
Patch (Ignore Space)

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

r=me. but there is at least one issue here worth testing.

> Source/WebInspectorUI/UserInterface/Views/ApplicationCacheFrameTreeElement.js:58
> +WebInspector.ApplicationCacheFrameTreeElement.StyleClassName = "application-cache-frame";

Nit: inline!

> Source/WebInspectorUI/UserInterface/Views/ApplicationCacheManifestTreeElement.js:71
> +WebInspector.ApplicationCacheManifestTreeElement.StyleClassName = "application-cache-manifest";

Nit: inline!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:222
> -                console.error(e);
> +                console.error(e, e.stack);

The thing about including stack, is that we shouldn't need to do it once we improve our console.error output. But fine for now.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:287
> +    superExpandRecursively()
> +    {
> +        // FIXME: This is a hack to allow using super in a callback.
> +        super.expandRecursively(Number.MAX_VALUE);
> +    }

Sounds like you already came up with a better solution on IRC.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1364
> +WebInspector.DOMTreeElement.ForbiddenClosingTagElements = [
> +    "area", "base", "basefont", "br", "canvas", "col", "command", "embed", "frame",
> +    "hr", "img", "input", "isindex", "keygen", "link", "meta", "param", "source"
> +].keySet();

Looking at:
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element

For "No end tag" includes:

    "wbr", "track", and "menuitem"?

Which may be worth adding.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1369
> +WebInspector.DOMTreeElement.EditTagBlacklist = [
> +    "html", "head", "body"
> +].keySet();

keySet could just become new Set(...) now that we consume a list properly:

    WebInspector.DOMTreeElement.EditTagBlacklist = new Set(["html", "head", "body"]);

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:242
> +
> +
> +    populateContextMenu(contextMenu, event, treeElement)

Nit: Extra newline.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeSetIndexTreeElement.js:33
>          console.assert(object instanceof WebInspector.RemoteObject);
>  
> -    this._object = object;
> -
>          // Treat the same as an array-index just with different strings and widths.
> -    WebInspector.ObjectTreeBaseTreeElement.call(this, this._object, propertyPath);
> +        super(this._object, propertyPath);

BUG! This is using "this._object" which will be both a TDZ and a bug since "this._object" is undefined at this point.

Also, I've been moving the super calls above the console.assert calls. Making super as early as possible to avoid potential TDZs.

So my preference would be:

    super(object, propertyPath);

    console.assert(object instanceof WebInspector.RemoteObject);

    this._object = object;

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:732
> +            this._listItemNode.addEventListener("mousedown", WebInspector.TreeElement.treeElementMouseDown, false);
> +            this._listItemNode.addEventListener("click", WebInspector.TreeElement.treeElementToggled, false);
> +            this._listItemNode.addEventListener("dblclick", WebInspector.TreeElement.treeElementDoubleClicked, false);

Style: remove the falses?

> Source/WebInspectorUI/UserInterface/Views/TypePropertiesSection.js:-84
> -WebInspector.TypePropertiesSection.PropertyComparator = function(propertyA, propertyB)

I think there are still uses of this function, so I'd expect this to either be made static or perhaps we should just switch users over to ObjectTreeView version and make that hand this case.

[Timothy Hatcher]

Comment on attachment 249510 [details]
Patch

https://trac.webkit.org/r182042