534425 - (layers) Create basic layers infrastructure

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

This bug is about creating basic layer APIs and integrating them into layout. See
https://wiki.mozilla.org/Gecko:Layers
This implementation will just use Thebes for everything.

Comment 1

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

Attachment: Part 1: Create Layers API

This patch introduces Layers.h, containing the core layers API. It's based on our discussion in https://wiki.mozilla.org/Gecko:Layers, but with some modifications due to things I learned during the implementation. In particular, the attributes of "visible region" and "content is opaque" I moved from ThebesLayer to apply to all layers (since layout knows them for all layers, anyway). In ThebesLayer I moved the region to invalidate out of BeginDrawing and gave it its own API, InvalidateRegion. I added clip-to-rect as an attribute that can be applied to all layers.

I tightened up the constraints on what can be done to the layer tree when. This actually simplifies things for the consumer as well as providing more flexibility to the layer implementation. Now there are two phases: 1) layer tree construction and 2) painting into the ThebesLayers.

In this bug we're only going to support the fundamental layer types: ThebesLayers and ContainerLayers. Additional leaf layer types will be added in other bugs to support acceleration of particular features, e.g. we'll add some kind of ImageLayer when we come to accelerate video.

Comment 2

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

Attachment: Part 2: Create BasicLayers implementation

This is the simplest possible implementation of layers. We do everything using cairo and nothing is retained. This is designed so that Gecko will exercise cairo in just the same way it does before layers. Not only is this a fallback for platforms that don't have an accelerated layer implementations (i.e. currently, all platforms), but it's also what we'll use when we need to do fallback because the layer system doesn't yet support a rendering feature (e.g. when we need to render a chunk of content into a cairo image surface so we can apply our SVG filters). We'll also always want it for rendering to non-screen cairo targets, e.g. printing.

Comment 3

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

Attachment: Part 3: Make nsRefPtr more flexible when assigning from already_AddRefed

When you have an already_AddRefed<S> and you want to assign it to an nsRefPtr<T>, you should be able to do that whenever S is a subtype of T, not just when S is equal to T. This makes it so.

Comment 4

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

Attachment: Part 4: Integrate layers into display list processing

This is where it gets interesting.

We reimplement nsDisplayList::Paint to construct a layer tree using a BasicLayerManager and then render that layer tree to the destination context.

We add nsDisplayItem::BuildLayer which lets any display item return a layer that should be used for the display item *instead of* calling Paint on the item. That is the key integration point where content can choose to not be rendered by cairo, but by the layer system instead. At this point only nsDisplayOpacity uses BuildLayer.

I hope the comments around nsDisplayList::Paint, nsDisplayList::BuildLayers and nsDisplayList::BuildLayer are sufficient. That's really the core of the layout/layers integration. I think it's actually fairly simple, the code is obfuscated a bit by error checking.

Comment 5

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

Attachment: Part 5: Testcase

This is a reftest to ensure that content that is logically above a layer in z-order (in this case, induced by 'opacity') but which can be placed in the bottom-most Thebes layer, actually is (or is otherwise handled in a way which preserves subpixel antialiasing).

Comment 6

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

Attachment: Part 6: Make LayerManagers the interface used to render into widgets

This patch overhauls widget painting so that instead of passing a renderingContext in an nsPaintEvent, layout retrieves a LayerManager from the widget and uses that for rendering. The widget needs to set up its LayerManager to draw into the right place while it dispatches the event.

I've also removed nsPaintEvent::rect, since it's redundant, and make nsPaintEvent::region an nsIntRegion.

In various places --- nsDisplayList::Paint, PresShell::RenderDocument, nsLayoutUtils::Paint, and DrawWindow, we need to pass in as a parameter whether to try to use the widget's layer manager or construct a one-off BasicLayerManager.

Drawing into non-root widgets is a pain, but we still have to deal with it for now. I've chosen to not try to accelerate those, since it will get confusing for display items with dedicated layers if they span widget boundaries and have to have two different accelerated layers for two different layer trees.  So for child widgets, the widget's layer manager is given just a single ThebesLayer, which we draw into using a BasicLayerManager constructed for that purpose.

This patch makes reftests use the widget layer manager. This means that reftests will test whatever layer implementation is active on the widget. Since a non-null context will be provided to nsDisplayList::Paint, we'll end up calling BeginTransactionWithTarget, so for example a GL implementation would render normally to some kind of offscreen GL surface and then read the results back as a pixmap and draw that into the cairo context.

Some refactoring of view manager code happens here since a rendering context is no longer passed through. This is a significant simplification.

I've updated Windows, GTK and Mac (and they work, we pass tryserver tests). I should probably at least attempt to also patch Qt and OS/2 before landing.

Comment 7

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

On top of these patches, I think someone can go ahead and build a GL or D3D immediate-mode layers implementation with changes only to gfx/layers and widget (and someone should in fact start doing that!).

We can also add some kind of transforms as a layer attribute, and add some kind of ImageLayer, and reimplement video rendering to go through layers (with acceleration, when a GL or D3D layer manager is available).

Perhaps the most important next step is to have layout retain layer trees between paints and try to avoid repainting everything (and extend BasicLayers to do some caching of ThebesLayer content). We'd also want to scrolling work on the persistent layer tree. Fennec needs this so we can get rid of its tile cache. That's on my plate.

Comment 8

[Bas Schouten (:bas.schouten)]

Couple of questions about the layers API in the current headers. Is it not possible then to perform transformations, or even layer positioning other than z-order? Since I'm not sure how one would specify positioning information?

Also I'm not sure how transactions and constructions relate to each other. I'm not sure I understand how animation would work if we can only change opacity during a transaction. Since as I understand it now the opacity would only update after completing the transaction. Meaning any animation frame would be bound to a transaction as I understand it currently? I might be missing something though :)

Comment 9

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

(In reply to comment #8)
> Couple of questions about the layers API in the current headers. Is it not
> possible then to perform transformations, or even layer positioning other than
> z-order? Since I'm not sure how one would specify positioning information?

Correct, some kind of transformation property has to be added.

> Also I'm not sure how transactions and constructions relate to each other. I'm
> not sure I understand how animation would work if we can only change opacity
> during a transaction. Since as I understand it now the opacity would only
> update after completing the transaction. Meaning any animation frame would be
> bound to a transaction as I understand it currently? I might be missing
> something though :)

Right, there is no support for animating properties yet. That has to be added too. However it doesn't make sense to add until we're ready to support off-main-thread animation.

Comment 10

[Bas Schouten (:bas.schouten)]

Patch looks good, considering the limitations you mentioned, one thing that I think is a little obscure from the header documentation is the relationship transaction<->construction phase. I think this should be slightly better documented as to avoid confusion on how to use the API.

Comment 11

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

OK, how about this:

 * Each transaction has two phases:
 * 1) Construction: layers are created, inserted, removed and have
 * properties set on them in this phase.
 * BeginTransaction and BeginTransactionWithTarget start a transaction in
 * the Construction phase. When the client has finished constructing the layer
 * tree, it should call BeginDrawing() to enter the drawing phase.
 * 2) Drawing: ThebesLayers are rendered into in this phase, in tree
 * order. When the client has finished drawing into the ThebesLayers, it should
 * call EndTransaction to complete the transaction.

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to comment #11)
> OK, how about this:
> 
>  * Each transaction has two phases:
>  * 1) Construction: layers are created, inserted, removed and have
>  * properties set on them in this phase.
>  * BeginTransaction and BeginTransactionWithTarget start a transaction in
>  * the Construction phase. When the client has finished constructing the layer
>  * tree, it should call BeginDrawing() to enter the drawing phase.
>  * 2) Drawing: ThebesLayers are rendered into in this phase, in tree
>  * order. When the client has finished drawing into the ThebesLayers, it should
>  * call EndTransaction to complete the transaction.
Sounds good to me. :)

Comment 13

[Bas Schouten (:bas.schouten)]

Patches currently do not compile, nsLayoutUtils.h/cpp seem to be missing GetDisplayRootFrame which is referenced from nsDisplayList.cpp.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's in patch 29 in bug 526394.  You probably need to build this on top of (at least part of) that patch queue.

Comment 15

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

Oops, sorry, yes, I forgot to mention that. I imported that into my patch queue for this bug.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 419102 [details] [diff] [review]
Part 1: Create Layers API

Minor suggestion:

>+  virtual already_AddRefed<gfxContext> BeginDrawing(nsIntRegion* aRegionToDraw) = 0;

Maybe make this just return a gfxContext* -- the layer will always hold a ref to it until EndDrawing, so I don't know that we need to enforce that it gets addref/release'd all along the way.

Comment 17

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

That's a good idea.

Comment 18

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

OK, I've done that in my tree.

Comment 19

[Bas Schouten (:bas.schouten)]

I assume nsBaseWidget::GetLayerManager is actually intended to be virtual?

Comment 20

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

It is virtual, since it's virtual in nsIWidget. But it should be marked virtual in nsBaseWidget, so I've done that.

Comment 21

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

I've published my patch queue here:
http://hg.mozilla.org/users/rocallahan_mozilla.com/layers-patches

Comment 22

[Bas Schouten (:bas.schouten)]

I've been looking at the API, and I think BeginTransactionWithTarget has a design problem looking forward to the actual rendering occurring off thread. And where EndTransaction does not per sé equal the actual drawing event occurring. In this case we would want a 'SetTarget' function that can set the target context (or NULL for drawing directly to the associated widget).

Comment 23

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

BeginTransactionWithTarget must render synchronously on the main thread. It could potentially use other threads, but all the rendering must be completed by the time EndTransaction returns. I'll make this clearer in the header file.

Comment 24

[Bas Schouten (:bas.schouten)]

(In reply to comment #23)
> BeginTransactionWithTarget must render synchronously on the main thread. It
> could potentially use other threads, but all the rendering must be completed by
> the time EndTransaction returns. I'll make this clearer in the header file.

Hrm, how would this be preferred to function implementation wise? For example suppose I have my LayerManager, happily rendering the layer tree and some animations from a rendering thread. Then something does a BeginTransactionWithTarget, do I drop the whole normal rendering process and switch to main thread rendering there?

Comment 25

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

How about
1) if the rendering thread is currently rendering, wait for it to finish
2) tell the rendering thread render into an offscreen buffer, and wait for that to finish
3) make the offscreen buffer available as a regular cairo surface
4) paint that surface into the destination cairo context

Comment 26

[Bas Schouten (:bas.schouten)]

Makes sense to me. Another question, I've got the general idea of my D3D backend working. The problem is I'm getting lots of updates to SetVisibleRegion on the single thebes root frame. With small sub-regions of the window. Since the window uses just one root thebes frame, shouldn't the visible region always just be the size of the window?

Comment 27

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

No, we're constructing a new layer tree for each paint, and many paints are only updating a small area of the window, and that's what's visible.

Comment 28

[Bas Schouten (:bas.schouten)]

(In reply to comment #27)
> No, we're constructing a new layer tree for each paint, and many paints are
> only updating a small area of the window, and that's what's visible.

So, this is a bit of a problem for any hardware accelerated implementation. First of all in my opinion the system basically guarantees the knowledge to draw the entire windows is contained within the layer tree. Otherwise it's really still just an immediate system wrapping cairo in a complicated way, and not really retaining everything :-). In hardware you always want to be able to redraw your entire backbuffer when you need to. I use the layer visible region to determine the size of the backing texture for that layer (i.e. the visible region is assumed to be the visible region of that layer, not what happens to be the visible region for that paint). I'm assuming InvalidateRegion is actually what indicates which part of the layer is being redrawn for that paint, but that is never being called.

Basically when I do a render pass of the new layer tree, everything that was there before, is gone, therefor the layers must retain their previous content if we only redraw part of them. The region outside of the redrawn part but still visible, need to be within the visible region in my opinion, that's hte only way it makes sense as well, I think :-). The layer tree is supposed to be a retained structure. If we throw the layer tree out completely on each paint (i.e. we create a new root thebes layer, etc.), without some sort of copy-on-write, hardware resource creation and destruction will cause so much performance overhead the entire exercise becomes pointless on hardware.

Comment 29

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

(In reply to comment #28)
> If we throw the layer tree out completely on each paint
> (i.e. we create a new root thebes layer, etc.), without some sort of
> copy-on-write, hardware resource creation and destruction will cause so much
> performance overhead the entire exercise becomes pointless on hardware.

Right. Building a new layer tree for every paint is only a temporary situation. Soon we will stop doing that, we will reuse parts of the layer tree between paints and start calling InvalidateRegion; the visible region will be the true visible region of the layer in the window; then it starts to make sense to retain content.

I hope that for certain situations like fullscreen video or fullscreen WebGL, even when we're creating new layers on every paint hardware acceleration still wins.

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

For the temporary case, I'd keep the HW surfaces in a cache since you'll have a very high hit rate.. basically cache all the previous' layer tree's surfaces until the next paint, and throw out any surfaces that you didn't use in N consecutive paints or something.

Comment 31

[Bas Schouten (:bas.schouten)]

(In reply to comment #30)
> For the temporary case, I'd keep the HW surfaces in a cache since you'll have a
> very high hit rate.. basically cache all the previous' layer tree's surfaces
> until the next paint, and throw out any surfaces that you didn't use in N
> consecutive paints or something.
Yeah, I was thinking the same thing. Give every layer a texture window size, caching those on the LayerManager, and then compositing only the visible region parts of the textures.

Comment 32

[Bas Schouten (:bas.schouten)]

So, a new issue, my D3D10 backend is now basically working. There is a problem though, which is similar to an issue I've experienced with D2D. GDI will naturally clip anything drawn to the invalidated region of the Window, D3D will not. What happens is that I get set a large visible region, and then my ThebesLayer gets drawn to, on BeginDrawing it will return the entire area that -has- to be redrawn. Which is a rectangular area(due to the texture being rectangular), which encompasses the visible region. This region that needs to redrawn is ignored though, and only the area is redrawn that had to be redrawn in the original invalidation is redrawn. This, needless to say, causes artifacts.

Comment 33

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

A simple option would be to modify OnPaint() so that you restrict the region to be repainted to be a single rectangle.

However, that would be a performance regression. We get a significant performance win in some situations (e.g., when only two small rectangles far apart in the window need to be repainted) by restricting painting to what's in the region, not what's in the bounding-box. Are you saying that's not possible with D3D?

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 419102 [details] [diff] [review]
Part 1: Create Layers API

I think:
+  void SetClipRect(const nsIntRect* aRect)
+  {
+    if (aRect) {
+      mUseClipRect = PR_TRUE;
+      mClipRect = *aRect;
+    } else {
+      mUseClipRect = PR_FALSE;
+    }
+  }

might be clearer than:
+  void SetClipRect(const nsIntRect* aRect)
+  {
+    mUseClipRect = aRect != nsnull;
+    if (aRect) {
+      mClipRect = *aRect;
+    }
+  }

but it doesn't matter much.

Comment 35

[Bas Schouten (:bas.schouten)]

Attachment: Initial experimental D3D10 Layers Implementation

Added my preliminary D3D10 work. Once I iron out some big bugs in my OGL implementation I'll add that as well. Note this is by no means perfect yet :-).

Comment 36

[Bas Schouten (:bas.schouten)]

(In reply to comment #35)
> Created an attachment (id=420094) [details]
> Initial experimental D3D10 Layers Implementation
> 
> Added my preliminary D3D10 work. Once I iron out some big bugs in my OGL
> implementation I'll add that as well. Note this is by no means perfect yet :-).

Added note to that, I haven't added resource releasing yet, so it'll leak like crazy.

Comment 37

[Bas Schouten (:bas.schouten)]

Attachment: Initial experimental OGL Layers Implementation

Added the OGL implementation as well, note this doesn't support CopyTo yet. This also leaks a bit on the layer manager and such. And is in other ways not perfect yet. Note both these backends have issues scrolling(we'll need different scrolling code for the layers system).

Comment 38

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

Can you create new bugs for those backends?

In those bugs, please describe the scrolling issues. Thanks!!!

Comment 39

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 419103 [details] [diff] [review]
Part 2: Create BasicLayers implementation

+  // valid region is empty (since we haven't pained this layer yet),

"pained" is typoed

The tree traversal in AdvancePaintingTo(BasicThebesLayer* aLayer) is sort of tricky to understand.

I tried to come up with something that was better but didn't have great results.
This one factors the begin outside of the conditional.

    while node:
        begin(node)
        if node.child:
            node = node.child
        elif node.sibling:
            node = node.sibling
        else:
            while True:
                end(node)
                node = node.parent
                if not node:
                    return
                if node.sibling:
                    end(node)
                    node = node.sibling
                    break

This one eliminates the inner loop and uses a 'visited' flag to accomplish something similar.

    visited = False
    while node:
        if not visited:
            begin(node)

        if not visited and node.child:
            node = node.child
        elif node.sibling:
            if visited:
                end(node)
            visited = False
            node = node.sibling
        else:
            end(node)
            visited = True
            node = node.parent

Maybe the best thing to do is to just add a comment near the inner loop saying that it moves up the tree until it finds a parent with a sibling.

Why is the tree structure maintained in BasicImplData and not just in the layer?
i.e. there are lot of ToData() calls littering the code...

Comment 40

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

(In reply to comment #39)
> Maybe the best thing to do is to just add a comment near the inner loop saying
> that it moves up the tree until it finds a parent with a sibling.

Yeah. It's just one function, it's short, so the complexity is well-contained, so I'm not too worried about it.

> Why is the tree structure maintained in BasicImplData and not just in the
> layer?
> i.e. there are lot of ToData() calls littering the code...

What do you mean "just in the layer"? The parent and prev/next sibling pointers need to go in a common type, so that's why BasicImplData exists. Then, given a Layer*, we can't just static_cast<BasicImplData*>, because the offset of the BasicImplData subobject is (or at least could be) different depending on which Layer type it occurs in. That's why Layer::mImplData exists, it's a place where we can store the location of the BasicImplData subobject (or any similar data needed by other backends). And of course we have to make mImplData a void*, so we need a cast whenever we use it, which is what ToData encapsulates.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #40)
> (In reply to comment #39)
> > Why is the tree structure maintained in BasicImplData and not just in the
> > layer?
> > i.e. there are lot of ToData() calls littering the code...
> 
> What do you mean "just in the layer"? The parent and prev/next sibling pointers
> need to go in a common type, so that's why BasicImplData exists. Then, given a
> Layer*, we can't just static_cast<BasicImplData*>, because the offset of the
> BasicImplData subobject is (or at least could be) different depending on which
> Layer type it occurs in. That's why Layer::mImplData exists, it's a place where
> we can store the location of the BasicImplData subobject (or any similar data
> needed by other backends). And of course we have to make mImplData a void*, so
> we need a cast whenever we use it, which is what ToData encapsulates.

I meant having sibling and parent pointers as members of "Layer". What are the situations where we want Layer's without these pointers?

Comment 42

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

Ah. That's a good point. I'll do that. We'll still need BasicImplData to store the visible region --- I don't want to commit all backends to having to store an nsIntRegion there (which can be large).

Comment 43

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

by the way, Bas, can you put the D3D layers and the OpenGL layers into their own subdirectories? I'm thinking gfx/layers/d3d and gfx/layers/opengl

Comment 44

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

Attachment: Part 1: Create Layers API v2

I moved GetPrevSibling/GetNextSibling/GetParent up to Layer. I also moved GetFirstChild up to Layer. I have to make SetNextSibling/SetPrevSibling/SetParent public in Layer, so that BasicContainerLayer::InsertAfter can use them.

Comment 45

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

Attachment: Part 2: Create BasicLayers implementation v2

Revised BasicLayers to match.

Comment 46

[Bas Schouten (:bas.schouten)]

Is it perhaps an idea to add a GetType() to the general layer as well? Currently I have this method on my LayerD3D10/LayerOGL class, but I think it would probably be useful for all backends. Since we regularly want to cast to the specific class.

Comment 47

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

I have not needed a public GetType() method so far, and I'd prefer to avoid it until we come up with a definite need for it.

Comment 48

[Bas Schouten (:bas.schouten)]

(In reply to comment #47)
> I have not needed a public GetType() method so far, and I'd prefer to avoid it
> until we come up with a definite need for it.

Fair enough, I'll keep it in my OGL D3D10 implementations :).

Comment 49

[Mats Palmgren (inactive)]

Comment on attachment 419105 [details] [diff] [review]
Part 4: Integrate layers into display list processing

r=mats with this addressed:

In nsDisplayList.cpp:
+CreateEmptyThebesLayer(...
+ ...
+  if (!thebesLayer)
+    return nsnull;

You're leaking |itemGroup| returning there?

Comment 50

[Mats Palmgren (inactive)]

Comment on attachment 419107 [details] [diff] [review]
Part 6: Make LayerManagers the interface used to render into widgets

r=mats with a few nits:

In nsWebBrowser.cpp:
case NS_PAINT: {
+      nsRefPtr<LayerManager> layerManager =
+        widget->GetLayerManager();

Fits on one line?

+      if (root) {
          ...
       }
+      root->EndDrawing();

Should be inside the if-block, right?


In nsLayoutUtils.h:
+   * maanger

typo

+   * we paint by construct a BasicLayerManager and calling

constructing?

In nsWindowGfx.cpp:
+// XXX: we may not receive invalidates when an OS dialog obscures out window

s/out/our/ ?

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 419105 [details] [diff] [review]
Part 4: Integrate layers into display list processing

+   * This also sets mIsOpaque if aVisibleRegion is empty on return.

Presumably it always sets mIsOpaque, but sets it to whether aVisibleRegion
is empty on return.

+  PRBool IsOpaque() {

should be a const method, since nsDisplayList does support const-ness
and has const methods.

+  /**
+   * This class represents a sublist of the items in a display list.
+   * The first item in the sublist is mStartItem and the last item
+   * is the item before mEndItem.
+   * 
+   * These sublists are themselves organized into a linked list of all
+   * the ItemGroups associated with a given layer, via mNextItemsForLayer.
+   * 
+   * These objects are allocated from the nsDisplayListBuilder arena.
+   */

Could you explain when we use linked lists of ItemGroups and what that
means?


sr=dbaron

Comment 53

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

(In reply to comment #52)
> (From update of attachment 419105 [details] [diff] [review])
> +   * This also sets mIsOpaque if aVisibleRegion is empty on return.
> 
> Presumably it always sets mIsOpaque, but sets it to whether aVisibleRegion
> is empty on return.

Right. Will clarify.

> +  PRBool IsOpaque() {
> 
> should be a const method, since nsDisplayList does support const-ness
> and has const methods.

Good point, will do.

> +  /**
> +   * This class represents a sublist of the items in a display list.
> +   * The first item in the sublist is mStartItem and the last item
> +   * is the item before mEndItem.
> +   * 
> +   * These sublists are themselves organized into a linked list of all
> +   * the ItemGroups associated with a given layer, via mNextItemsForLayer.
> +   * 
> +   * These objects are allocated from the nsDisplayListBuilder arena.
> +   */
> 
> Could you explain when we use linked lists of ItemGroups and what that
> means?

Each ItemGroup represents a sublist of consecutive nsDisplayItems in an nsDisplayList. So when display items come from different nsDisplayLists, or when they come from the same nsDisplayList but are not consecutive, we have to use multiple ItemGroups. I'll explain this in the comment.

Comment 54

[Bas Schouten (:bas.schouten)]

Just a small note, license boilerplates should be updated to 2010 probably.

Comment 55

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

Attachment: Part 1: Create Layers API v3

Updated to make the cliprect on a layer apply after the layer has been transformed. This is easier to implement and more useful, e.g. if we're rendering video and the video has been letterboxed using a transform.

Comment 56

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

Oh, that patch also adds an IntersectClipRect API which combines a rectangle with the current clip rect.

Comment 57

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

Attachment: Part 4: Integrate layers into display list processing (v2)

Use IntersectClipRect instead of SetClipRect so that an nsDisplayItem can set its own cliprect on its layer if desired.

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 427053 [details] [diff] [review]
Part 1: Create Layers API v3

+   * CONSTRUCTION PHASE ONLY
+   * Set a clip rect which will be applied to this layer as it is
+   * composited to the destination. The coordinates are relative to
+   * the parent layer, in device pixels (i.e. the contents of this layer
+   * are transformed before this clip rect is applied). The provided rect
+   * is intersected with any existing clip rect.
+   */
+  void IntersectClipRect(const nsIntRect& aRect)

I find comments like this a little confusing sometimes (saying that it's
both in device pixels and that it's transformed); I wonder if there's a
more precise way to say what is meant here (i.e., that the units would
be device pixels if there are no transformations, and therefore users
should convert lengths they have to device pixels and the layer system
will apply the transforms) that isn't too wordy.



Is there a reason that BeginDrawing is on the Layer base class but
EndDrawing is only on ThebesLayer?  (Also that BeginDrawing has two
different signatures?  Would it make more sense to not bother with
virtual methods on the base class, but just say that each layer
implementation defines how to start/end the drawing phase?  Or, if both
calls are needed, maybe the ones on ThebesLayer should be called
BeginThebesDrawing/EndThebesDrawing?

+   * DRAWING PHASE ONLY
...
+   * aSource will make corresponding areas of this layer invalid. You
+   * must not call this after BeginDrawing/EndDrawing on this layer.

I'm confused.  (Maybe related to previous comment?)

sr=dbaron

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 419107 [details] [diff] [review]
Part 6: Make LayerManagers the interface used to render into widgets

>+   * If aFlags contains PAINT_CAN_USE_WIDGET_LAYERS and
>+   * ShouldUseWidgetLayerManager() is set, then we will paint using
>+   * the reference frame's widget's layer manager (and ctx may be null),
>+   * otherwise we will use a temporary BasicLayerManager and ctx must
>+   * not be null.
>+   * 
>    * ComputeVisibility must be called before Paint.
>    */
>-  void Paint(nsDisplayListBuilder* aBuilder, nsIRenderingContext* aCtx) const;
>+  enum {
>+    PAINT_DEFAULT = 0,
>+    PAINT_CAN_USE_WIDGET_LAYERS = 0x01
>+  };

Why is this called PAINT_CAN_USE_WIDGET_LAYERS rather than PAINT_USE_WIDGET_LAYERS?  If it only causes changes in behavior some of the time, could you describe when it does and when it doesn't change behavior?


sr=dbaron

Comment 60

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

(In reply to comment #58)
> (From update of attachment 427053 [details] [diff] [review]) 
> +   * CONSTRUCTION PHASE ONLY
> +   * Set a clip rect which will be applied to this layer as it is
> +   * composited to the destination. The coordinates are relative to
> +   * the parent layer, in device pixels (i.e. the contents of this layer
> +   * are transformed before this clip rect is applied). The provided rect
> +   * is intersected with any existing clip rect.
> +   */
> +  void IntersectClipRect(const nsIntRect& aRect)
> 
> I find comments like this a little confusing sometimes (saying that it's
> both in device pixels and that it's transformed); I wonder if there's a
> more precise way to say what is meant here (i.e., that the units would
> be device pixels if there are no transformations, and therefore users
> should convert lengths they have to device pixels and the layer system
> will apply the transforms) that isn't too wordy.

Yeah, OK.

> Is there a reason that BeginDrawing is on the Layer base class but
> EndDrawing is only on ThebesLayer?  (Also that BeginDrawing has two
> different signatures?  Would it make more sense to not bother with
> virtual methods on the base class, but just say that each layer
> implementation defines how to start/end the drawing phase?  Or, if both
> calls are needed, maybe the ones on ThebesLayer should be called
> BeginThebesDrawing/EndThebesDrawing?

It's accidental. LayerManager::BeginDrawing is a completely different method from ThebesLayer::BeginDrawing. The former means "enter the drawing phase of the current transaction". I'll rename it. How about EnterTransactionDrawingPhase?

> Why is this called PAINT_CAN_USE_WIDGET_LAYERS rather than
> PAINT_USE_WIDGET_LAYERS?  If it only causes changes in behavior some of the
> time, could you describe when it does and when it doesn't change behavior?

I'll rename it to PAINT_USE_WIDGET_LAYERS.

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #60)
> It's accidental. LayerManager::BeginDrawing is a completely different method
> from ThebesLayer::BeginDrawing. The former means "enter the drawing phase of
> the current transaction". I'll rename it. How about
> EnterTransactionDrawingPhase?

That's ok.  Though perhaps EnterDrawingPhase or DoneConstruction might work too?

Comment 62

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

EndConstruction?

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

sure

Comment 64

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

http://hg.mozilla.org/mozilla-central/rev/3784039460d9
http://hg.mozilla.org/mozilla-central/rev/068a5ca443b1
http://hg.mozilla.org/mozilla-central/rev/038a75a355ec
http://hg.mozilla.org/mozilla-central/rev/9958e7a9771e
http://hg.mozilla.org/mozilla-central/rev/97ad975ec651
http://hg.mozilla.org/mozilla-central/rev/c85d57ea1d37
and a couple of bustage fixes for Windows due to merging with D2D stuff:
http://hg.mozilla.org/mozilla-central/rev/d680b1505f9a
http://hg.mozilla.org/mozilla-central/rev/1cfa33c0e178

Comment 65

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

Also
http://hg.mozilla.org/mozilla-central/rev/e7970f6c7cdd

Comment 66

[Alfred Kayser]

Attachment: Screenshot of redraw issue

It seems that these patches causes strange border effects in menu/menulist rendering. When moving the mouse over the menulist from top to botton, dark lines remain/are not cleared. This effect happened before, and it seems to be caused by a redraw rectangle being one pixel too small...

Comment 67

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/mozilla-central/rev/a8c9c6f8190e
Mark Banner - Linux debug bustage fix from bug 534425 for non-FF apps.

http://hg.mozilla.org/mozilla-central/rev/6addf16ed14c
Mark Banner - Hopefully last Windows fix for non-FF apps following bug 534425

***

SeaMonkey test suites are still all broken...

Comment 68

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: uuid, stray "."

The uuid for nsIDOMCanvasRenderingContext2D should've been changed, and a dot was unnecessarily introduced.

Comment 69

[Dão Gottwald [:dao]]

(In reply to comment #67)
> SeaMonkey test suites are still all broken...

Sounds like somebody should file a new bug. Unless you're suggesting this should be backed out, I don't see why you've changed this bug's status.

Comment 70

[Serge Gautherie (:sgautherie)]

(In reply to comment #69)
> Sounds like somebody should file a new bug. Unless you're suggesting this
> should be backed out, I don't see why you've changed this bug's status.

Yes, I implied all this should (have) be backed out ... but now there is an additional "bustage fix" waiting for review...
Why a new bug when this one is obviously not (correctly) fixed?

Comment 71

[Robert Kaiser]

(In reply to comment #70)
> Yes, I implied all this should (have) be backed out ... but now there is an
> additional "bustage fix" waiting for review...

Whoa, don't go too fast on calling for backout of rather large things, better to have it stick and fix the bustage timely.

> Why a new bug when this one is obviously not (correctly) fixed?

Because it's not even 100% clear that it's this bug that causes the problem, and having a separate bug with a separate checkin that fixes things is cleaner usually.

Comment 72

[Serge Gautherie (:sgautherie)]

(In reply to comment #71)

> Whoa, don't go too fast on calling for backout of rather large things, better
> to have it stick and fix the bustage timely.

Sure, but, at the time I noticed, neither author nor sheriff was responding (on irc): this condition is supposed to imply backout, isn't it?
(Note that I did _not_ back it out...)

> Because it's not even 100% clear that it's this bug that causes the problem,
> and having a separate bug with a separate checkin that fixes things is cleaner
> usually.

It's the only (multiple) checkin in the timeframe.
What other cause do you suspect?

Comment 73

[Robert Kaiser]

Filed bug 549390 on the SeaMonkey issue, it's a shared build packaging problem as it seems.

Comment 74

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

(In reply to comment #72)
> Sure, but, at the time I noticed, neither author nor sheriff was responding (on
> irc): this condition is supposed to imply backout, isn't it?
> (Note that I did _not_ back it out...)

Sorry about that. I stayed up long enough to ensure tests were passing on Tinderbox.

Comment 75

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

Alfred, can you file a bug on comment #66? I'll get onto it.

Comment 76

[pal-moz]

sorry if wrong place,
this cause border issue ?
http://forums.mozillazine.org/viewtopic.php?p=8828145#p8828145

Comment 77

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

Probably.

Comment 78

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

We need bugs filed for that and comment #66.

Comment 79

[pal-moz]

(In reply to comment #76)
> sorry if wrong place,
> this cause border issue ?
> http://forums.mozillazine.org/viewtopic.php?p=8828145#p8828145

filed, bug 549622

Comment 80

[Oleg Romashin (:romaxa)]

ups, wrong bug

Comment 81

[Alfred Kayser]

bug 549630 also covers #66

Comment 82

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Rev uuid

r=roc. The dot was already removed in bug 549858.

Comment 83

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to fix painting on winmo

GetUpdateRgn can return an empty region, in which case we want to use the paint rect.

bug 542280 actually broke this and made us use the paint rect every time, this bug reversed that mistake to use the update region every time.

Comment 84

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/1beaf3d5335f

Comment 85

[Marco Bonardo [:mak]]

what is checkin-needed here?

Comment 86

[Dão Gottwald [:dao]]

(In reply to comment #85)
> what is checkin-needed here?

attachment 430684 [details] [diff] [review]

Comment 87

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/82230aecccee