[webkit-reviews] review granted: [Bug 192739] Web Inspector: Styles: fix race conditions when editing : [Attachment 361008] 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 192739: Web Inspector: Styles: fix race conditions when editing
https://bugs.webkit.org/show_bug.cgi?id=192739

Attachment 361008: Patch

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




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

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

r=me, thanks for adding the extra tests.  I think there's a few tweaks to be
made, but its basically there 😁  Please address them before landing.  Awesome
work!

> LayoutTests/inspector/css/modify-css-property-race.html:8
> +function expand(iterationCount) {

Is there a reason to add this maximum count?  I feel like on slower debug
builds, this may cause problems.  I think this logic would still work even if
you let it continue rAF-looping infinitely.

> LayoutTests/inspector/css/modify-css-property-race.html:30
> +    TestPage.dispatchEventToFrontend("TestPage-didStopExpanding");

This isn't used anywhere.  Given my comments below, I think we can remove it.

> LayoutTests/inspector/css/modify-css-property-race.html:71
> +		   if (valueModifiedByWebInspector) {

We should wait for one more non-inspector-modification update to make sure that
we haven't "ruined" the iteration cycle.  Rather than keeping a boolean
`valueModifiedByWebInspector`, we could keep a `updatedCount` and do different
things depending on the value (e.g. `0` is the current `else`, `1` is the
current `if`, and `2` would do the same as the `if` but instead check that the
value is > 10 and would then end the test).

> LayoutTests/inspector/css/modify-css-property-race.html:79
> +		       InspectorTest.log("First style change.");

We should assert (or even `expectGreaterThan`) that the current value is > 42,
just to make sure that the `expand` is working as expected.

> LayoutTests/inspector/css/modify-css-property.html:127
> +	       // WI.CSSStyleDeclaration.Event.PropertiesChanged event should
not fire when the style declaration is locked.

Along these lines, we should `assert` in the event listener that
`!styleDeclaration.locked` just to make sure we aren't firing at the wrong
time.

> LayoutTests/inspector/css/modify-css-property.html:159
> +	      
styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).the
n((event) => {

One issue with using `awaitEvent` is that there isn't a good way to remove the
added event listener, and we will want to remove this event listener before the
next test so that each case is "independent".  I think this would be better off
using `singleFireEventListener` (as well as `removeEventListener).

> LayoutTests/inspector/css/modify-css-property.html:176
> +	       resolve();

Putting `resolve` here won't give the event listener above a chance to fire.  I
think we need to wait for the `PropertiesChanged` event to actually fire
(expected) before finishing the test.

> LayoutTests/inspector/css/modify-css-property.html:203
> +	       styleDeclaration.locked = false;

Would there be any different of a result if `locked` was set to `true`?  I'd
imagine that this wouldn't be different, as `locked` should only affect backend
updates to the frontend.