1100159 - Rename FrameLayerBuilder methods and members that claim the layer manager is retained when it often is not

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

FrameLayerBuilder::DidBeginRetainedLayerTransaction is called by FrameLayerBuilder::AddPaintedDisplayItem which passes |tempManager|, which is not retained. DidBeginRetainedLayerTransaction is therefore a misleading name. Since the passed LayerManager pointer is stored in mRetainingManager, the name of mRetainingManager is also misleading, as is the getter GetRetainingLayerManager.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Matt Woodrow (:mattwoodrow)]

The naming is a bit confusing, but a 'retained' here means that we retain both the LayerManager and all the display item data about what went into it (but not necessarily the buffer contents).

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> 'retained' here means that we retain both the LayerManager

I'm confused. If an nsIWidget's GetLayerManager() implementation chooses to return the same object across calls then the layer manager would seem to be retained across paints. The tempManager object mentioned in comment 0 isn't kept around across paints though. In what sense do we retain that LayerManager?

Comment 4

[Matt Woodrow (:mattwoodrow)]

We generally assume that the nsIWidget will return the same layer manager for each paint, afaik all the implementations do that, since it'd be pretty useless not to.

'tempManager' in AddPaintedDisplayItem is absolutely retained between paints. We store it as mInactiveLayer on the DisplayItemData retained for the display item.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8523557 [details] [diff] [review]
patch

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

I agree there's some confusion here. The confusion is that in some places "retained layer manager" means specifically the widget's layer manager. In other places it means more generally "the widget's layer manager and any temporary layer managers retained between paints".

I propose we use "retained" to mean the latter and "widget layer manager" to mean the former.

Comment 6

[Gregory Pappas [:gregp]]

FrameLayerBuilder was removed in bug 1726291