[webkit-reviews] review granted: [Bug 190159] Web Inspector: prevent layer events from firing until the layer information is re-requested : [Attachment 351347] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 190159: Web Inspector: prevent layer events from firing until the layer
information is re-requested
https://bugs.webkit.org/show_bug.cgi?id=190159

Attachment 351347: Patch

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




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

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

Awesome! r=me with some test nits.

> Source/WebInspectorUI/UserInterface/Controllers/LayerTreeManager.js:33
> -	   if (this._supported)
> +	   if (this.supported)
>	       LayerTreeAgent.enable();

Lets make this the usual pattern:

    if (window.LayerTreeAgent)
	LayerTreeAgent.enable();

And we can simplify LayerTreeObserver by removing its
`WI.layerTreeManager.supported` check which seems unnecessary.

> LayoutTests/inspector/layers/layerTreeDidChange-expected.txt:6
> +PASS: LayerTreeDidChange should be able to fire on page load

I'm not sure I understand the `should fire on page load`. This is firing after
a change.

> LayoutTests/inspector/layers/layerTreeDidChange-expected.txt:10
> +PASS: LayerTreeDidChange should be able to fire on page load

I'm not sure I understand the `should fire on page load`. This is firing after
a change.

> LayoutTests/inspector/layers/layerTreeDidChange.html:12
> +    document.body.appendChild(element);

Rather than a setTimeout down below it would be best to trigger an event and
more deterministically advance the test.

A good idea here may be to trigger an event after a paint:

    requestAnimationFrame(() => {
	TestPage.dispatchEventToFrontend("TestPageDidAppendElement");
    });

> LayoutTests/inspector/layers/layerTreeDidChange.html:34
> +		       });

To confirm "TestPageDidAppendElement" is late enough you should be able to move
the `resolve` here:

	InspectorTest.awaitEvent("TestPageDidAppendElement").then(() => {
	    InspectorTest.assert(count === 2);
	    resolve();
	});

> LayoutTests/inspector/layers/layerTreeDidChange.html:63
> +		       setTimeout(() => {

And this setTimeout should be able to change to:

    InspectorTest.awaitEvent("TestPageDidAppendElement").then(() => {
	...
    });