[webkit-reviews] review denied: [Bug 156271] Web Inspector: CSS autocomplete: suggestion hint should be the most commonly used property and not the alphabetically first one : [Attachment 356657] Patch

[bugzilla-daemon at webkit.org]

Nikita Vasilyev <nvasilyev at apple.com> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 156271: Web Inspector: CSS autocomplete: suggestion hint should be the most
commonly used property and not the alphabetically first one
https://bugs.webkit.org/show_bug.cgi?id=156271

Attachment 356657: Patch

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




--- Comment #33 from Nikita Vasilyev <nvasilyev at apple.com> ---
Comment on attachment 356657
  --> https://bugs.webkit.org/attachment.cgi?id=356657
Patch

r- because the math seems to be wrong.

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:84
> +		   let existing = WI.CSSProperty.propertyNameUsage(property);

It isn't trivial to guess what `propertyNameUsage` returns. Maybe
`getUsageCount` or `getPropertyNameUsageCount` would be a better name.

    let existingCount = WI.CSSProperty.getPropertyNameUsageCount(property);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:393
> +	       let index =
completions.minIndex(WI.CSSProperty.sortByPropertyNameUsage);

This method is pretty dense and hard to follow. To understand what it does, I
had to read it once, read `minIndex` and
`WI.CSSProperty.sortByPropertyNameUsage`, come back and read it a couple more
times.

`index` of what? You should either add more code comments or make variable
names more descriptive (e.g. mostUsedPropertyIndex).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:394
> +	       let usage = completions.reduce((accumulator, current) =>
accumulator + WI.CSSProperty.propertyNameUsage(current), 0);

Nit: `totalUsage`.
Nit: I'd add an `averageUsage` variable as well.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:399
> +	       if (WI.CSSProperty.propertyNameUsage(completions[index]) >=
(usage / completions.length) * 0.9)
> +		   return index;

This math seems questionable.

Let's say we have two completion items with the following usage counts 100 and
101.

    average = 100.5
    100.5 * 0.9 = 90.45
    101 >= 90.45

Did you mean

    if (WI.CSSProperty.propertyNameUsage(completions[index]) * 0.9 >= (usage /
completions.length))

?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:402
> +	   return 0; // Select the first item

Nit: this comment is obvious and unnecessary.