[webkit-reviews] review granted: [Bug 137131] Web Inspector: tests under LayoutTests/inspector/debugger are flaky : [Attachment 380254] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 137131: Web Inspector: tests under LayoutTests/inspector/debugger are flaky
https://bugs.webkit.org/show_bug.cgi?id=137131

Attachment 380254: Patch

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




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

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

rs=me

> LayoutTests/ChangeLog:3
> +	   Web Inspector: tests under LayoutTests/inspector/debugger are flaky

It's a bit odd to have the title of this bug be about debugger when both of the
tests being modified in this patch aren't related to the debugger at all.  I'd
either make a new bug, or include some explanation about how r250655 caused
these tests to become "more" flakey.  My guess is the changes made to use the
`RunLoop` instead of a `Timer` in `InspectorFrontendClientLocal`.  Some
explanation of why this happened would be great :)

> LayoutTests/inspector/layers/layers-for-node.html:29
> +	       callback: getDocument.bind(this, layersChanged)

This is really odd.  We usually try to avoid binding arguments like this. 
Instead, please just create `let layersChanged = null;` outside this function
and set it here, then use it inside `getDocument` (just like `documentNode` and
`initialLayers`).

> LayoutTests/inspector/layers/layers-for-node.html:35
> +	   layersChangedPromise.then(step({

`step` doesn't return a function, so I don't think this works as you expect. 
You'll need to wrap it in a function.
```js
    layersChangedPromise.then(() => {
	step({
	    ...
	});
    });
```

> LayoutTests/inspector/timeline/line-column.html:44
> +    const eventWhitelist = new Set(["RenderingFrame", "ConsoleProfile",
"EventDispatch", "FunctionCall"]);

Interesting idea!  Other than "ScheduleStyleRecalculation", were there any
other events that would sometimes get recorded? :P