Bug 173187 – Web Inspector: Unify contextmenu items for all node links/previews

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Currently, the contextmenu items for interacting with nodes (e.g. Log Element) are only available inside the DOM tree.  Since these nodes are often linked/previewed elsewhere, these contextmenu items should also be visible too.

[Devin Rousso]

Created attachment 312497 [details]
Patch

[Devin Rousso]

Created attachment 312498 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 312501 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 312501 [details]
Patch

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

I really like this direction.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:130
> +        contextMenu.appendItem(WebInspector.UIString("Jump to Definition"), this._showCustomElementDefinition.bind(this));

This `this._showCustomElementDefinition` does not exist and would be an exception. You can make this an inline arrow function.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:148
> +    if (!options.excludeLogElement && !domNode.isInUserAgentShadowTree()) {

Nobody uses `excludeLogElement`, lets drop it.

Existing code only allows "Log Element" for Node.ELEMENT_NODE nodes but didn't for Node.TEXT_NODE and others. I think we probably want to continue that here and only do this if:

    domNode.nodeType() === Node.ELEMENT_NODE

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:182
>                  newDOMFragment.appendChild(inheritedLabel);

Lets move this after inheritedLabel is done getting setup or at the beginning at line 180.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:190
> +                const options = {
> +                    maxLength: 100,
> +                    excludeRevealElement: true,
> +                };
> +                inheritedLabel.appendChild(WebInspector.linkifyNodeReference(style.node, options));

Style: I think this would read much clearer as:

    label.appendChild(WebInspector.linkifyNodeReference(style.node, {
        maxLength: 100,
        excludeRevealElement: true,
    });

In fact any place that takes an options dictionary as the last parameter, I don't think we should have a temporary variable. I think thats something we should standardize on and break only in rare circumstances.

[Devin Rousso]

Comment on attachment 312501 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:130
>> +        contextMenu.appendItem(WebInspector.UIString("Jump to Definition"), this._showCustomElementDefinition.bind(this));
> 
> This `this._showCustomElementDefinition` does not exist and would be an exception. You can make this an inline arrow function.

Oops.  Forgot to change this from copying.  Thanks =D

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:148
>> +    if (!options.excludeLogElement && !domNode.isInUserAgentShadowTree()) {
> 
> Nobody uses `excludeLogElement`, lets drop it.
> 
> Existing code only allows "Log Element" for Node.ELEMENT_NODE nodes but didn't for Node.TEXT_NODE and others. I think we probably want to continue that here and only do this if:
> 
>     domNode.nodeType() === Node.ELEMENT_NODE

I will be using this in <https://webkit.org/b/138941>.

I think that we should add support for logging other types of nodes, seeing as they are completely valid to retrieve simply by logging the parent element and getting it via the parent's children.  In these cases, however, the text should probably say "Log Node".

>> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:190
>> +                inheritedLabel.appendChild(WebInspector.linkifyNodeReference(style.node, options));
> 
> Style: I think this would read much clearer as:
> 
>     label.appendChild(WebInspector.linkifyNodeReference(style.node, {
>         maxLength: 100,
>         excludeRevealElement: true,
>     });
> 
> In fact any place that takes an options dictionary as the last parameter, I don't think we should have a temporary variable. I think thats something we should standardize on and break only in rare circumstances.

I've heard two different things regarding this style (the other being from Brian), and have been following the format of using const local variables.  I'm happy to go either way, but I think we should decide on a rule and stick to it.  Some examples:

<https://webkit.org/b/168021> Web Inspector: Prefer Resources tab over Network tab when showing files
<https://webkit.org/b/168709> Web Inspector: Don't show the Search tab if it's open and matches the representedObject

[Devin Rousso]

Created attachment 312565 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 312565 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:148
> +                const options = {
> +                    ignoreNetworkTab: true,
> +                    ignoreSearchTab: true,
> +                };
> +                WebInspector.showSourceCodeLocation(sourceCodeLocation, options);

I'd inline this object.

> Source/WebInspectorUI/UserInterface/Views/Main.css:343
> +    -webkit-user-select: none;

What if I want to select the selector in the sidebar or something?

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:189
> +                const options = {
> +                    maxLength: 100,
> +                    excludeRevealElement: true,
> +                };
> +                inheritedLabel.appendChild(WebInspector.linkifyNodeReference(style.node, options));

Again, I'd inline this.

[Devin Rousso]

Created attachment 313327 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313327 [details]
Patch

Rejecting attachment 313327 [details] from commit-queue.

New failing tests:
inspector/canvas/create-canvas-contexts.html
Full output: http://webkit-queues.webkit.org/results/3962111

[WebKit Commit Bot]

Created attachment 313330 [details]
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 313337 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313337 [details]
Patch

Clearing flags on attachment: 313337

Committed r218521: <http://trac.webkit.org/changeset/218521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.