Bug 190159 – Web Inspector: prevent layer events from firing until the layer information is re-requested

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Considering how often `LayerTree.events.layerTreeDidChange` is fired, we shouldn't allow a re-firing unless the frontend has re-requested the layer information.

[Devin Rousso]

Created attachment 351304 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351304 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No tests as this patch only has an effect when the layer information isn't needed.

r- just because we should write a test for this because it would be pretty important that we don't break it.

1. A test that triggers (layer change -> request -> layer change) and confirm 2 layer change events
2. A test that triggers (layer change -> no request -> layer change) and confirm 1 layer change event

> Source/WebCore/inspector/agents/InspectorLayerTreeAgent.h:94
> +    bool m_layersRequested { true };

While the logic is right, this feels the opposite of what we want from a naming convention. It seems unintuitive to start out by saying layers were requested. How about any of these?

    m_suppressLayerChangeEvents { false };
    m_hasRequestedLayersSinceLastChange { false };

[Devin Rousso]

Created attachment 351347 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351347 [details]
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(() => {
        ...
    });

[Devin Rousso]

Created attachment 351439 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351439 [details]
Patch

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

> LayoutTests/inspector/layers/layerTreeDidChange-expected.txt:16
> +PASS: LayerTreeDidChange should not fire before layersForNode is 

Incomplete sentence!?

> LayoutTests/inspector/layers/layerTreeDidChange.html:70
> +                ++layerTreeDidChanageCount;

Typo: `layerTreeDidChanageCount`

> LayoutTests/inspector/layers/layerTreeDidChange.html:88
> +                    InspectorTest.expectEqual(layerTreeDidChanageCount, 1, "LayerTreeDidChange should not fire before layersForNode is ");

Grammaro: Incomplete sentence.

[EWS Watchlist]

Comment on attachment 351439 [details]
Patch

Attachment 351439 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9433032

New failing tests:
inspector/layers/layerTreeDidChange.html

[EWS Watchlist]

Created attachment 351452 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 351455 [details]
Patch

[Devin Rousso]

Created attachment 351456 [details]
Patch

Fixed typos

[Joseph Pecoraro]

Comment on attachment 351456 [details]
Patch

Nice! 👍

[WebKit Commit Bot]

Comment on attachment 351456 [details]
Patch

Clearing flags on attachment: 351456

Committed r236777: <https://trac.webkit.org/changeset/236777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44959712>

[Truitt Savell]

Looks like the new test inspector/layers/layerTreeDidChange.html added in https://trac.webkit.org/changeset/236777/webkit is flakey. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Flayers%2FlayerTreeDidChange.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/inspector/layers/layerTreeDidChange-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/inspector/layers/layerTreeDidChange-actual.txt
@@ -6,12 +6,16 @@
 Creating layer...
 Layer tree changed
 Creating layer...
-Layer tree changed
-PASS: LayerTreeDidChange should fire after layersForNode is called
+FAIL: LayerTreeDidChange should fire after layersForNode is called
+    Expected: 2
+    Actual: 1
 
 -- Running test case: LayerTree.layerTreeDidChange.WithoutRequest
+Layer tree changed
+Creating layer...
 Creating layer...
 Layer tree changed
-Creating layer...
-PASS: LayerTreeDidChange should not fire before layersForNode is called
+FAIL: LayerTreeDidChange should not fire before layersForNode is called
+    Expected: 1
+    Actual: 2

[Joseph Pecoraro]

>  -- Running test case: LayerTree.layerTreeDidChange.WithoutRequest
> +Layer tree changed
> +Creating layer...
>  Creating layer...
>  Layer tree changed
> -Creating layer...

This seems to be the telling part, the Layer tree changed happened too late, which means we started the next test too early.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #16)
> >  -- Running test case: LayerTree.layerTreeDidChange.WithoutRequest
> > +Layer tree changed
> > +Creating layer...
> >  Creating layer...
> >  Layer tree changed
> > -Creating layer...
> 
> This seems to be the telling part, the Layer tree changed happened too late,
> which means we started the next test too early.

I spoke to Simon, we will try forcing a layout after adding the DOM node, that should help trigger the layerTreeDidChange event before the test page sends an event to the frontend expecting to have performed a layer tree update and to continue to test.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #17)
> (In reply to Joseph Pecoraro from comment #16)
> > >  -- Running test case: LayerTree.layerTreeDidChange.WithoutRequest
> > > +Layer tree changed
> > > +Creating layer...
> > >  Creating layer...
> > >  Layer tree changed
> > > -Creating layer...
> > 
> > This seems to be the telling part, the Layer tree changed happened too late,
> > which means we started the next test too early.
> 
> I spoke to Simon, we will try forcing a layout after adding the DOM node,
> that should help trigger the layerTreeDidChange event before the test page
> sends an event to the frontend expecting to have performed a layer tree
> update and to continue to test.

Attempted to improve this with:
https://trac.webkit.org/changeset/236884/webkit