1554499 - Refactor display item building so that building function knows the item index beforehand

Status: RESOLVED FIXED

(Core :: Web Painting, task, P3)

Description

[Miko Mynttinen]

Currently display items have their index passed in as an arbitrary positioned constructor parameter. This index is then returned by CalculatePerFrameKey(), and MakeDisplayItem() uses it and the display item type to construct the per frame key for the display item.

This hack makes it difficult to implement the reuse of display items that can have index (= display item types that frame can have more than one of). We should refactor the display item creation, so that the callers requiring items with index need to pass it more explicitly.

Comment 1

[Matt Woodrow (:mattwoodrow)]

My suggestion for this is to take the per-type logic within the virtual CalculatePerFrameKey, and move it to the static per-type callsites of MakeDisplayItem(or AppendToTop). Most item types only have one creation site, so this shouldn't result in much duplication, and we can add static helper functions as needed.

We'll probably also want a second variation of MakeDisplayItem that takes the computed index (MakeDisplayItemWithIndex?) so that we don't need to touch the callsites of items that aren't affected by this change.

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1554499 - change ComputePerFrameKey to be a static method.

This static method is assumed to have the same signature as the type's constructor,
and so we must have an implementation of ComputePerFrameKey for each constructor
a display item provides that is called by MakeDisplayItem. Notably this excludes
the MakeClone constructor for a lot of items.

There is a default varargs implementation on nsDisplayItem which everyone
inherits by default, so types which previously didn't overload this method
still don't need to.

Providing an implementation of ComputePerFrameKey on some display item type
shadows the varargs implementation, so one doesn't need to worry about overloading
one constructor but forgetting about another -- if you do, the compiler will only
see the overload and complain that the signature doesn't match.

One slightly annoying result of this is that display items which previously
inherited an overloaded implementation from a superclass now must provide
their own manual implementations. Although as far as I could tell, all of
those cases had a trivial implementation of key=0 (the super class supported
custom keys but the subclasses didn't make use of it).

In those cases I just hardcoded key=0, but it's possible that it would be
better to call into the superclass' implementation to be more robust to changes.

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: Bug 1554499 - Rename PerFrameKey -> PerFrameIndex for most methods

This distinguishes better between the overloaded aspect of the PerFrameKey and the
actual mixed value.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/789e5d3fcf18
change ComputePerFrameKey to be a static method. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1a6a6a38c987
Rename PerFrameKey -> PerFrameIndex for most methods r=mattwoodrow

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1554499) for mochitest failures at test_restyles.html.

Backout: https://hg.mozilla.org/integration/autoland/rev/5fceb8c496bfe98c1081d9ff8712e9107dd22767

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1a6a6a38c9877b47da1cbb81449fc15d8e4afb87&selectedJob=257212351

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257212351&repo=autoland&lineNumber=7483

Comment 6

[Andreea Pavel [:apavel]]

Hi Alexis, your patches also turned bug 1512068 into a permafailure:

https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C10%2Cx64%2Cquantumrender%2Cshippable%2Copt%2Cmochitests%2Ctest-windows10-64-shippable-qr%2Fopt-mochitest-e10s-5%2Cm%285%29&tochange=cc3b096379b79bd834b4350e07fc92be48dd82fc&fromchange=a8eb1b6edb40822aa1b4e72fb6954a559bd9aca8&selectedJob=257541971

Comment 7

[Aria Beingessner [:Gankra]]

investigating...

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:Gankro, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Aria Beingessner [:Gankra]]

this is in my queue, there's just several higher priority things in the way (unless matt or miko really need/want this to make progress soon)

Comment 10

[Aria Beingessner [:Gankra]]

Unassigning myself as there seems to perpetually be higher priority webrender bugs to look into since we started shipping more to release... :/

Comment 11

[Miko Mynttinen]

Attachment: Bug 1554499 - Add missing CalculatePerFrameIndex implementations

Depends on D37804

Comment 12

[Miko Mynttinen]

Attachment: Bug 1554499 - Remove member variables that were previously only used to calculate per frame index

Depends on D50185

Comment 13

[Miko Mynttinen]

Attachment: Bug 1554499 - Store WebRenderAnimationData using display item type as key

Depends on D50186

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/133cddb65f59
change ComputePerFrameKey to be a static method. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a49d1c9e8b14
Rename PerFrameKey -> PerFrameIndex for most methods r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/f60fee484460
Add missing CalculatePerFrameIndex implementations r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c802ab8cc730
Remove member variables that were previously only used to calculate per frame index r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3a49bec95338
Store WebRenderAnimationData using display item type as key r=jrmuizel

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/133cddb65f59
https://hg.mozilla.org/mozilla-central/rev/a49d1c9e8b14
https://hg.mozilla.org/mozilla-central/rev/f60fee484460
https://hg.mozilla.org/mozilla-central/rev/c802ab8cc730
https://hg.mozilla.org/mozilla-central/rev/3a49bec95338

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequent crashes, at least on OS X (Bug 1594381):

https://hg.mozilla.org/mozilla-central/rev/d9daf28a5559712a8db772aa8388f444a93f4063

Crash reports:
https://crash-stats.mozilla.org/report/index/cfdad603-bf93-4920-af93-470030191106
https://crash-stats.mozilla.org/report/index/5f8b00c2-73c8-4483-87d0-c50d80191106
https://crash-stats.mozilla.org/report/index/7dbb0f6e-94cb-4717-a84c-0d3d20191106

Comment 17

[Alexandru Michis [:malexandru]]

It also seems to have caused Bug 1567274 to appear again:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=Windows%2C10%2Cx64%2CMinGW%2Cdebug%2CFirefox%2Cfunctional%2Ctests%2C%28local%29%2Ctest-windows10-64-mingwclang%2Fdebug-firefox-ui-functional-local-e10s%2CFxfn-l%28en-US%29&tochange=5319361284412e36acd0235d85cfa25149bc9a67&group_state=expanded&fromchange=082bcb14a8c5796ea53715de00b37852f9d45b35&selectedJob=274827079

Comment 18

[Aria Beingessner [:Gankra]]

miko's on it

Comment 19

[Aria Beingessner [:Gankra]]

Assigning this to miko since he's working on this and not me.

Comment 20

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 1: Add MakeDisplayItemWithIndex r=mattwoodrow

Comment 21

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 2: Add nsDisplayList::AppendNewToBottomWithIndex() and nsDisplayList::AppendNewToTopWithIndex() r=mattwoodrow

Depends on D74080

Comment 22

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 3: Rename SetPerFrameKey() to SetPerFrameIndex() and mKey to mPerFrameIndex r=mattwoodrow

This clarifies that mPerFrameIndex is just a part of the key returned by GetPerFrameKey().

Depends on D74081

Comment 23

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 4: Remove index from nsDisplaySolidColor r=mattwoodrow

Depends on D74082

Comment 24

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 5: Remove index from nsDisplayTextOverflowMarker r=mattwoodrow

Depends on D74083

Comment 25

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 6: Remove index from nsDisplayBackgroundImage, nsDisplayTableBackgroundImage, nsDisplayFixedPosition, nsDisplayTableFixedPosition r=mattwoodrow

Depends on D74084

Comment 26

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 7: Remove index from nsDisplayCompositorHitTestInfo r=mattwoodrow

Depends on D74085

Comment 27

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 8: Remove index from nsDisplayMathMLCharForeground, nsDisplayMathMLBar, nsDisplayNotation r=mattwoodrow

Depends on D74086

Comment 28

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 9: Remove index from nsDisplayTransform r=mattwoodrow

Depends on D74087

Comment 29

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 10: Remove index from nsDisplayTableThemedBackground r=mattwoodrow

Depends on D74088

Comment 30

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 11: Remove index from nsDisplayTableBackgroundColor r=mattwoodrow

Depends on D74089

Comment 31

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 12: Remove index from nsDisplayOwnLayer r=mattwoodrow

Depends on D74090

Comment 32

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 13: Remove index from nsDisplayWrapList r=mattwoodrow

Depends on D74091

Comment 33

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 14: Remove index from nsDisplayBlendContainer, nsDisplayTableBlendContainer, nsDisplayBlendMode, nsDisplayTableBlendMode r=mattwoodrow

Depends on D74092

Comment 34

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 15: Use MakeDisplayItemWithIndex with nsDisplayCanvasBackgroundImage r=mattwoodrow

Depends on D74093

Comment 35

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 16: Move table per frame index helpers to nsDisplayList.cpp r=mattwoodrow

Depends on D74094

Comment 36

[Miko Mynttinen]

Attachment: Bug 1554499 - Part 17: Remove CalculatePerFrameKey() r=mattwoodrow

Depends on D74095

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe5dc2700bbe
Part 1: Add MakeDisplayItemWithIndex r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e8af75b90320
Part 2: Add nsDisplayList::AppendNewToBottomWithIndex() and nsDisplayList::AppendNewToTopWithIndex() r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fba736154f5f
Part 3: Rename SetPerFrameKey() to SetPerFrameIndex() and mKey to mPerFrameIndex r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2a5158285cd5
Part 4: Remove index from nsDisplaySolidColor r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/cd01a403c04d
Part 5: Remove index from nsDisplayTextOverflowMarker r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/75651ced7904
Part 6: Remove index from nsDisplayBackgroundImage, nsDisplayTableBackgroundImage, nsDisplayFixedPosition, nsDisplayTableFixedPosition r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3986b6f09e62
Part 7: Remove index from nsDisplayCompositorHitTestInfo r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3fcba0495040
Part 8: Remove index from nsDisplayMathMLCharForeground, nsDisplayMathMLBar, nsDisplayNotation r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a6d9f235d7bb
Part 9: Remove index from nsDisplayTransform r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e75e94d2c712
Part 10: Remove index from nsDisplayTableThemedBackground r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/af84a5159a44
Part 11: Remove index from nsDisplayTableBackgroundColor r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/9cd738ed8e4f
Part 12: Remove index from nsDisplayOwnLayer r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/24885f67361f
Part 13: Remove index from nsDisplayWrapList r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/18a3148eff4b
Part 14: Remove index from nsDisplayBlendContainer, nsDisplayTableBlendContainer, nsDisplayBlendMode, nsDisplayTableBlendMode r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7f0a11b398ce
Part 15: Use MakeDisplayItemWithIndex with nsDisplayCanvasBackgroundImage r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/5f9b5f39ff69
Part 16: Move table per frame index helpers to nsDisplayList.cpp r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/22b375ad5f36
Part 17: Remove CalculatePerFrameKey() r=mattwoodrow

Comment 38

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe5dc2700bbe
https://hg.mozilla.org/mozilla-central/rev/e8af75b90320
https://hg.mozilla.org/mozilla-central/rev/fba736154f5f
https://hg.mozilla.org/mozilla-central/rev/2a5158285cd5
https://hg.mozilla.org/mozilla-central/rev/cd01a403c04d
https://hg.mozilla.org/mozilla-central/rev/75651ced7904
https://hg.mozilla.org/mozilla-central/rev/3986b6f09e62
https://hg.mozilla.org/mozilla-central/rev/3fcba0495040
https://hg.mozilla.org/mozilla-central/rev/a6d9f235d7bb
https://hg.mozilla.org/mozilla-central/rev/e75e94d2c712
https://hg.mozilla.org/mozilla-central/rev/af84a5159a44
https://hg.mozilla.org/mozilla-central/rev/9cd738ed8e4f
https://hg.mozilla.org/mozilla-central/rev/24885f67361f
https://hg.mozilla.org/mozilla-central/rev/18a3148eff4b
https://hg.mozilla.org/mozilla-central/rev/7f0a11b398ce
https://hg.mozilla.org/mozilla-central/rev/5f9b5f39ff69
https://hg.mozilla.org/mozilla-central/rev/22b375ad5f36