[webkit-reviews] review granted: [Bug 194619] Web Inspector: Styles: valid values in style attributes are reported as unsupported property values : [Attachment 361972] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 13 17:53:07 PST 2019
Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 194619: Web Inspector: Styles: valid values in style attributes are
reported as unsupported property values
https://bugs.webkit.org/show_bug.cgi?id=194619
Attachment 361972: Patch
https://bugs.webkit.org/attachment.cgi?id=361972&action=review
--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 361972
--> https://bugs.webkit.org/attachment.cgi?id=361972
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361972&action=review
rs=me, please wait for EWS
> LayoutTests/inspector/css/modify-inline-style.html:7
> + let nodeStyles = null;
Style: missing newline after.
> LayoutTests/inspector/css/modify-inline-style.html:11
> + name: "AddPropertyAndEdit",
Please prefix the test case name with the test suite name.
name: "ModifyInlineStyle.AddPropertyAndEdit",
> LayoutTests/inspector/css/modify-inline-style.html:27
> + fontProperty.rawValue = "12px normal sans-serif!important";
Style: missing space before `!important`.
> LayoutTests/inspector/css/modify-inline-style.html:28
> + let colorProperty = null;
Style: missing newline before.
> LayoutTests/inspector/css/modify-inline-style.html:30
> + WI.CSSProperty.addEventListener(WI.CSSProperty.Event.Changed,
function(event) {
NIT: just in case, you should remove this listener when the test case
completes.
NIT: arrow function?
> LayoutTests/inspector/css/modify-inline-style.html:38
> +
fontProperty.awaitEvent(WI.CSSProperty.Event.Changed).then((event) => {
Please put the `.then(` on a new line.
> LayoutTests/inspector/css/modify-inline-style.html:56
> + }).then(() => {
> + resolve();
> + return true;
> + });
Rather than put `resolve()` inside a `.then(`, you can use it directly as the
callback.
.then(resolve, reject);
> Source/WebInspectorUI/ChangeLog:10
> + the actual text of the payload รข it has an extra empty line at the
end.
non-ascii character :(
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:539
> + if (payload.range) {
In order to avoid retyping `payload.range` everywhere, you should pull it out
into a variable (like all the other ones at the top of this function) and
include a fallback value (an empty object).
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:540
> + // Last property of inline style has mismatching range.
If this is only supposed to happen for inline styles, we should assert inside
the last `if` that `styleDeclaration.type ===
WI.CSSStyleDeclaration.Type.Inline`. If not, please remove this comment (or
make it more general).
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:544
> + let textLineCount = payload.text.lineCount;
You can just use `text`, since it has a fallback as well.
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:547
> + payload.range.endColumn = payload.range.startColumn +
payload.text.lastLine.length;
Ditto (>544).
More information about the webkit-reviews
mailing list