[webkit-reviews] review granted: [Bug 178328] Web Inspector: [PARITY] Styles Redesign: Ability to modify style attributes : [Attachment 325217] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 30 11:49:54 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178328: Web Inspector: [PARITY] Styles Redesign: Ability to modify style
attributes
https://bugs.webkit.org/show_bug.cgi?id=178328

Attachment 325217: Patch

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




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

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

> LayoutTests/inspector/unit-tests/text-range.html:7
> +    let suite = InspectorTest.createAsyncSuite("WI.TextRange");

This should be a createSyncSuite. No need for async in this test.

> LayoutTests/inspector/unit-tests/text-range.html:12
> +	   test(resolve, reject) {

This would just be `test() {` in a Sync suite.

> LayoutTests/inspector/unit-tests/text-range.html:25
> +	       resolve();

This would turn into `return true` in a Sync suite.

> LayoutTests/inspector/unit-tests/text-range.html:47
> +    });

What are the expected values when the text ends in multiple line breaks?

    let text = "\ncolor: blue;\n\n"

We may want to add a test for that, since the CSSProperty.js change looks like
it could make that happen.

> LayoutTests/inspector/unit-tests/text-range.html:54
> +    <p>Testing that WI.TextRange.prototype.resolveOffsets works.</p>

Nit: The test description should be for all of TextRange. We only happen to be
testing resolveOffsets but we could test anything in TestRange in this
generically named test.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:352
> +	   range.resolveOffsets(styleText + "\n");

Should we only append a newline if it didn't end in a newline, otherwise we
could have multiple newlines. Maybe it doesn't matter.


More information about the webkit-reviews mailing list