[webkit-reviews] review granted: [Bug 193859] Web Inspector: Changes: style attribute changes aren't being tracked : [Attachment 364894] 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 193859: Web Inspector: Changes: style attribute changes aren't being
tracked
https://bugs.webkit.org/show_bug.cgi?id=193859

Attachment 364894: Patch

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




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

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

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:48
> +	   this._modifiedStyleDeclarations = new Map;

I'd drop the `Declaration`, as we almost always refer to them as a `style`, not
a `styleDeclaration`.

    this._modifiedStyles = new Map;

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:424
> +    get modifiedStyleDeclarations()

Ditto (>48).

    get modifiedStyles()

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:426
> +	   return Array.from(this._modifiedStyleDeclarations.values());

Ditto (>48).

    return Array.from(this._modifiedStyles.values());

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:429
> +    addModifiedStyleDeclaration(declaration)

Ditto (>48).

    addModifiedStyle(style)

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:431
> +	   this._modifiedStyleDeclarations.set(declaration.stringId,
declaration);

Ditto (>48).

   this._modifiedStyles.set(style.stringId, style);

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:532
> +	   this._modifiedStyleDeclarations.clear();

Ditto (>48).

    this._modifiedStyles.clear();

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:68
> +	   if (this._id)
> +	       return this._id.styleSheetId + "/" + this._id.ordinal;

We should always have a return for any function that can ever has a return
value, so either make the `this._id` check an assert, or add a default
`return`.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:383
> +	   WI.cssManager.addModifiedStyleDeclaration(this);

Ditto (>CSSManager.js48).

   WI.cssManager.addModifiedStyle(this);

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:84
> +	   let modifiedStyles = WI.cssManager.modifiedStyleDeclarations;

Ditto (>CSSManager.js48).

   let modifiedStyles = WI.cssManager.modifiedStyles;

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:94
> +			   return stylesForNode.matchedRules.some((matchedRule)
=> style.ownerRule.isEqualTo(matchedRule))

We should always have a return for any function that can ever has a return
value, so please add a base case `return false;`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:106
> +	   let declarationsForStylesheet = new Map();

Style: unnecessary parentheses.
NIT: the "s" of "sheet" should be capitalized, like
`declarationsForStyleSheet`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:113
> +	       let styleDeclarations =
declarationsForStylesheet.get(style.ownerStyleSheet);
> +	       if (!styleDeclarations) {
> +		   styleDeclarations = [];
> +		   declarationsForStylesheet.set(style.ownerStyleSheet,
styleDeclarations);
>	       }
> -	       cssRules.push(cssRule);
> +	       styleDeclarations.push(style);

You could use a `MultiMap` to do this work for you.  If you do, you'd need to
introduce a new iteration method, mabye something called
`MultiMap.prototype.sets`:

    sets()
    {
	return this._map[Symbol.iterator]();
    }

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:116
> +	   for (let [styleSheet, styles] of declarationsForStylesheet) {

which you could then call here, like `declarationsForStylesheet.sets()`.  The
current `MultiMap.prototype.[Symbol.iterator]()` iterates as `[key, value]`,
not `[key, valuesForKey]` as you'd like here.

Alternatively, there are no callers of the current
`MutliMap.prototype.[Symbol.iterator]()` so you could change it if you feel
that it would make more sense as `[key, valuesForKey]`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:123
> +	       let resourceHeaderContent;

Style: please initialize this to some value (e.g. `null`).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:124
> +	       if (styleSheet.isInlineStyleAttributeStyleSheet() && styles[0])

It should not be possible for `styles[0]` to be falsy.	You can get rid of that
check.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:129
> +	       resourceHeader.append(resourceHeaderContent);

I'd inline this, as it's simple/small/short enough that the code de-duplication
isn't that beneficial (if at all) considering the "length" of
`resourceHeaderContent` (I'm specifically referring to the variable name).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:132
> +	       for (let style of styles)
> +		   resourceSection.append(this._createRuleElement(style));

This is more of a stylistic preference/opinion, but I think we should add some
sort of border between each rule element, as otherwise they look quite cramped.
 I'm not sure how to best do that, but I think something should be done.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:145
> +	   selectorLineElement.className = "selector-line";
> +	   let selectorElement =
selectorLineElement.appendChild(document.createElement("span"));

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:190
> +	   if (styleSheet.isInlineStyleAttributeStyleSheet())
> +	       return WI.UIString("Style Attribute");

This is unnecessary, as you only call this in an else of
`styleSheet.isInlineStyleAttributeStyleSheet()`.