[webkit-reviews] review denied: [Bug 193803] Web Inspector: Add Changes panel to Elements tab : [Attachment 360192] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 193803: Web Inspector: Add Changes panel to Elements tab
https://bugs.webkit.org/show_bug.cgi?id=193803

Attachment 360192: Patch

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




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

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

r-, as we should have tests for the diff logic

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:356
> +	   let cssRules = [];
> +	   this._modifiedCSSRules.forEach((value) => { cssRules.push(value) });
> +	   return cssRules;

NIT: this can be simpler

    get modifiedCSSRules()
    {
	return Array.from(this._modifiedCSSRules.values());
    }

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:361
> +	   let key = cssRule.id.styleSheetId + "/" + cssRule.id.ordinal;

NIT: since this is used in many places, I'd rather we create a `stringId`
function on `WI.CSSRule` so that it keeps the logic consistent everywhere.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:103
> +	   if (typeof this._text !== "undefined" && this._text !== text)

Why are we also checking that `typeof this._test !== "undefined"`?  Is that for
the case where we're creating a new property in the frontend and have yet to
add any text (e.g. empty)?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:132
> +	   this._markModified();

Wouldn't it be valid to just have this call inside
`this._updateStyleOwnerText`?  It seems like all these cases end up there, so
it may help centralize the code.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:441
> +	   if (!this._initialState) {

Can you inline this in `this._markModified`?

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:183
> +	   if (!this._initialState) {

Ditto (>CSSProperty.js:441).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:217
> +	   if (!this._enabledProperties)
> +	       this._enabledProperties = this._properties.filter((property) =>
property.enabled);

Nice!

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:392
> +    _saveInitialState()

Ditto (>CSSProperty.js:441).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:70
> +    }
> +    .changes-panel del {

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:45
> +	   if (this.didInitialLayout)
> +	       this.needsLayout();

This check shouldn't be necessary.  We should always be calling `layout` right
after we call `shown`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:53
> +    hidden()
> +    {
> +	   super.hidden();
> +    }

NIT: unnecessary

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:67
> +	   WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange,
this._mainResourceDidChange, this);

Please move the addition/removal of event listeners to `attached`/`detached`
functions.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:73
> +	   super.layout();
> +	   this.element.removeChildren();

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:77
> +	   this.element.classList.toggle("empty", cssRules.length === 0);

NIT: `!cssRules.length`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:84
> +	       let selectorElement = document.createElement("span");

NIT: `this.element.append(document.createElement("span"))`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:90
> +	       const indent = "  ";

NIT: we can have this match `WI.settings.indentUnit`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93
> +		   if (action === "equal" || action === "moved") {

Can you use an enum instead of hardcoded strings?

    WI.ChangesDetailsSidebarPanel.DiffAction = {
	Equal: "equal",
	Added: "added",
	Removed: "removed",
	Moved: "moved",
    };

Alternatively, you could have `action` be an integer instead (`0` for a
"neutral change", `-1` for deletion, `1` for addition).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:103
> +		       delElement.classList.add("css-property", "initial");

It seems like the "initial" and "current" classes aren't used in CSS. 
"css-property" is also somewhat redundant since the only things using it are
`<ins>` and `<del>` elements.  Perhaps we can remove them to remove duplicate
code.

    let tag = null;

    if (action === "added")
	tag = "ins";
    else if (action === "removed")
	tag  = "del";
    else
	tag = "span";

    let propertyElement = this.element.append(document.createElement(tag));
    propertyElement.append(indent, cssProprety.formattedText);

    this.element.append("\n");

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:139
> +    _diff(listA, listB, onEach)

This seems like a useful enough function to maybe be added to Utilities. 
Furthermore, it might deserve a comment (or better variable names) explaining
which of `listA` vs `listB` is what's considered "current" (e.g. what will be
diff'd against) and what's considered "initial" (e.g. what will bee diff'd
from).

We also 100% should have tests for this.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:151
> +	       if (i >= shortListLength)

This check seems redundant given that you're individually comparing each list
and their index on the next if.  Is there a reason to keep it?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:168
> +		       --i;
> +		       ++deltaB;

This logic could use an explanation.  I originally thought this would cause
skipping, but I after re-reading I see that you're resetting (not adding to)
`indexB` each iteration.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
css:139
> +    background-color: var(--background-color-selected) !important;

Can you add a `:not(.selected)` to a different rule instead of using an
`!important`?  :(