[Webkit-unassigned] [Bug 138812] Web Inspector: Ability to Copy entire CSS Rule from Styles Sidebar

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138812

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

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:108
> +    this._headerElement.addEventListener("contextmenu", function(event) {

Style: We would probably have a _handleContextMenuEvent method down below and here we would just have:

    this._headerElement.addEventListener("contextmenu", this._handleContextMenuEvent.bind(this));

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:110
> +        if (window.getSelection().toString().length)
> +            return;

What is this bail for?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:115
> +            InspectorFrontendHost.copyText(this._generateCSSRuleString.call(this));

Nit: this.foo.call(this) is no different than this.foo();

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:293
> +    _generateCSSRuleString: function() {

Style: Brace on new line.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:301
> +        function repeatText(text, numRepeats) {
> +            var output = "";
> +
> +            for (var i = 0; i < numRepeats; ++i)
> +                output += text;
> +
> +            return output;
> +        }

Actually, we support String.prototype.repeat. So you could just do:

    "test".repeat(3)

And get:

    "testtesttest"

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:312
> +                for (var i = numMediaQueries - 1; i >= 0; --i)
> +                    styleText += repeatText("    ", numMediaQueries - i - 1) + "@media " + mediaList[i].text + " {\n";

I wonder if this would be a good place to adopt template strings:

    styleText += `${"    ".repeat(numMediaQueries - i - 1)} @media ${mediaList[i].text} {\n`

But maybe I'm just getting too excited. Stick with just strings for now.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:325
> +            if (styleText.slice(-1) !== ";")
> +                styleText += ";";

I think: !styleText.endsWith(";") is more expressive.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150604/def48143/attachment-0001.html>