[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