Bug 164266

Summary: Web Inspector: Add the support for custom elements
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Web InspectorAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, cdumez, cmarcelo, commit-queue, dbates, esprehn+autocc, joepeck, kangil.han, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, sbarati, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 154907    
Attachments:
Description Flags
Adds the support
none
Addressed review comments
none
Patch for landing none

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
<rdar://problem/29038883>
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)
        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.
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.