[webkit-reviews] review granted: [Bug 193859] Web Inspector: Changes: style attribute changes aren't being tracked : [Attachment 364894] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 15 23:31:19 PDT 2019
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()`.
More information about the webkit-reviews
mailing list