881451 - ThebesLayerComposite::GetCompositionBounds() needs to be fixed up in various ways

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There are at least a couple of problems with this code:
1) The logic at the top of the loop to update the scrollableLayer variable and potentially end up using it with a different parentMetrics seems very wrong.
2) The function doesn't appear to take into account any intermediate surfaces it might be rendering to.

BenWa and I were discussing this code today and it will probably misbehave when there are multiple scrollable layers in the tree, such as the multi-apzc project is designed to allow, so this will need to be fixed up.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

There also appear to be various coordinate system mismatches in this function. The most obvious one is the calculation of the "scrollOffset" variable, which takes a CSSPixel value, multiplies it by LayerPixelsPerCSSPixel to get the LayerPixel value, and then divides it by something which gets back LayoutDevicePixels (or maybe CSSPixels - I'm still trying to figure that out). It then subtracts that value from a LayerPixel "content" variable, which is wrong no matter how you slice it.

Comment 2

[Benoit Girard (:BenWa)]

Moving to depend on bug 776030 because it blocks only Fennec APZC.

Comment 3

[Chris Lord [:cwiiis]]

So we just don't use this code at all anymore. Will attach a patch that removes the unused bits.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Remove unused members of TiledLayerProperties

This code could probably use an auditing, but if we just end up shrinking it into nothingness, that'd do too.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 827560 [details] [diff] [review]
Remove unused members of TiledLayerProperties

Review of attachment 827560 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but this doesn't address the bug so keep the bug open.

Comment 6

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a775f0d923a6

Seems we're tracking the real problem in bug 935219 now, so I'm going to co-opt this bug to track just removing these unused properties - let me know if you think that's too messy and we can agree on something nicer :)

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #6)
> Seems we're tracking the real problem in bug 935219 now

Bug 935219 won't affect the code in ThebesLayerComposite. I might change what mCompositionBounds is, but I don't understand any of the code in ThebesLayerComposite so I'm not going to touch it.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Chris Lord [:cwiiis]]

This has been fixed in various other bugs, closing.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

A patch landed here, changing resolution to FIXED.