[webkit-reviews] review denied: [Bug 175343] Web Inspector: Styles Redesign: hook up real data to spreadsheet style editor : [Attachment 319531] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 175343: Web Inspector: Styles Redesign: hook up real data to spreadsheet
style editor
https://bugs.webkit.org/show_bug.cgi?id=175343

Attachment 319531: Patch

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




--- Comment #14 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 319531
  --> https://bugs.webkit.org/attachment.cgi?id=319531
Patch

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

I think it's very close to being good to land.	I'd like to see one more
revision.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:51
> +	   super.layout();

Is there a reason that this is at the end?  I would expect setup style
functions to have super as the first line.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:54
> +    renderProperty(cssProperty)

NIT: This could be private.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:75
> +    set delegate(delegate) { this._delegate = delegate || null; }

NIT: this isn't used.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:100
> +	   else

Style: since the `if` returns, this `else` isn't needed.

    if (this._style.styleSheetTextRange)
	return this._style.visibleProperties;
    return this._style.properties;

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.css:37
> +    float: right;

Is there no way around using floats?  They tend to just cause so much havoc...
:(

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:44
> +    get element() { return this._element; }

This getter already exists on WI.View.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:82
> +	   super.initialLayout();

See comment on SpreadsheetCSSStyleDeclarationEditor.js:51

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:103
> +		   let tooltip = WI.UIString("Specificity: (%d, %d,
%d)").format(specificity[0], specificity[1], specificity[2]);

NIT: you can use a spread here:

    let tooltip = WI.UIString("Specificity: (%d, %d,
%d)").format(...specificity);

Also, do we want to call toLocaleString() on these numbers?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:115
> +		   let tooltip = WI.UIString("Specificity: No value for
selected element");
> +		   tooltip += "\n";
> +		   tooltip += WI.UIString("Dynamically calculated for the
selected element and did not match");

I think we usually just put the \n and second line in the same UIString.

    selectorElement.title = WI.UIString("Specificity: No value for selected
element\nDynamically calculated for the selected element and did not match");

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:126
> +	   let appendSelectorTextKnownToMatch = (selectorText) => {
> +	       let selectorElement =
this._selectorElement.appendChild(document.createElement("span"));
> +	       selectorElement.textContent = selectorText;
> +	      
selectorElement.classList.add(WI.SpreadsheetCSSStyleDeclarationSection.MatchedS
electorElementStyleClassName);
> +	   };

This function duplicates some logic inside `appendSelector`.  You could rework
this function to return `selectorElement` and take another `matched` parameter,
and then use it inside `appendSelector` so that this code only exists once.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:211
> +	   super.layout();

See comment on SpreadsheetCSSStyleDeclarationEditor.js:51.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
56
> +	       super.refresh();

You should also pass along `significantChange`, just for completeness.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
115
> +	       previousSection.lastInGroup = true;

I realize that you copied much of this, but `lastInGroup` doesn't seem to be
used anywhere.	It is used by CSSStyleDeclarationSection.js, but not in this
patch.	I think that for now it would be better to remove it so we don't
potentially bring unneeded code into the future.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
117
> +	   this.removeAllSubviews();

It reads awkwardly to have this be after a loop that calls a function called
`appendStyleSection`, especially since `appendStyleSection` doesn't actually
add the section to the DOM.  Can you move this avoid the for-loop, or better
yet combine/rework both loops into one?