[webkit-reviews] review granted: [Bug 195264] Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements : [Attachment 368625] 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 195264: Web Inspector: CSS Changes: modifications aren't shared for rules
that match multiple elements
https://bugs.webkit.org/show_bug.cgi?id=195264

Attachment 368625: Patch

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




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

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

r=me, please address the `Array.diffArrays` question before landing.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:504
> +    value(initialArray, currentArray, hashFunction, onEach)

NIT: I'd make `hashFunction` an "optional" and have a fallback implementation,
which is just an ident function.  This matches other uses of this pattern, like
`comparator`.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:523
> +	   function decrementUsageCount(map, key) {

Style: put this below the other function so that they're all nearby.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-531
> -		       ++deltaCurrent;

Why did this move?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:578
> +		   onEach(initialArray[i], -1);

I think you should always be doing this, not just after the final decrement. 
If I had three items left in this array, I'd expect to see three removals.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:589
> +		   onEach(current, 1);

Ditto (>578).

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:408
> +	   let cssProperty = new WI.CSSProperty(
> +	       this._index,
> +	       this._text,
> +	       this._name,
> +	       this._rawValue,
> +	       this._priority,
> +	       this._enabled,
> +	       this._overridden,
> +	       this._implicit,
> +	       this._anonymous,
> +	       this._valid,
> +	       this._styleSheetTextRange);

I hate this syntax T.T

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:59
> +    set initialState(state) { this._initialState = state; }

Why is this needed?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:166
> +	       propertyLineElement.classList.add("css-property-line",
className);
> +	       let stylePropertyView = new WI.SpreadsheetStyleProperty(null,
cssProperty, {readOnly: true});

Style: missing newline.
NIT: you should add a `const delegate = null;` so it's clearer.