Closed Bug 564993 Opened 10 years ago Closed 10 years ago

Restructure layers to prepare for retained layers

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(6 files, 2 obsolete files)

4.92 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.11 KB, patch
mats
: review+
Details | Diff | Splinter Review
14.17 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.62 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
101.90 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.05 KB, patch
Details | Diff | Splinter Review
I have some patches to clean up layer code.
CopyFrom won't be needed after all. It was dumb of me to add it in the first place.
Attachment #444605 - Flags: review?(bas.schouten)
Currently in layout we try to flatten the layer tree as much as possible, e.g. replacing a container with one child with just the child. This makes it really hard to implement retained layers, though, e.g. if we need to add a child to the container. So stop doing this flattening. Layer backends can still flatten internally for rendering if necessary.
Attachment #444606 - Flags: review?(matspal)
Currently visible rects are set from a display item when we add its layer to a container, but for container layers they are also set in nsDisplayList::BuildLayers by adding up the areas of their children. This is a duplication of effort for all non-root containers. The visible rect for the root layer for the whole layer tree can be set specially by recording the visible rect of its display list instead. This requires splitting nsDisplayList::Paint into PaintRoot and PaintForFrame.
Attachment #444609 - Flags: review?(matspal)
This is trivial.
Attachment #444610 - Flags: superreview?(vladimir)
Attachment #444610 - Flags: review?(bas.schouten)
The layer changes here remove the explicit "drawing phase". After layer tree construction, layout calls EndTransaction immediately but passes a callback that the layer manager uses to draw into any ThebesLayers that need drawing. I'm also adding an AsThebesLayer virtual method to do safe dynamic downcast to ThebesLayer. These layer changes are a good simplification.

On the layout side, everything that renders into ThebesLayers needs to adapt to this callback-based API. I'm also moving the layer tree construction code out of nsDisplayList into a new class FrameLayerBuilder. An nsDisplayListBuilder owns one of these. Right now nothing else changes, but in later patches FrameLayerBuilder is massively restructured to handle retaining of layers.
Attachment #444612 - Flags: superreview?(vladimir)
Attachment #444612 - Flags: review?(bas.schouten)
Comment on attachment 444612 [details] [diff] [review]
Part 5: Change ThebesLayer painting to be callback-based; move layer tree construction to FrameLayerBuilder.

Need Mats review on the layout stuff here.
Attachment #444605 - Flags: review?(bas.schouten) → review+
Attachment #444610 - Flags: review?(bas.schouten) → review+
Whiteboard: [needs landing]
Comment on attachment 444606 [details] [diff] [review]
Part 2: Don't flatten the layer tree in layout

>  nsRefPtr<Layer> layer = container.forget();
>  return layer.forget();

Seems like we could avoid the extra ref counting here.
"return container.forget().get()" perhaps?
Attachment #444606 - Flags: review?(matspal) → review+
Comment on attachment 444609 [details] [diff] [review]
Part 3: Split nsDisplayList::Paint into PaintForFrame and PaintRoot and fix visible rect computation

>+    mVisibleRect.ToOutsidePixels(...

Is this correct? -- BuildLayers use ToNearestPixels when
setting mVisibleRect for the items...
Attachment #444609 - Flags: review?(matspal) → review+
Comment on attachment 444612 [details] [diff] [review]
Part 5: Change ThebesLayer painting to be callback-based; move layer tree construction to FrameLayerBuilder.

In FrameLayerBuilder.h
> aItem may be null

There's no aItem.

In nsPresShell.cpp,  DrawThebesLayer:

The old code:
  nsPoint offsetToRoot = aViewToPaint->GetOffsetTo(aDisplayRoot);
  nsRegion dirtyRegion = aDirtyRegion;
  dirtyRegion.MoveBy(offsetToRoot);

  nsPoint translate = -offsetToRoot + aViewToPaint->ViewToWidgetOffset();
  nsIRenderingContext::AutoPushTranslation
    push(rc, translate.x, translate.y);

The new code:
  nsPoint offsetToRoot = params->mOffsetToRoot;
  dirtyRegion.MoveBy(offsetToRoot);
  nsIRenderingContext::AutoPushTranslation
    push(rc, -offsetToRoot.x, -offsetToRoot.y);

 where mOffsetToRoot is calculated as:
  aViewToPaint->GetOffsetTo(aDisplayRoot) -  aViewToPaint->ViewToWidgetOffset(),

So it seems we're now factoring in ViewToWidgetOffset also in the
dirty rect MoveBy.  Please explain why this is right, because I've
stared at this code before and at that time I came to the conclusion
that it was correct (MoveBy should not have the ViewToWidgetOffset).
I could be wrong though.

Nit: there's two spaces in the aViewToPaint->ViewToWidgetOffset() line 
and it exceeds 80 columns.
(In reply to comment #7)
> (From update of attachment 444606 [details] [diff] [review])
> >  nsRefPtr<Layer> layer = container.forget();
> >  return layer.forget();
> 
> Seems like we could avoid the extra ref counting here.
> "return container.forget().get()" perhaps?

There's no addref there. forget() returns an already_AddRefed, so assigning it to 'layer' doesn't do an addref.
(In reply to comment #8)
> (From update of attachment 444609 [details] [diff] [review])
> >+    mVisibleRect.ToOutsidePixels(...
> 
> Is this correct? -- BuildLayers use ToNearestPixels when
> setting mVisibleRect for the items...

I was being conservative, but you're right, these should be consistent. I'll change this to ToNearestPixels.
(In reply to comment #9)
> (From update of attachment 444612 [details] [diff] [review])
> In FrameLayerBuilder.h
> > aItem may be null
> 
> There's no aItem.

Ah, that should read aContainer. I'll fix it.

> So it seems we're now factoring in ViewToWidgetOffset also in the
> dirty rect MoveBy.  Please explain why this is right, because I've
> stared at this code before and at that time I came to the conclusion
> that it was correct (MoveBy should not have the ViewToWidgetOffset).
> I could be wrong though.

You're right. That's just a bug, sorry. I'll fix it and submit a new patch.
(In reply to comment #10)
ok, the compiler can probably optimize away 'layer' then.
Attached patch Part 5 v2 (obsolete) — Splinter Review
Fixed that offset bug by passing two points in the parameters struct.
Attachment #444612 - Attachment is obsolete: true
Attachment #444755 - Flags: superreview?(vladimir)
Attachment #444755 - Flags: review?(matspal)
Attachment #444612 - Flags: superreview?(vladimir)
Attachment #444612 - Flags: review?(matspal)
Attachment #444612 - Flags: review?(bas.schouten)
Comment on attachment 444755 [details] [diff] [review]
Part 5 v2

r=mats on layout/*
Attachment #444755 - Flags: review?(matspal) → review+
Whiteboard: [needs landing]
Attachment #444755 - Flags: review?(bas.schouten) → review+
Attachment #444610 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 444755 [details] [diff] [review]
Part 5 v2

Looks fine; I dislike the way the thebes layer callback is passed to every layer's paint/render method (instead of just to thebes layers), but I guess it works.
Attachment #444755 - Flags: superreview?(vladimir) → superreview+
Whiteboard: [needs landing]
Attached patch Part 5 v3Splinter Review
There was still a leak. I've fixed that, but I also noticed that ContainerLayerOGL doesn't addref its children, and it needs to or we crash with this patch. This fixes that.
Attachment #446999 - Flags: review?
Attachment #446999 - Flags: review? → review?(bas.schouten)
Attachment #446999 - Flags: review?(bas.schouten) → review+
Bas pointed out that ContainerLayerOGL::InsertAfter shouldn't addref aChild if aChild isn't inserted because we can't find aAfter. It doesn't matter much since we don't ever pass in an aAfter that can't be found, but we should still fix this. This patch fixes it.

(Done as a separate patch since I just landed part 5.)
Attachment #444755 - Attachment is obsolete: true
Attachment #447008 - Flags: review?
Comment on attachment 447008 [details] [diff] [review]
Part 6: fix possible leak in ContainerLayerOGL::InsertAfter

Bas already OKed this verbally on IRC
Attachment #447008 - Flags: review?
You need to log in before you can comment on or make changes to this bug.