[webkit-reviews] review denied: [Bug 173187] Web Inspector: Unify contextmenu items for all node links/previews : [Attachment 312501] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 173187: Web Inspector: Unify contextmenu items for all node links/previews
https://bugs.webkit.org/show_bug.cgi?id=173187

Attachment 312501: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 312501
  --> https://bugs.webkit.org/attachment.cgi?id=312501
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.