[webkit-reviews] review granted: [Bug 177208] Web Inspector: Styles Redesign: support editing of properties and property values : [Attachment 321501] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177208: Web Inspector: Styles Redesign: support editing of properties and
property values
https://bugs.webkit.org/show_bug.cgi?id=177208

Attachment 321501: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 321501
  --> https://bugs.webkit.org/attachment.cgi?id=321501
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:164
> +    set name(newName)

Nit: You can just use `name`.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:166
> +	   this._name = newName;

Nit: Lets bail if things did not change:

    if (this._name === name)
	return;

    this._name = name;
    ...

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:138
> +	       nameElement.addEventListener("input", () => {
> +		   this._property.name = nameElement.textContent.trim();
> +	       });
> +
> +	       valueElement.contentEditable = "plaintext-only";
> +	       valueElement.addEventListener("input", () => {
> +		   this._property.rawValue = valueElement.textContent.trim();
> +	       });

As far as I can, by using the "input" event that means we are going to replace
the CSS every single keystroke. For our model objects that may be fine, but it
does look like that synchronously triggers a backend update. That is probably
is not as performant as we want. If I'm typing "color: red" I'm able to type
that pretty quickly, all of the intermediate "c", "co", "col", "colo" etc
inputs don't need to be sent to the backend because they are about to be
obsoleted very very quickly.

We probably want to limit/throttle/debounce backend updates. Example strategies
could be:

    • Maximum 1 update every 100ms.
	=> If the user types super fast this would only update at most 10 times
a second

    • Only update if no user change in 100ms.
	=> As long as the user is typing fast there are no updates until they
stop typing.

The second strategy seems to be what the sidebar does right now. That said, the
first strategy seems pretty reasonable.

For now this seems fine, but lets add a FIXME somewhere in order to throttle
updates.