Bug 143111 - Web Inspector: Convert TreeElement classes to ES6
Summary: Web Inspector: Convert TreeElement classes to ES6
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks: 142891
  Show dependency treegraph
 
Reported: 2015-03-26 12:13 PDT by Timothy Hatcher
Modified: 2015-04-21 20:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (282.81 KB, patch)
2015-03-26 12:16 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (Ignore Space) (197.98 KB, patch)
2015-03-26 12:17 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (282.70 KB, patch)
2015-03-26 13:10 PDT, Timothy Hatcher
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-03-26 12:13:18 PDT
Convert TreeOutline and TreeElement to ES6 classes.
Comment 1 Radar WebKit Bug Importer 2015-03-26 12:13:38 PDT
<rdar://problem/20313226>
Comment 2 Timothy Hatcher 2015-03-26 12:16:18 PDT
Created attachment 249506 [details]
Patch
Comment 3 Timothy Hatcher 2015-03-26 12:17:08 PDT
Created attachment 249507 [details]
Patch (Ignore Space)
Comment 4 Timothy Hatcher 2015-03-26 13:10:54 PDT
Created attachment 249510 [details]
Patch
Comment 5 Joseph Pecoraro 2015-03-26 15:06:01 PDT
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.
Comment 6 Timothy Hatcher 2015-03-26 16:40:31 PDT
Comment on attachment 249510 [details]
Patch

https://trac.webkit.org/r182042