[webkit-reviews] review denied: [Bug 176643] Web Inspector: Styles Redesign: support toggling properties : [Attachment 321027] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 176643: Web Inspector: Styles Redesign: support toggling properties
https://bugs.webkit.org/show_bug.cgi?id=176643

Attachment 321027: Patch

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




--- Comment #14 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 321027
  --> https://bugs.webkit.org/attachment.cgi?id=321027
Patch

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

Mostly nits, but r- because of the issue with CSSProperty.prototype.commentOut.

> LayoutTests/inspector/unit-tests/string-utilities.html:60
> +	       InspectorTest.expectEqual("1\n2\n3".lineCount, 3, "a string with
two line breaks should have three lines");

Descriptions should all begin with a capital and end in a period.

> LayoutTests/inspector/unit-tests/string-utilities.html:63
> +	       return true;

The following cases would be good to have:

- Two consequtive line breaks
- Trailing line break

> LayoutTests/inspector/unit-tests/string-utilities.html:72
> +	       InspectorTest.expectEqual("".lastLine, "", "last line of an
empty string is empty string");

Same as above.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:123
> +	   this._enabled = !disabled;

Early return if `this._enabled` isn't changing. It should be safe to call this
repeatedly with the same `disabled` value, without thrashing `this._text`.

> Source/WebInspectorUI/UserInterface/Models/TextRange.js:114
> +	   console.assert(!isNaN(this._startLine), "TextRange needs line/column
data");

Assert description should end in a period.

> Source/WebInspectorUI/UserInterface/Models/TextRange.js:120
> +	   console.assert(!isNaN(this._startLine), "TextRange needs line/column
data");

Same.

>
Source/WebInspectorUI/UserInterface/Views/SyntaxHighlightingDefaultTheme.css:38
> +    color: var(--syntax-highlight-comment-color);

Nice.