Bug 164266 - Web Inspector: Add the support for custom elements
Summary: Web Inspector: Add the support for custom elements
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
Reported: 2016-10-31 22:16 PDT by Ryosuke Niwa
Modified: 2016-11-01 01:15 PDT (History)
16 users (show)

See Also:

Adds the support (28.59 KB, patch)
2016-10-31 22:44 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed review comments (33.74 KB, patch)
2016-10-31 23:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (34.45 KB, patch)
2016-11-01 00:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-10-31 22:16:42 PDT
Show the custom element state in the DOM node details pane and add the context menu item to jump to the custom element definition.
Comment 1 Radar WebKit Bug Importer 2016-10-31 22:17:01 PDT
Comment 2 Ryosuke Niwa 2016-10-31 22:44:08 PDT
Created attachment 293530 [details]
Adds the support
Comment 3 Joseph Pecoraro 2016-10-31 23:10:11 PDT
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.
Comment 4 Ryosuke Niwa 2016-10-31 23:48:50 PDT
Created attachment 293532 [details]
Addressed review comments
Comment 5 Joseph Pecoraro 2016-11-01 00:13:59 PDT
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)

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.
Comment 6 Ryosuke Niwa 2016-11-01 00:38:58 PDT
Created attachment 293535 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2016-11-01 01:15:13 PDT
Comment on attachment 293535 [details]
Patch for landing

Clearing flags on attachment: 293535

Committed r208218: <http://trac.webkit.org/changeset/208218>
Comment 8 WebKit Commit Bot 2016-11-01 01:15:19 PDT
All reviewed patches have been landed.  Closing bug.