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

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 360274: Patch

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




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

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

r=me, nice test!

One thing I thought of is maybe we should add a "Show All"-style checkbox to
the top of the changes panel that shows only rules associated with the current
node (e.g. those in the Rules panel) when unchecked.  I could then check the
checkbox to see everything as it currently is now.  Being able to see
specifically what changed for a given node is quite helpful sometimes.

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

I'm still not sure why we need to check for `undefined`.  Isn't is easier to
just check `this._text === undefined`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:451
> +	       this._initialState.ownerStyle = this._ownerStyle.initialState;

NIT: should we be resetting this each time we call `_markModified`, or should
this only happen the first time we create `this._initialState`?

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:54
> +	   else

Style: remove the else.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:151
> +    get initialState()

NIT: simple getters like this can be one-lined at the beginning of the "Public"
section.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:70
> +		   [], // Passing CSS properties here would change their
ownerStyle.

Interesting.  This seems like a weird edge-case.  I'd imagine that we'd want
`set properties` to also have this effect, or neither of them.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:285
> +    get initialState()

Ditto (>CSSRule.js:151).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:87
> +	   if (WI.settings.indentWithTabs.value)
> +	       indent = "\t";
> +	   else
> +	       indent = " ".repeat(WI.settings.indentUnit.value);

You can just use `WI.indentString()` for this.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:105
> +		   if (action == 0) {

Style: should be `===`.

Also, you could use a `switch` here instead of a bunch of `if`s.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:111
> +

Style: extra newline.

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:35
> +

Style: I'd remove this newline.