[webkit-reviews] review denied: [Bug 174754] Web Inspector: add context menu item for taking a screenshot of a node : [Attachment 316190] 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 174754: Web Inspector: add context menu item for taking a screenshot of a
node
https://bugs.webkit.org/show_bug.cgi?id=174754

Attachment 316190: Patch

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




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 316190
  --> https://bugs.webkit.org/attachment.cgi?id=316190
Patch

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

r- for lack of test for the utility function. But also cause I wanted to see a
version 2.

Were we going to add a full page screenshot button in the navigation bar? Or is
that going to be done separately?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1131
> +Object.defineProperty(Number, "pad",
> +{
> +    value(num, length)
> +    {
> +	   let string = num.toLocaleString();
> +	   while (string.length < length)
> +	       string = "0" + string;
> +	   return string;
> +    },
> +});

String.prototype.padStart already exists and is standard. It can be used here
instead of the while loop.

How about making this Number.zeroPad / Number.prototype.zeroPad?

    let string = num.toLocaleString();
    return string.padStart(length, "0");

This should have a test in:
LayoutTests/inspector/unit-tests/number-utilities.html

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:468
> +	       function scrollIntoView()

Since we are moving this, lets fix the name of this inner function to be:

    function inspectedPage_node_scrollIntoView

That is because this function is evaluated in the inspected page and the `this`
value is the node.

We name these specially so (1) they are easy to find and (2) we are aware that
we have to be backwards compatible with older iOS releases for the contents of
these functions. We can't use `let` for instance (we support back to iOS 7).

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:203
> +    contextMenu.appendItem(WebInspector.UIString("Screenshot Node"), () => {

We should wrap this in:

    if (window.PageAgent) {
	...
    }

I don't think there exists a case where it is possible to have DOMNodes without
a PageAgent, but lets do it anyways to be safe.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:218
> +		   // url: "web-inspector:///Node.png",

Stale comment it seems.