Bug 164266 – Web Inspector: Add the support for custom elements

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Show the custom element state in the DOM node details pane and add the context menu item to jump to the custom element definition.

[Radar WebKit Bug Importer]

<rdar://problem/29038883>

[Ryosuke Niwa]

Created attachment 293530 [details]
Adds the support

[Joseph Pecoraro]

Comment on attachment 293530 [details]
Adds the support

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

r- because it lacks a test for the protocol change. Comments below will guide you through an example for that, but otherwise this is great!

> Source/WebCore/ChangeLog:9
> +        Set "customElementState" property on each DOMNode object when building an protocol object for node.

Typo: "an protocol" => "a protocol"
Typo: "for node" => "for the node".

> Source/JavaScriptCore/inspector/protocol/DOM.json:61
>                  { "name": "shadowRootType", "$ref": "ShadowRootType", "optional": true, "description": "Shadow root type." },
> +                { "name": "customElementState", "$ref": "CustomElementState", "optional": true, "description": "Custom element state." },

We can and should test this. Look at LayoutTests/inspector/dom/shadowRootType.html for an example. It should end up really similar to that. Let me know if you have any questions!

> Source/JavaScriptCore/inspector/protocol/DOM.json:559
> +                { "name": "elementId", "$ref": "NodeId", "description": "Element id." },
> +                { "name": "customElementState", "$ref": "CustomElementState", "description": "Custom element state." }

Style: "elementId" => "nodeId". You can keep the description to say this must be an element, but otherwise have it match the type.
You can drop the descriptions if you want, because here they just match the types which have the same description.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:87
> +            this._customElementState = payload.customElementState || WebInspector.DOMNode.CustomElementState.BuiltIn;

Nit: This should be "Builtin" not "BuiltIn"

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:273
> +        return this._customElementState == WebInspector.DOMNode.CustomElementState.Custom;

Style: ===

> Source/WebInspectorUI/UserInterface/Protocol/DOMObserver.js:88
> +    customElementStateChanged(elementId, customElementState)
> +    {
> +        WebInspector.domTreeManager._customElementStateChanged(elementId, customElementState);
> +    }

Style: elementId => nodeId (throughout the frontend). Although element is clearer, we use nodeId more canonically.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:32
> +        WebInspector.domTreeManager.addEventListener(WebInspector.DOMTreeManager.Event.CustomElementStateChanged, this._customElementStateChanged, this);

Style: Put this one at the bottom of this list (after CharacterDataModified). It more closely matches the order the Events are defined in DOMTreeManager.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:759
> +                WebInspector.RemoteObject.resolveNode(node, "", (remoteObject) => {
> +                    remoteObject.getProperty("constructor", (error, result, wasThrown) => {
> +                        if (error) {
> +                            console.error(error);
> +                            return;
> +                        }
> +
> +                        DebuggerAgent.getFunctionDetails(result.objectId, (error, response) => {
> +                            if (error) {
> +                                console.error(error);
> +                                return;
> +                            }

There should be an if (!remoteObject) return at the top level. Since removeNode can fail if the node was deleted on the page between when you right clicked it and did "Jump to Definition".

We should be calling remoteObject.release() after the getProperty call. See other users of WebInspector.RemoteObject.resolveNode. This is necessary to remove the inspector's strong reference to the node that we created here.
We should be calling result.release() after getFunctionDetails as well to remove the inspector strong reference to the function that we created here.

We should remove the console.errors now.

[Ryosuke Niwa]

Created attachment 293532 [details]
Addressed review comments

[Joseph Pecoraro]

Comment on attachment 293532 [details]
Addressed review comments

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

Awesome! r=me with some cleanup suggestions for the test

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1404
> +        value->setCustomElementState(customElementState(element));

We can avoid setting this in the default case (Builtin) to slightly reduce the JSON message size for nodes in the common case:

    Inspector::Protocol::DOM::CustomElementState customElementState = customElementState(element);
    if (customElementState != Inspector::Protocol::DOM::CustomElementState::Builtin)
        value->setCustomElementState(customElementState);

The frontend defaults to Builtin when customElementState is missing.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:746
> +

Style: Remove this empty line now that you've nicely moved this to a function!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1504
> +                if (error || result.type != "function")

Style: !==

> LayoutTests/inspector/dom/customElementState.html:10
> +    let suite = InspectorTest.createAsyncSuite("DOMNode.shadowRootType");

Typo: DOMNode.customElementState

> LayoutTests/inspector/dom/customElementState.html:14
> +        test: (resolve, reject) => {

Style: "test: (resolve, reject) =>" => "test(resolve, reject) {" for all of these.

> LayoutTests/inspector/dom/customElementState.html:17
> +                InspectorTest.expectThat(node.customElementState(), "builtin");

This is not testing what you expect. This is testing that node.customElementState() is truthy, and producing the message "builtin". We should use the newer InspectorTest.expectEqual and provide a message.

    InspectorTest.expectEqual(node.customElementState(), WebInspector.DOMNode.CustomElementState.Builtin, "#builtin should be CustomElementState.Builtin.");
    InspectorTest.expectEqual(node.customElementState(), WebInspector.DOMNode.CustomElementState.Custom, "constructed-element should be CustomElementState.Custom.")
    ...

The last argument (the message) should be a complete sentence of the form "Foo should be bar." so that it reads nicely in the expected results.

> LayoutTests/inspector/dom/customElementState.html:73
> +customElements.define('constructed-element', class extends HTMLElement { });

Style: Double quote strings when possible. (Normally LayoutTests is a style wild-west, but we try to be pretty consistent in LayoutTests/inspector).

Lets move this inside the <body> but before <div style="display:none"> since it is easy to miss here.

> LayoutTests/inspector/dom/customElementState.html:78
> +<p>Test for DOMNode.shadowRootType.</p>

Typo: DOMNode.customElementState

> LayoutTests/inspector/dom/customElementState.html:92
> +window.onerror = function (message) {
> +    if (message == 'failedElementError')
> +        return true;
> +    return false;
> +}

I think this can just be a TestPage.allowUncaught something or other.

> LayoutTests/inspector/dom/customElementState.html:94
> +customElements.define('upgraded-element', class extends HTMLElement { });
> +customElements.define('failed-element', class extends HTMLElement { constructor() { super(); throw 'failedElementError'; } });

Style: Double quote strings when possible.

[Ryosuke Niwa]

Created attachment 293535 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293535 [details]
Patch for landing

Clearing flags on attachment: 293535

Committed r208218: <http://trac.webkit.org/changeset/208218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.