[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

Attachment 361972: Patch


--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 361972
  --> https://bugs.webkit.org/attachment.cgi?id=361972

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
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

    .then(resolve, reject);

> Source/WebInspectorUI/ChangeLog:10
> +	   the actual text of the payload รข it has an extra empty line at the

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 +

Ditto (>544).

More information about the webkit-reviews mailing list