Web Inspector: Layer mutations should be purely based on layerId, not based on nodeId

[Ross Kirsling]

Created attachment 324318 [details]
[Image] Problem

The current implementation of LayerTreeManager#layerTreeMutations tries to cleverly detect when a compositing layer for a given DOM node has been destroyed and recreated (resulting a new layerId but the same nodeId).

This is nonsense for two reasons:
1. Even if you call this layer "preserved", the old layer object and new layer object share NO data, so there's no way for a consumer to act on this supposed preservation.
2. Crucially, a single nodeId can correspond to multiple layerIds at once, such as when a pseudo-element of a DOM node gives rise to a layer of its own.

Turns out that this causes problems even in the legacy Layers sidebar -- simply visit https://devinrousso.com/ for easy repro.
(See attached image, where the layer count is incorrect and some popover content is missing.)

This method should be rewritten to only check layerIds, not nodeIds.

[Ross Kirsling]

Created attachment 324320 [details]
Patch

[Devin Rousso]

Comment on attachment 324320 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        * UserInterface/Controllers/LayerTreeManager.js:
> +        (WI.LayerTreeManager.prototype.layerTreeMutations):

This could use an explanation as to why this is needed or what the problem originally was.

> Source/WebInspectorUI/UserInterface/Controllers/LayerTreeManager.js:-59
> -            return layer.isGeneratedContent ? layer.pseudoElementId : layer.nodeId;

Does this patch still work with pseudo-elements?  Adding a test for this would be awesome =D

> Source/WebInspectorUI/UserInterface/Controllers/LayerTreeManager.js:-73
> -            if (layer.isReflection)

Is the concept of `isReflection` no longer valid/useful?

[Ross Kirsling]

(In reply to Devin Rousso from comment #2)
> > +        * UserInterface/Controllers/LayerTreeManager.js:
> > +        (WI.LayerTreeManager.prototype.layerTreeMutations):
> 
> This could use an explanation as to why this is needed or what the problem
> originally was.

Can do.

> > Source/WebInspectorUI/UserInterface/Controllers/LayerTreeManager.js:-59
> > -            return layer.isGeneratedContent ? layer.pseudoElementId : layer.nodeId;
> 
> Does this patch still work with pseudo-elements?  Adding a test for this
> would be awesome =D
> 
> > Source/WebInspectorUI/UserInterface/Controllers/LayerTreeManager.js:-73
> > -            if (layer.isReflection)
> 
> Is the concept of `isReflection` no longer valid/useful?

Sorry if this wasn't all clear above: It was incorrect to be looking for special cases involving nodeIds at all (as these need not be unique in the layer list). A preserved layer should be nothing more or less than a recycled layerId (the thing that *is* unique). Pseudo-elements were actually the very thing being mishandled on your website, as a result of this special-casing. :) Reflections should follow suit.

[Ross Kirsling]

Created attachment 324564 [details]
Patch

[Ross Kirsling]

Comment on attachment 324564 [details]
Patch

Made ChangeLog and test more informative.

[Devin Rousso]

Comment on attachment 324564 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 324564 [details]
Patch

Clearing flags on attachment: 324564

Committed r223914: <https://trac.webkit.org/changeset/223914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568610>