[webkit-reviews] review granted: [Bug 170184] Web Inspector: tabbing in Styles sidebar is broken when additional ":" and "; " are in the property value : [Attachment 306562] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 170184: Web Inspector: tabbing in Styles sidebar is broken when additional
":" and ";" are in the property value
https://bugs.webkit.org/show_bug.cgi?id=170184

Attachment 306562: Patch

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




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

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

r=me, nice tests!

>
Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:408
> +    _rangeForNextNameOrValue(codeMirror, cursor, text) {

Nit: put open brace on its own line.

> LayoutTests/inspector/unit-tests/text-utilities-expected.txt:21
> +PASS: Next highlight range of "foo: url(http://baz);" starting at index 5 is
[5, 20]

A couple thoughts on the test output:

1) Highlight is a higher-level UI concept, and doesn't appear in the name of
the utility method. It's confusing unless you know how this is going to be used
in the UI. Maybe use name/value instead.
2) It is hard to tell from the resulting range bounds alone where an error
occurred. What do you think about including the resulting substring in the
output? Something like:

	Next name/value token in "foo: url(http://baz);" starting at index 5 is
"url(http://baz)" (5, 20)

> LayoutTests/inspector/unit-tests/text-utilities.html:14
> +		  
InspectorTest.expectShallowEqual(WebInspector.rangeForNextCSSNameOrValue(text,
index), expected, `Next highlight range of "${text}" starting at index ${index}
is [${expected.from}, ${expected.to}]`);

Style: I think this would read a little better as:

function testValid(text, index, expected) {
    let actual = WebInspector.rangeForNextCSSNameOrValue(text, index);
    InspectorTest.expectShallowEqual(actual, expected, ...);
}