This is my master bug to implement retaining of layer trees and layer contents between paints. A key part of this work is to redesign scrolling to leverage retained layer buffers instead of the platform nsIWidget::Scroll APIs.

The patch queue for this work is here: http://hg.mozilla.org/users/rocallahan_mozilla.com/layers-patches/

We'll need this for retained layers.

I scavenged this from Michael's display-list-based-invalidation work. We'll need it later to identify which parts of which frames ended up in which layers.

See comments. We need this on top of part 3 to associate frame parts with their layers.

Trivial...

Now we start to get to the meaty stuff. The main thing here is that we add a mode flag to BasicLayerManager which lets you turn on the retaining of contents of BasicLayers. Most of the work is delegated to ThebesLayerBuffer, which is somewhat tricky and evil because of the optimizations we do to avoid self-copies when scrolling, but that's hidden behind a clean interface. We also add API to allow ThebesLayers to propagate out "aRegionToInvalidate", so they can suddenly decide to switch depths or transparency and rerender the entire layer, and we'll invalidate the window appropriately.

Later on we will sometimes detect during painting that areas outside the current paint area need to be invalidated (e.g. if we detect that a layer has become completely opaque, we might change it from RGBA to RGB and rerender with subpixel-AA everywhere). It's convenient to implement that by issuing normal invalidates during painting; we just need to defer those invalidates so they don't happen synchronously during paint (which can be very bad, depending on platform). We also want to throw out invalidates for areas which are currently being painted, since they're being updated already. This patch creates frame API we can use to do this. The patch also refactors PresShell::Paint a bit. We don't need to pass mOffsetToRoot in PaintParams, we can just pre-apply the offset to the region.

Comment on attachment 451465 [details] [diff] [review] Part 2: Create retained layer API: methods on FrameLayerBuilder that frames can use to access, reuse and invalidate retained layers >Bug 564991. Part 2: Create retained layer API: methods on FrameLayerBuilder that frames can use to access, reuse and invalidate retained layers. r=mats,sr=vlad I believe this needs to destroy any retained layer for the canvas when SetDimensions is called -- this can (should?) happen in nsHTMLCanvasElement instead of in each context. The size of the canvas is part of the CanvasLayer's data when it's created, so if it ever gets resized, we need to recreate the layer. The rest of the patch looks fine, though.

Comment on attachment 451481 [details] [diff] [review] Part 8: Hoist GetLayerType out of individual layer manager types and make it available for all layers. Seems like a good thing to do.

Comment on attachment 451478 [details] [diff] [review] Part 6: Make BasicLayers support retained ThebesLayer contents. >Bug 564991. Part 6: Make BasicLayers support retained ThebesLayer contents. r=cjones,sr=vlad Looks fine, just two general code comments: - I like the ThebesLayerBuffer; one minor comment is that talking about "rotation" in there is a little confusing, given that we're in a graphics context. I understand how you're using the term, but maybe "buffer offset"? Not much better, similar confusion I guess. - The comments refer to "aXQuadrant", but you use aXSide. 0 and 1 are a little confusing there, -1 and 1 would make more sense to me if they referred to the direction with the rotation point at the center. Another option might be to define TOP_LEFT, TOP_RIGHT, etc. and use those instead of magic 0's and 1's.

(In reply to comment #11) > (From update of attachment 451465 [details] [diff] [review]) > >Bug 564991. Part 2: Create retained layer API: methods on FrameLayerBuilder that frames can use to access, reuse and invalidate retained layers. r=mats,sr=vlad > > I believe this needs to destroy any retained layer for the canvas when > SetDimensions is called -- this can (should?) happen in nsHTMLCanvasElement > instead of in each context. The size of the canvas is part of the > CanvasLayer's data when it's created, so if it ever gets resized, we need to > recreate the layer. We don't destroy layers from here. Instead we need to check whether the old layer can be reused, and create a new layer if we can't. I'll do that, thanks for the catch. I think we need a test for resizing canvases though. (In reply to comment #13) > - I like the ThebesLayerBuffer; one minor comment is that talking about > "rotation" in there is a little confusing, given that we're in a graphics > context. I understand how you're using the term, but maybe "buffer offset"? > Not much better, similar confusion I guess. If you give me a better term, I'll use it :-). > - The comments refer to "aXQuadrant", but you use aXSide. 0 and 1 are a little > confusing there, -1 and 1 would make more sense to me if they referred to the > direction with the rotation point at the center. Another option might be to > define TOP_LEFT, TOP_RIGHT, etc. and use those instead of magic 0's and 1's. I'll fix that. Thanks.

Fixed to avoid reusing an old layer if the canvas was reinitialized.

This is the core of the entire patch set. I've added quite a lot of comments to FrameLayerBuilder to try to explain how it works. Note that with this patch we don't actually retain layers yet because we don't call BeginUpdatingRetainedLayers.

Actually turn on part 10. Note that this means we have to start setting the "visible region" we use to compute display lists to the entire window, otherwise we chop off parts of the layer tree!

We were getting code duplication between nsPresShell::Paint/nsLayoutUtils::PaintFrame on one hand and nsPresShell::RenderDocument on the other hand. This fixes that.

Part 12 also enables the use of the retained layer tree by drawWindow, so that reftests start picking up the retained layer tree.

With part 9, whenever we invalidate part of a frame, we cause the contents of all ThebesLayer children of the frame's nearest container layer to be invalidated in that region. With this patch, we add an InvalidateLayer API and an nsChangeHint_UpdateLayer style change hint, and we use it when invalidating video, canvas and changes in 'opacity' from one fractional value to another. With this patch, and retained layer contents turned on (later patch), scripted fades using 'opacity' do not need to repaint any ThebesLayer content at all!

Basically we just rip out everything we used to scroll before and invalidate the scrolled area instead. That can still be efficient because we just need to recomposite our layer tree instead of doing a lot of genuine rendering. This makes the repaint for scrolling asynchronous, which is great, because it can coalesce with other painting. It's also good for scripts that like to scroll a lot. We'll even be able to throttle scroll painting with the refresh driver when painting gets hooked up there. The patch introduces an "active" flag for scrolled elements. With this patch, an element is active if it's the root scroll frame for a toplevel content/chrome window, or if it's ever been scrolled. Part 9 makes sure that ThebesLayers contain items that all have the same nearest active scrolled root. That means we can scroll just by bumping the layer transform and updating visible regions; Part 9 already does that. There's more code to rip out yet. nsDisplayList still has some cruft left over from scroll display list analysis. nsIWidget::Scroll is mostly unused now ... XUL trees still use it, but I'll rip that out in a later patch.

Flip the switch.

The scrollbars for a window cause a wee bit of a problem ... they form a non-moving layer that's the size of the window, but almost entirely empty. We can save a lot of time and memory by putting each scroll part in its own layer, so we get 1-3 small layers instead of one really big layer. This patch also marks Cocoa scrollbars as being opaque. This will help performance because then we know those layers are opaque. Windows scrollbars already have this right, and Karl has a patch in bug 571104 to fix this for GTK2.

Astute readers will have noticed that Part 9 removed support for generating ColorLayers instead of single-color ThebesLayers. This patch re-adds that support. We track in ThebesLayerData whether the layer is a solid color rectangle. If it still is when we've finished adding items to it, then we create a ColorLayer. We don't destroy the ThebesLayer --- that would result in unnecessary layer churn every time we update the layer tree. Instead we just make the ThebesLayer disappear by clipping it away and setting its visible region to empty.

We can do some good optimizations on Windows and X where we're responsible for double-buffering. Basically, if the layer tree is such that the dirty area only intersects a single layer, blitting that layer to the window can't cause flicker, so we don't need to do extra double-buffering. To lay the groundwork for that optimization, first we move double-buffering into the layer manager. We create an API that lets clients request double-buffering from BasicLayerManager.

Matt Woodrow measured a significant performance win from this. (This win could have been had on trunk without all the rest of the layers stuff!)

We can avoid PushGroup for a layer with opacity if it has only one leaf descendant. This is fairly straightforward. This avoids a temporary surface for the common case where a single ThebesLayer has opacity set on it.

Finish the optimization described in Part 19.

Active scrolled elements that are transparent, e.g. the anonymous DIV in a <textarea>, tend to have non-subpixel-AA text in them, which doesn't look good. They also make compositing the window more expensive because we have an extra layer to composite. So this patch waits for 300-400ms after scrolling and then marks the scrolled element inactive. We can fiddle with the timeout based on testing, but this value seems to work pretty well ... on my Mac at least, it's hard to see that we ever disabled subpixel AA, because you can't get a good look at the text while it's moving...

Comment on attachment 451478 [details] [diff] [review] Part 6: Make BasicLayers support retained ThebesLayer contents. >diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h >--- a/gfx/layers/Layers.h >+++ b/gfx/layers/Layers.h >@@ -142,27 +142,33 @@ public: >+ * aRegionToInvalidate contains a region whose contents have been >+ * changed by the layer manager and which must therefore be invalidated. >+ * For example, this would be non-empty if >+ * Looks like this comment is >diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in >--- a/gfx/layers/Makefile.in >+++ b/gfx/layers/Makefile.in >@@ -63,22 +63,23 @@ EXPORTS = \ > Layers.h \ > LayerManagerOGL.h \ > LayerManagerOGLProgram.h \ > $(NULL) > > CPPSRCS = \ > BasicImages.cpp \ > BasicLayers.cpp \ >- LayerManagerOGL.cpp \ >+ ThebesLayerBuffer.cpp \ >+ CanvasLayerOGL.cpp \ > ColorLayerOGL.cpp \ >- ThebesLayerOGL.cpp \ > ContainerLayerOGL.cpp \ > ImageLayerOGL.cpp \ >- CanvasLayerOGL.cpp \ >+ LayerManagerOGL.cpp \ >+ ThebesLayerOGL.cpp \ > $(NULL) > ^^ (There's whitespace here that emacs highlights. It's not your fault, but it's been bothering for a while, so I wouldn't complain if you fixed it :) ). >diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp >--- a/gfx/layers/basic/BasicLayers.cpp >+++ b/gfx/layers/basic/BasicLayers.cpp >@@ -233,40 +251,78 @@ public: > void > BasicThebesLayer::Paint(gfxContext* aContext, > LayerManager::DrawThebesLayerCallback aCallback, > void* aCallbackData) > { > NS_ASSERTION(BasicManager()->InDrawing(), > "Can only draw in drawing phase"); > gfxContext* target = BasicManager()->GetTarget(); > NS_ASSERTION(target, "We shouldn't be called if there's no target"); > >- aCallback(this, target, mVisibleRegion, aCallbackData); >+ // We update mValidRegion now to represent what will be valid in >+ // our buffer after EndDrawing. This comment confuses me slightly given the different implementations. Maybe drop, or edit to something like "After Paint() returns, mValidRegion represents what's valid in our buffer"? Not a big deal regardless. >@@ -275,17 +331,17 @@ public: > { > MOZ_COUNT_DTOR(BasicImageLayer); > } > > virtual void SetVisibleRegion(const nsIntRegion& aRegion) > { > NS_ASSERTION(BasicManager()->InConstruction(), > "Can only set properties in construction phase"); >- mVisibleRegion = aRegion; >+ ImageLayer::SetVisibleRegion(aRegion); > } > Do you mind fixing this in BasicColorLayer too, while you're at it? >diff --git a/gfx/layers/basic/ThebesLayerBuffer.cpp b/gfx/layers/basic/ThebesLayerBuffer.cpp >new file mode 100644 >--- /dev/null >+++ b/gfx/layers/basic/ThebesLayerBuffer.cpp I agree with vlad re: enum instead of xside/yside throughout. >+nsIntRect >+ThebesLayerBuffer::GetQuadrantRectangle(PRInt32 aXSide, PRInt32 aYSide) >+{ >+ // quadrantTranslation is the amount we translate the top-left >+ // of the quadrant by to get coordinates relative to the layer >+ nsIntPoint quadrantTranslation = -mBufferRotation; >+ quadrantTranslation.x += aXSide == 0 ? mBufferRect.width : 0; >+ quadrantTranslation.y += aYSide == 0 ? mBufferRect.height : 0; >+ return mBufferRect + quadrantTranslation; I think it's a little odd to return mBufferRect + quadrantTranslation (I can't think of an independent interpretation of it), but it works well for the fill below. IMHO |QuadrantRectangleInLayerSpace()| is a little clearer, since there's also a "quandrant rectangle in buffer space [<0,0>, <width, height>)", but feel free to ignore me :).

I realized Part 13 can be improved (and needs to be to fix some bugs with later patches). We pass to InvalidateLayer a display item key to identify exactly what part is being invalidated. Then, if there is currently a real retained layer for that part, we don't need to invalidate any ThebesLayers, otherwise we do. This actually simplifies things a bit since InvalidateLayer doesn't need to check to see whether we're in a container that would have forced non-retained layers. It does mean we have to have a change hint for every type of container layer, so that nsCSSFrameConstructor knows which display item type to invalidate. Asking dbaron for sr due to the style system change.

In many situations we have content in layers that's very static, e.g. Slashdot where some of the headings use CSS opacity. In this case it's not good to have the content in its own layer, because that uses more memory and requires us to recomposite the layer every time we paint the window, e.g. while scrolling the page. However, we don't want to have two rendering paths where sometimes we need to paint opacity and other effects through the layer system and sometimes we paint directly ... that will result in code duplication. So we introduce the concept of "inactive" layers. The idea is that if a display item creates an inactive layer, FrameLayerBuilder will treat it like any other display item that just draws using Thebes, assigning it to a ThebesLayer. However, instead of calling Paint on that display item, we create a temporary BasicLayerManager, create a layer for the display item, make that layer the root layer of the temporary layer manager, and use that temporary layer manager to render the inactive layer into the ThebesLayer. One wrinkle is that a container layer can't be inactive if it has active layer children. E.g. an opacity layer with an accelerated video child needs to be accelerated itself. However, FrameLayerBuilder needs to construct the container before it constructs the child layers. So we need to add a GetLayerState method to nsDisplayItems so we can determine if an item will construct an active layer before we actually construct the layer. The policy for whether a display item's layer should be active or not is determined by the display item, it is allowed to change dynamically, and it is allowed to depend on the type of the retained layer manager (although I don't currently use that).

If the video is not "potentially playing" (i.e. it's paused, we have no data to play, or we've reached the end of the video), make its layer inactive so it's composited into the ThebesLayer content around it. So a page with lots of paused videos will scroll and repaint efficiently. (We won't save memory in general because the video frame needs to be stored anyway.)

(In reply to comment #31) > Looks like this comment is Fixed > (There's whitespace here that emacs highlights. It's not your fault, > but it's been bothering for a while, so I wouldn't complain if you > fixed it :) ). Fixed > This comment confuses me slightly given the different implementations. > Maybe drop, or edit to something like "After Paint() returns, > mValidRegion represents what's valid in our buffer"? Not a big deal > regardless. Dropped > Do you mind fixing this in BasicColorLayer too, while you're at it? Done > I agree with vlad re: enum instead of xside/yside throughout. Fixed > I think it's a little odd to return mBufferRect + quadrantTranslation > (I can't think of an independent interpretation of it), but it works > well for the fill below. IMHO |QuadrantRectangleInLayerSpace()| is a > little clearer, since there's also a "quandrant rectangle in buffer > space [<0,0>, <width, height>)", but feel free to ignore me :). I did :-)

Comment on attachment 451847 [details] [diff] [review] Part 18: Move double-buffering into the layer manager. Looks fine, though: >+ * >+ * aDoubleBuffering can request double-buffering when drawing to the >+ * default target. When not NONE, the layer manager avoids blitting >+ * temporary results to aContext and then overpainting them with final >+ * results. BUFFER_OPAQUE promises that the clipbox of aContext will be >+ * filled with opaque data by the layer tree. > */ >- void SetDefaultTarget(gfxContext* aContext); >+ enum BufferMode { >+ BUFFER_NONE, >+ BUFFER_OPAQUE, >+ BUFFER_TRANSPARENT >+ }; This description and BUFFER_TRANSPARENT is weird, because the buffer is always painted with OPERATOR_SOURCE. So no matter what, the clipbox has to be fully rendered.. so maybe BUFFER_PRESERVE_ALPHA ? And maybe the comment should state that BUFFER_OPAQUE means that the alpha channel can be ignored for buffering purposes, or something to that effect. (I'm basically concerned about the confusion between, say, buffering for a translucent div or something where we'll composite it to the destination, vs. overwriting the destination.)

Comment on attachment 451850 [details] [diff] [review] Part 20: Performance win by avoiding PushGroup for single-layer opacity. Not super excited about passing the opacity around everywhere, but I don't see a way around it.

Separate TRANSPARENT and OPAQUE buffering modes are not actually needed. We can do the right thing just by checking the content type of the target surface.

Comment on attachment 452120 [details] [diff] [review] Part 18 v2 Even better.

I forgot to mention it before, but in part 23 I made 'opacity' layers always inactive unless they contain an active layer (e.g. a playing video). This patch introduces a flag you can set on frames to mark them as needing active layers. The flag times out automatically after 300-400ms. I use this flag for 'opacity' layers and mark the frame active every time the opacity style changes. This means, for example, that during fade effects we'll keep the layer active and we won't need to redraw Thebes content. We can use this flag for other things. E.g. we could use it for canvas, to make the canvas layer inactive if we haven't drawn into the canvas for a while. I don't have a patch for that yet though, it can be followup.

This makes the active scrolling root used for background-attachment:fixed backgrounds that cover the viewport be the viewport, not the scrolled canvas. This means those backgrounds get their own layer, and the scrolled content is in one or more layers above the background layer. That means we can scroll mostly by just recompositing. This makes scrolling with background-attachment:fixed super fast in common cases.

One challenge with the new scrolling setup is to make the movement of plugin windows while scrolling happen as close as possible to the moment when we blit the scrolled Web content to the screen. With the scrolling repaint happening asynchronously, there's a significant gap between synchronously moving windows during nsGfxScrollFrame::ScrollVisual and the Web content visibly moving; this makes plugins seem to be jumping ahead of the content as you scroll smoothly. This patch fixes the problem by making plugin window motion asynchronous; in fact, we try to make plugin window motion always happen at the same time as painting. The best results are obtained when we move the plugin window immediately *after* painting the Web content to the screen; doing it before painting gives a greater delay, since painting takes some time before we do the final blit. (Moving the plugin window *during* painting is not really possible since Windows and probably other platforms get confused.) So I introduce an NS_DID_PAINT event analogous to NS_WILL_PAINT; this fires as soon as possible after painting, synchronously, and it's safe to move windows during this event. We also guarantee that if invalidation is triggered during NS_DID_PAINT, we'll process that invalidation synchronously right there. So the plan for plugins is that immediately after painting Web content to the screen, we fire NS_DID_PAINT, which reaches PresShell::DidPaint, then we'll update the plugin window geometry. Typically that will cause some more invalidation because a strip on the trailing edge of the plugin will need to be repainted, so the paint code that fired DID_PAINT will reenter itself to repaint that strip synchronously. This extra repaint has a slight performance penalty but thanks to retained layers it should remain only a small penalty, we should just be compositing out of the layer buffers. Not all platforms can fire a synchronous DID_PAINT event during which we can move widgets. Mac can't, but that's OK because plugins are fine on Mac in general. Still, we need to make sure we update the plugin geometry at some point, so I've added a willSendDidPaint flag to paint events to indicate whether layout can wait for DID_PAINT or needs to do its work in WILL_PAINT. There's one other hard case: if a plugin is covering the window completely, we might not ever get any paint events, but we may still need to update geometry. So we fire an XPCOM event to force geometry updates in case no paint event arrives. The actual forcing is done by triggering a synchronous paint; that means if the plugin window is not completely covering our window, we'll do a paint and we'll keep the plugin window motion in sync with our content as discussed above. If after the synchronous paint we still haven't updated plugin geometry (because the plugin window really is completely covering our window, or maybe our window is hidden or something), then we'll do the plugin geometry update explicitly. Note that we need to update plugin geometry for Flush_Layout, since tests depend on coordinate conversion giving correct results after the plugin has been moved by a reflow. It's entirely possible that Web content depends on this too.

UL trees currently call nsIWidget::Scroll to scroll themselves. This is a grotesque hack since it assumes there is no overlaying content, no clipping, no effects, etc applying to the tree. But with retained layers it's even worse since the contents of ThebesLayers containing trees need to be updated, but they aren't. (The situation will be even worse when we move to GL/D3D layer managers for everything, since nsIWidget::Scroll doesn't work there.) So this patch just makes us repaint instead of trying to do anything clever while scrolling. I hope that XUL trees can paint fast enough for this to not matter. If we need to rescue accelerated scrolling of XUL trees we probably can, but it will require some hackery. XUL trees would probably need to use a custom ThebesLayer for the tree body. I'd rather not do it if we don't need to, especially since in the long term I'd like to XUL trees to be implemented as pure JS on top of APIs that make tree-style "virtual widgets" work well. This removes the last use of nsIWidget::Scroll, which will let us remove lots of code. See bug 572900 for cruft-removal patches that depend on this bug.

That's all I have for now! There are still a few test failures that I'm working on.

Comment on attachment 452135 [details] [diff] [review] Part 27: Make plugin geometry changes asynchronous and make them happen as close to the final paint as possible This looks ok to me, though one thing I'm not clear on -- RequestUpdatePluginGeometry can be called from a few different places, but the patch only stores one root frame to find updates for. Can we ever get into the situation where the update geometry request comes in from another source before an existing one has been processed?

Yes. In RequestUpdatePluginGeometry, if there is a non-null mUpdatePluginGeometryForFrame and it's not equal to aFrame, we set mUpdatePluginGeometryForFrame to null to indicate we should just scan all frames.

Comment on attachment 452138 [details] [diff] [review] Part 28: Make XUL trees repaint everything when they scroll Should be ok. Can or did we test scrolling performance on a Thunderbird message list?

I haven't. I suppose someone should. Maybe I'll just check that patch into trunk and see what happens...

Comment on attachment 452138 [details] [diff] [review] Part 28: Make XUL trees repaint everything when they scroll Moved this patch to bug 573991 so we can track it separately

Comment on attachment 453551 [details] [diff] [review] Part 29: Sort plugins so they get moved in the best order Now that we're not calling nsIWidget::Scroll, all the fancy logic in those widget implementations to move plugins intelligently is no longer used. But we can easily replicate the essential stuff here. In fact it's even better than before because we'll move plugins intelligently when we paint after a reflow, as well as after scrolling.

A couple of fixes here from the previous version of the patch: -- In FrameLayerBuilder::InternalDestroyDisplayItemData, ensure that the LayerManager is released *after* we've destroyed the DisplayItemData array, since that will release layers and the layers might want their layer manager around for cleanup (thanks cjones) -- In FrameLayerBuilder::DrawThebesLayers, we need to repeat our visibility analysis to figure out which display items, and which parts of display items, need to be painted into the ThebesLayer's aRegionToDraw. The ThebesLayer might ask us to paint into regions that aren't actually visible on the screen and we must comply (because that content might become visible later due to scrolling, for example). This can also result in *less* painting because the ThebesLayer might have cached buffer data that's visible in the window but it's not asking us to draw.

Fix bug where we were calling gdk_window_get_update_area to see if there was anything to repaint, but that actually clears out the update area. We need to re-invalidate the update area we got.

Updated ... there was a bug in nsGfxScrollFrame's LayoutAndInvalidate where we invalidate scrollbars after reflowing a scrollframe. We were invalidating the area of the scrollbar in the scrollframe, rather than invalidating the scrollbar itself, because scrollframes suppress invalidates coming up from hidden scrollbars. But when scrollbars have their own layers we need to invalidate the scrollbar to ensure the layer gets updated.

Comment on attachment 451480 [details] [diff] [review] Part 7: Create Begin/EndDeferredInvalidates API so we can catch and defer invalidates on frames (and suppress certain areas completely) > if (frame && aViewToPaint == aDisplayRoot) { >+ // Defer invalidates that are triggered during painting, and discard >+ // invalidates of areas that are already being repainted. >+ frame->BeginDeferringInvalidatesForDisplayRoot(aDirtyRegion); Boy, I'm a little curious what triggers invalidates during painting. > /** >+ * For frames that are display roots, this causes all invalidates What does this mean exactly? Does it mean it can only be called on frames that are display roots, or that it does nothing if the frame on which it's called is not a display root? And what's a display root, anyway? (I think the comment ought to answer these questions.) Anyway, the API seems fine; I didn't look closely at the rest. sr=dbaron

Comment on attachment 451902 [details] [diff] [review] Part 13 v2: Optimize invalidation to avoid repainting ThebesLayer contents sometimes >diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp >+/* static */ >+PRBool >+FrameLayerBuilder::HasDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey) >+{ >+ void* propValue = aFrame->Properties().Get(DisplayItemDataProperty()); >+ if (!propValue) >+ return PR_FALSE; >+ >+ nsTArray<DisplayItemData>* array = >+ (reinterpret_cast<nsTArray<DisplayItemData>*>(&propValue)); You need a PR_STATIC_ASSERT(sizeof(nsTArray<DisplayItemData> == sizeof(void*)) in patch 9! >diff --git a/layout/base/nsChangeHint.h b/layout/base/nsChangeHint.h >--- a/layout/base/nsChangeHint.h >+++ b/layout/base/nsChangeHint.h >@@ -132,16 +138,18 @@ inline PRBool NS_IsHintSubset(nsChangeHi > nsChangeHint(nsChangeHint_NeedReflow | \ > nsChangeHint_ClearAncestorIntrinsics | \ > nsChangeHint_ClearDescendantIntrinsics | \ > nsChangeHint_NeedDirtyReflow) > #define NS_STYLE_HINT_REFLOW \ > nsChangeHint(NS_STYLE_HINT_VISUAL | nsChangeHint_ReflowFrame) > #define NS_STYLE_HINT_FRAMECHANGE \ > nsChangeHint(NS_STYLE_HINT_REFLOW | nsChangeHint_ReconstructFrame) >+#define NS_STYLE_HINT_UPDATELAYER \ >+ nsChangeHint_UpdateOpacityLayer Please don't introduce this; just use nsChangeHint_UpdateOpacityLayer in the one place where you use this. >diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h > /** >+ * As Invalidate above, except that the rendering that has changed Does "except that" mean "except use this function when" ? >+ * is performed using layers so we can avoid updating the contents of >+ * ThebesLayers in most cases. >+ * @param aDisplayItemKey must not be zero; indicates the kind of display >+ * item that is being invalidated. The implementation should assert that it's not zero. (But do you really mean TYPE_GENERIC? Should this parameter be an nsDisplayItem::Type?) >diff --git a/layout/style/nsStyleContext.cpp b/layout/style/nsStyleContext.cpp >--- a/layout/style/nsStyleContext.cpp >+++ b/layout/style/nsStyleContext.cpp > // We begin by examining those style structs that are capable of > // causing the maximal difference, a FRAMECHANGE. > // FRAMECHANGE Structs: Display, XUL, Content, UserInterface, > // Visibility, Outline, TableBorder, Table, Text, UIReset, Quotes >+ nsChangeHint maxHint = nsChangeHint(NS_STYLE_HINT_FRAMECHANGE | >+ nsChangeHint_UpdateOpacityLayer | nsChangeHint_UpdateCursor); You don't need UpdateCursor in here. (There's no need anymore that the maxHint be in decreasing order, since we examine all structs.) sr=dbaron with that

Comment on attachment 451468 [details] [diff] [review] Part 3: Create unique nsDisplayItem types for every single display item. >+++ b/layout/base/nsDisplayItemTypes.h >+enum Type { >+ TYPE_NULL, >+ TYPE_MAX Do we want to have a null type? I would think we want to enforce every item to have its own type. Also, do we need TYPE_MAX here? >+++ a/layout/base/nsLayoutDebugger.cpp >- if (i->GetType() == nsDisplayItem::TYPE_TRANSFORM) { >- nsDisplayTransform* t = static_cast<nsDisplayTransform*>(i); >- PrintDisplayListTo(aBuilder, *(t->GetStoredList()->GetList()), aIndent + 4, aOutput); >- } nsDisplayTransform's still don't return their list from GetList(), so I think this should stay. >+++ b/layout/generic/nsHTMLContainerFrame.cpp >+ // Create our shadow surface, then paint the text decorations onto it >+ nsContextBoxBlur contextBoxBlur; >+ gfxContext* shadowCtx = contextBoxBlur.Init(shadowRect, shadow->mRadius, >+ presContext->AppUnitsPerDevPixel(), >+ thebesCtx, mVisibleRect, nsnull); >+ if (!shadowCtx) >+ continue; >+ >+ thebesCtx->Save(); >+ thebesCtx->NewPath(); >+ thebesCtx->SetColor(gfxRGBA(shadowColor)); >+ The comments for nsContextBoxBlur say that the color should be set on the context before calling Init. A 30 second look at the code indicates that maybe this is no longer true. Either update the nsContextBoxBlur comment, or follow it. I realize this problem was present in the old code too. There are some 80 char violations in this patch; I only mention this because it is not your patch and you may want to correct them.

Comment on attachment 451469 [details] [diff] [review] Part 4: Create GetFrameKey API >+ * If this returns a non-zero value, then pairing this with the the >+ * GetUnderlyingFrame() pointer gives a key that uniquely identifies >+ * this display item in the display item tree. >+ * This will only return a zero value for items which wrap display lists >+ * and do not create a CSS stacking context, therefore requiring >+ * display items to be individually wrapped --- currently nsDisplayClip >+ * only. Ok, so this is why we need TYPE_NULL. Can we set the first item in the enum equal to 1 instead? Or at least add a comment about why we need the TYPE_NULL in the enum.

Comment on attachment 451469 [details] [diff] [review] Part 4: Create GetFrameKey API >+ virtual PRUint32 GetFrameKey() { return PRUint32(GetType()); } The name FrameKey seems to imply it has something to do with the frame, when in fact it is based purely on the display item. I can't think of anything better though.

Comment on attachment 451470 [details] [diff] [review] Part 5: Make GetClipRect return a const reference >- nsRect GetClipRect() { return mClip; } >+ const nsRect& GetClipRect() { return mClip; } Is this important for something? Or just an oversight you wanted to correct?

Comment on attachment 453551 [details] [diff] [review] Part 29: Sort plugins so they get moved in the best order >+ * Given a list of plugin windows to move to new locations, sort the list to >+ * so that for each window move, the window moves to a location that -to- >+ for (i = 0; i < pluginsToMove.Length() - 1; ++i) { "i + 1 < pluginsToMove.Length()" for the unsigned arithmetic. I know there's a !pluginsToMove.IsEmpty() check but this way we don't need to check that pluginsToMove is not modified. >+ nsIWidget::Configuration* config = &pluginsToMove[i]; >+ PRBool foundOverlap = PR_FALSE; >+ for (PRUint32 j = 0; j < aConfigurations->Length(); ++j) { >+ if (HasOverlap(aConfigurations->ElementAt(j).mBounds.TopLeft(), >+ aConfigurations->ElementAt(j).mClipRegion, >+ config->mBounds.TopLeft(), >+ config->mClipRegion)) { >+ foundOverlap = PR_TRUE; >+ break; >+ } >+ } >+ if (foundOverlap) >+ continue; I don't understand why the candidate is being compared against already-moved plugins. If the candidate is going to hit the already-moved plugin now, it will still hit it later. (Though I thought the destination regions would be non-overlapping - is z-order involved?) Seems that this loop is not necessary. >+static PRBool >+HasOverlap(const nsIntPoint& aOffset1, const nsTArray<nsIntRect>& aClipRects1, >+ const nsIntPoint& aOffset2, const nsTArray<nsIntRect>& aClipRects2) >+{ >+ nsIntPoint offsetDelta = aOffset1 - aOffset2; >+ for (PRUint32 i = 0; i < aClipRects1.Length(); ++i) { >+ for (PRUint32 j = 0; j < aClipRects2.Length(); ++j) { >+ if ((aClipRects1[i] + offsetDelta).Intersects(aClipRects2[j])) >+ return PR_TRUE; >+ } >+ } Probably just comparing clip bounding rects would have been sufficient, but I understand that would probably actually be more code, so this is fine until its an issue. >+ * We need to take into account that windows are clipped to particular >+ * regions and the clip regions change as the windows are moved. Only the intersection region of initial and final clips really needs to considered. Portions not in final won't be visible anyway, and portions not in initial will need to be painted anyway, but again, probably don't need to make that optimization. >+ pluginsToMove[j].mChild->GetWindowClipRegion(&clipRects); However, I suspect that fetching the initial clip region is not necessary, and just using the final pluginsToMove[j].mClipRegion would work just as well. Would it make sense to add a comment in the nsIWidget::ConfigureChildren docs to indicate that children should be moved in the order provided? Do you know whether the test passes on other platforms now?

Comment on attachment 451469 [details] [diff] [review] Part 4: Create GetFrameKey API s/the the/the/

Comment on attachment 451820 [details] [diff] [review] Part 12: Refactor RenderDocument to use nsLayoutUtils::PaintFrame In layout/base/nsPresShell.cpp: >+ nsIFrame* rootFrame = FrameManager()->GetRootFrame(); Move this to just before "if (!rootFrame)" ?

There's a few compile warnings that looks relevant (I don't know which part they came from): layout/base/nsDisplayList.cpp:1251: warning: unused variable 'isMoving' layout/base/nsLayoutUtils.cpp:1049: warning: 'gDumpRepaintRegionForCopy' defined but not used layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:4140: warning: unused variable 'delta'

(In reply to comment #62) > Only the intersection region of initial and final clips really needs to > considered. Portions not in final won't be visible anyway, and portions not > in initial will need to be painted anyway, but again, probably don't need to > make that optimization. > > >+ pluginsToMove[j].mChild->GetWindowClipRegion(&clipRects); > > However, I suspect that fetching the initial clip region is not necessary, and > just using the final pluginsToMove[j].mClipRegion would work just as well. That all assumes that ConfigureChildren is clipping all children to the intersection before moving any, which gtk2/nsWindow's Scroll was doing, but ConfigureChildren is not, so feel free to leave this as is, if you like.

I finally got my patch queue through tryserver with no test failures! Performance test results are hard to interpret but retained layers *appears* to be a wash performance-wise, except for clear wins on Tscroll (unsurprisingly) and possibly a Twinopen regression on Mac. I'll look into the latter. (In reply to comment #56) > (From update of attachment 451480 [details] [diff] [review]) > > if (frame && aViewToPaint == aDisplayRoot) { > >+ // Defer invalidates that are triggered during painting, and discard > >+ // invalidates of areas that are already being repainted. > >+ frame->BeginDeferringInvalidatesForDisplayRoot(aDirtyRegion); > > Boy, I'm a little curious what triggers invalidates during painting. FrameLayerBuilder does. For example during painting we might detect that a layer has become completely opaque, in which case we will rerender the entire layer since subpixel-AA might be enabled now. That might update areas that aren't currently being painted. I updated the comment. > > /** > >+ * For frames that are display roots, this causes all invalidates > > What does this mean exactly? Does it mean it can only be called on frames > that are display roots, or that it does nothing if the frame on which it's > called is not a display root? And what's a display root, anyway? (I think > the comment ought to answer these questions.) A display root is the root frame of a root prescontext, or a popup frame. Basically, a frame with an associated widget that is the root of a frame subtree that gets rendered together. See nsLayoutUtils::GetDisplayRootFrame. It's a concept we've had in the view system for a while, but it's migrating out. This function should simply not be called on non-displayroots, so I improved the comment and added assertions to check that. (In reply to comment #57) > You need a PR_STATIC_ASSERT(sizeof(nsTArray<DisplayItemData> == sizeof(void*)) > in patch 9! Done. > >+#define NS_STYLE_HINT_UPDATELAYER \ > >+ nsChangeHint_UpdateOpacityLayer > > Please don't introduce this; just use nsChangeHint_UpdateOpacityLayer in the > one place where you use this. Done. We might want to reintroduce it later when we have more styles that update other kinds of layers (e.g. CSS transforms and filters). > >+ * As Invalidate above, except that the rendering that has changed > > Does "except that" mean "except use this function when" ? Yes. Fixed comment. > >+ * @param aDisplayItemKey must not be zero; indicates the kind of display > >+ * item that is being invalidated. > > The implementation should assert that it's not zero. (But do you really mean > TYPE_GENERIC? Should this parameter be an nsDisplayItem::Type?) Added the assertion. I mean the magic number returned by nsDisplayItem::GetFrameKey --- see Part 4. > You don't need UpdateCursor in here. (There's no need anymore that the > maxHint be in decreasing order, since we examine all structs.) OK, done.

(In reply to comment #58) > (From update of attachment 451468 [details] [diff] [review]) > >+++ b/layout/base/nsDisplayItemTypes.h > >+enum Type { > >+ TYPE_NULL, > >+ TYPE_MAX > > Do we want to have a null type? I would think we want to enforce every item to > have its own type. Also, do we need TYPE_MAX here? Removed TYPE_NULL (but set TYPE_ALT_FEEDBACK to 1 so all types are nonzero). Kept TYPE_MAX, since it lets us terminate the enum cleanly without a trailing comma. > nsDisplayTransform's still don't return their list from GetList(), so I think > this should stay. Done. > The comments for nsContextBoxBlur say that the color should be set on the > context before calling Init. A 30 second look at the code indicates that maybe > this is no longer true. Either update the nsContextBoxBlur comment, or follow > it. I realize this problem was present in the old code too. Fixed by moving the thebesCtx-> code up above the Init. > There are some 80 char violations in this patch; I only mention this because > it is not your patch and you may want to correct them. Fixed.

(In reply to comment #61) > (From update of attachment 451470 [details] [diff] [review]) > >- nsRect GetClipRect() { return mClip; } > >+ const nsRect& GetClipRect() { return mClip; } > > Is this important for something? Or just an oversight you wanted to correct? Just a tiny fixup for efficiency. (In reply to comment #62) > -to- Fixed. > >+ for (i = 0; i < pluginsToMove.Length() - 1; ++i) { > > "i + 1 < pluginsToMove.Length()" for the unsigned arithmetic. Done. > I don't understand why the candidate is being compared against already-moved > plugins. If the candidate is going to hit the already-moved plugin now, > it will still hit it later. (Though I thought the destination regions > would be non-overlapping - is z-order involved?) > > Seems that this loop is not necessary. You're right. Removed. > (Though I thought the destination regions would be non-overlapping - is > z-order involved?) We won't necessarily guarantee that if the regions are too complicated. > Probably just comparing clip bounding rects would have been sufficient, but I > understand that would probably actually be more code, so this is fine until > its an issue. Clip regions can have overlapping bounding rects without actually overlapping. > Only the intersection region of initial and final clips really needs to > considered. Portions not in final won't be visible anyway, and portions not > in initial will need to be painted anyway, but again, probably don't need to > make that optimization. I suppose that's true, but it seems unnecessarily complicated to use it here. > Would it make sense to add a comment in the nsIWidget::ConfigureChildren docs > to indicate that children should be moved in the order provided? Yes, added. > Do you know whether the test passes on other platforms now? I don't. We can try that later. (In reply to comment #63) > (From update of attachment 451469 [details] [diff] [review]) > s/the the/the/ Fixed. (In reply to comment #64) > (From update of attachment 451820 [details] [diff] [review]) > In layout/base/nsPresShell.cpp: > >+ nsIFrame* rootFrame = FrameManager()->GetRootFrame(); > > Move this to just before "if (!rootFrame)" ? Done. (In reply to comment #65) > There's a few compile warnings that looks relevant (I don't know which > part they came from): > layout/base/nsDisplayList.cpp:1251: warning: unused variable 'isMoving' > layout/base/nsLayoutUtils.cpp:1049: warning: 'gDumpRepaintRegionForCopy' > defined but not used > layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:4140: warning: unused > variable 'delta' Fixed.

Comment on attachment 453556 [details] [diff] [review] Part 9 v2 In layout/base/FrameLayerBuilder.cpp The static DEBUG functions DumpIntRegion, DumpLayer could be useful as methods in their respective class...

Comment on attachment 451836 [details] [diff] [review] Part 14: Scroll using the layer system. In layout/base/nsLayoutUtils.cpp > nsLayoutUtils::GetActiveScrolledRootFor >+ while (PR_TRUE) { >+ if (f == aStopAtAncestor) { >+ *aOffset = offset; >+ return f; >+ } > ... This loop can be simplified: while (f != aStopAtAncestor) { ... } *aOffset = offset; return f; The two 'return' blocks inside the loop can now just 'break' instead. (might want to do this in part 23 though)

FYI, the following reftests fails consistently with the latest patch set (rev 832a2486cdc3) in a local (Ubuntu 10.04 x86-64) debug build: layout/reftests/bugs/309914-1.xul layout/reftests/bugs/331809-1.html layout/reftests/bugs/376375-1.html layout/reftests/bugs/446100-1f.html layout/reftests/bugs/513153-1a.html layout/reftests/bugs/513153-1b.html layout/reftests/bugs/513153-2a.html layout/reftests/bugs/513153-2b.html layout/reftests/marquee/413027-4.html I can't see the difference with the naked eye, but comparing the images with ImageMagick it looks like an (anti-aliasing?) difference at the corners of form controls, or the edge of some text.

Comment on attachment 451844 [details] [diff] [review] Part 17: Reimplemnt ColorLayers. There's a typo in the commit message.

Comment on attachment 453551 [details] [diff] [review] Part 29: Sort plugins so they get moved in the best order r+ based on comment 62

Comment on attachment 451854 [details] [diff] [review] Part 22: Mark scrolled elements as inactive after a timeout. layout/generic/nsGfxScrollFrame.*: r=mats xpcom/ds/nsExpirationTracker.h: I think you need an xpcom peer to review this file (I'm not). FYI, nsExpirationTracker::TimerCallback could use this new method. Also, there's an existing typo in the comment for NotifyExpired, it says "mTimerPeriod seconds" in two places. s/seconds/milliseconds/

Comment on attachment 453563 [details] [diff] [review] Part 16 v2 I would invoke the static function as ::AppendToTop for clarity.

Comment on attachment 452124 [details] [diff] [review] Part 25: Introduce a 'has active layers' flag on frames, which times out, and use it for 'opacity' I assume nsExpirationTracker can't fire NotifyExpired while PresShell::Destroy is tearing down the frame tree? sr=mats if that's correct.

Comment on attachment 452126 [details] [diff] [review] Part 26: Put background-attachment:fixed backgrounds in their own layer In layout/base/nsDisplayList.cpp > + nsStyleContext *bgSC; Nit: it looks like '*' goes with the type name in this file... In layout/generic/nsCanvasFrame.cpp > + nsStyleContext *bgSC; in this file too... > + NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT(i, bg) { > + const nsStyleBackground::Layer &layer = bg->mLayers[i]; and I guess that applies to '&' as well.

Painting Flash plugins is severely broken with this patch set - is there a dependent patch I need? (Firefox debug build, Ubuntu 10.04 x86-64, Flash 10.0 r45)

(In reply to comment #70) > (From update of attachment 453556 [details] [diff] [review]) > In layout/base/FrameLayerBuilder.cpp > The static DEBUG functions DumpIntRegion, DumpLayer could > be useful as methods in their respective class... FWIW bug 567421 covers the latter and moves the gfx* types logging "more centrally".

This patch actually caused most of the observed performance regression on Mac Twinopen. The problem is in the !IsRetaining path in BasicThebesLayer::Paint. In BasicLayerManager::PaintLayer before we call PushGroup we have logic to clip to the bounding-box of the layer subtree, so that the size of the temporary pushgroup surface is minimized. With part 20 we lost that optimization when BasicThebesLayer does its own pushgroup. This version of the patch puts the optimization back into BasicThebesLayer, sharing code with PaintLayer via a ClipToContain helper function.

(In reply to comment #79) > Painting Flash plugins is severely broken with this patch set - > is there a dependent patch I need? > (Firefox debug build, Ubuntu 10.04 x86-64, Flash 10.0 r45) Not patch yet, but this is bug 574583.

This optimization is a small win on Mac Twinopen, and it's really simple. Well, it's actually a *very* small win. Most of the cost of this CLEAR is the kernel faulting in pages for the buffer, so with this patch we just hit those faults later while painting the chrome window. Unfortunately we can't eliminate the costs of that allocation altogether, so there's still a small Twinopen regression observable (about 3ms out of 120ms on my machine). To some extent this is just an extra cost of retained layers. There are a few things we could do: 1) Cache ThebesLayerBuffer surfaces across windows. That feels like gaming the test, since it would only benefit people who open a window, close it, and repeat. Not very realistic. 2) Make BasicThebesLayers for chrome content non-retained. That would make scrolling in those layers slow, but maybe that's OK? We could be selective about which content gets non-retained layers. 3) Reduce the size of the ThebesLayerBuffer. Currently it's the size of the chrome window, which is overkill since almost the entire area is covered by the content window. I can probably come up with ways to make our ThebesLayers just be the size of the status bar and the top chrome. The solution might be different depending on bug 130078. Option #3 is the best. I'll think about it.

OK, after applying bug 130078 and a small fix on top of that which I'll attach elsewhere, *and* hiding the statusbar (which I believe will be the default in FF4?), the ThebesLayer for the top-chrome is the right size and the time spent in nsCocoaWindow::Show (where the bulk of the painting happens, and where I was measuring slowdown) goes from 3ms slower than trunk to 3ms faster :-). So I don't think we need to do more work on Twinopen here. We could handle cases where the statusbar is visible by arranging for it to be in its own layer.

(In reply to comment #71) > (From update of attachment 451836 [details] [diff] [review]) > In layout/base/nsLayoutUtils.cpp > > nsLayoutUtils::GetActiveScrolledRootFor > >+ while (PR_TRUE) { > >+ if (f == aStopAtAncestor) { > >+ *aOffset = offset; > >+ return f; > >+ } > > ... > > This loop can be simplified: > > while (f != aStopAtAncestor) { > ... > } > *aOffset = offset; > return f; > > The two 'return' blocks inside the loop can now just 'break' instead. > (might want to do this in part 23 though) Done. I pulled the part 23 changes into this patch for simplicity. (In reply to comment #72) > I can't see the difference with the naked eye, but comparing the images > with ImageMagick it looks like an (anti-aliasing?) difference at the > corners of form controls, or the edge of some text. Sounds like more of https://bugzilla.mozilla.org/show_bug.cgi?id=573933#c2. The patch there works around the problem on tinderboxes; if we're right that the problem is inaccuracy in alpha recovery, Karl is working on a complete solution. (In reply to comment #77) > I assume nsExpirationTracker can't fire NotifyExpired while > PresShell::Destroy is tearing down the frame tree? Only if we spin the event loop, in which case we've already lost. (In reply to comment #78) > Nit: it looks like '*' goes with the type name in this file... > > In layout/generic/nsCanvasFrame.cpp > in this file too... > and I guess that applies to '&' as well. Fixed

Just need review on the trivial extension to nsExpirationTracker.

Comment on attachment 454001 [details] [diff] [review] Part 22 v2 r=me on the nsExpirationTracker pieces; I didn't look at nsGfxScrollFrame.

Comment on attachment 453972 [details] [diff] [review] Part 20 v2 Yep, I can believe that killing perf..

Comment on attachment 453975 [details] [diff] [review] Part 30: Skip clearing our ThebesLayerBuffer surface if we just created it chris@cjones.org is (the | an)other cjones, ":cjones" gets the one speaking.

(In reply to comment #60) > (From update of attachment 451469 [details] [diff] [review]) > >+ virtual PRUint32 GetFrameKey() { return PRUint32(GetType()); } > > The name FrameKey seems to imply it has something to do with the frame, when in > fact it is based purely on the display item. I can't think of anything better > though. Actually, in the comments in other parts you refer to this as the "display item key". So why not DisplayItemKey or ItemKey?

Comment on attachment 453559 [details] [diff] [review] Part 27 v2 In layout/base/nsPresContext.cpp: > + mUpdatePluginGeometryForFrame(nsnull), > + mNeedsToUpdatePluginGeometry(PR_FALSE) Zero-ing these members is redundant since nsPresContext has a NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW. Insert a comment here instead? In layout/base/nsPresContext.h: > * tree. Only frames at or under aChangedRoot can have changed their > * geometry. > */ > - void UpdatePluginGeometry(nsIFrame* aChangedRoot); > + void UpdatePluginGeometry(); The comment still refers to aChangedRoot. In layout/base/nsPresShell.cpp > + mPresContext->NotifyDestroyingFrame(aFrame); The above is suppressed during PresShell::Destroy. I don't see any code that resets 'mUpdatePluginGeometryForFrame' in the root prescontext in that case. In view/public/nsIViewObserver.h > + * Notify the observer that we finished painting painting. This s/painting painting/painting/ In view/src/nsViewManager.cpp > +nsViewManager::CallDidPaintOnObservers() > +{ > + NS_PRECONDITION(IsRootVM(), "Must be root VM for this to be called!\n"); Remove the \n. Also in CallWillPaintOnObservers and FlushPendingInvalidates. In widget/public/nsGUIEvent.h > +// Widget was repainted (called on Windows when it's safe to move widgets) > +#define NS_DID_PAINT (NS_WINDOW_START + 28) "sent" or "dispatched" instead of "called" ? It's also sent on GTK2 platforms.

(In reply to comment #90) > Actually, in the comments in other parts you refer to this as the "display item > key". So why not DisplayItemKey or ItemKey? It's a method of nsDisplayItem so it seems redundant to say DisplayItemKey or ItemKey. How about GetPerFrameKey()? (In reply to comment #91) > (From update of attachment 453559 [details] [diff] [review]) > In layout/base/nsPresContext.cpp: > > + mUpdatePluginGeometryForFrame(nsnull), > > + mNeedsToUpdatePluginGeometry(PR_FALSE) > > Zero-ing these members is redundant since nsPresContext > has a NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW. > Insert a comment here instead? It is redundant, but I hate the NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW thing. You have to remember that certain classes have it and others don't, and you shouldn't initialize for the classes that have it, but you must for the classes that don't. Every so often I'm looking for where a field is initialized and can't find it and I get confused until I remember to check whether the class has NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW. I think we should just remove it, and in the meantime, I think we should be OK with explicit field initialization. What do you think? > In layout/base/nsPresContext.h: > The comment still refers to aChangedRoot. Fixed. > In layout/base/nsPresShell.cpp > > + mPresContext->NotifyDestroyingFrame(aFrame); > > The above is suppressed during PresShell::Destroy. > I don't see any code that resets 'mUpdatePluginGeometryForFrame' > in the root prescontext in that case. Yes, that's a problem. I'll fix this by moving the code to reset mUpdatePluginGeometryForFrame to a new method nsPresContext::ForgetUpdatePluginGeometryFrame and call that even during presshell teardown. > In view/public/nsIViewObserver.h > > + * Notify the observer that we finished painting painting. This > > s/painting painting/painting/ Fixed. > In view/src/nsViewManager.cpp > Remove the \n. Also in CallWillPaintOnObservers and FlushPendingInvalidates. Fixed. > In widget/public/nsGUIEvent.h > > +// Widget was repainted (called on Windows when it's safe to move widgets) > > +#define NS_DID_PAINT (NS_WINDOW_START + 28) > > "sent" or "dispatched" instead of "called" ? > It's also sent on GTK2 platforms. Fixed.

Updated as described above

(In reply to comment #92) > How about GetPerFrameKey()? Either GetPerFrameKey or just GetKey would be fine.

(In reply to comment #94) > Either GetPerFrameKey or just GetKey would be fine. GetPerFrameKey it is.

(In reply to comment #92) > It is redundant, but I hate the NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW thing. Ok. I always thought they were a preparation for eventually allocating those objects from a pool of *pre-zeroed* memory. If that's not a goal then I agree we should remove them.

Comment on attachment 454389 [details] [diff] [review] Part 27 v3 Looks great! r=mats

(In reply to comment #96) > Ok. I always thought they were a preparation for eventually allocating > those objects from a pool of *pre-zeroed* memory. If that's not a goal > then I agree we should remove them. That macro has been around for 10 years and I've never heard of such a goal :)

I checked in part 28 to fix orange from one of the dependent bugs that I just checked in: http://hg.mozilla.org/mozilla-central/rev/1feed65f4d5b

Comment on attachment 451844 [details] [diff] [review] Part 17: Reimplemnt ColorLayers. >+/** >+ * The address of gThebesDisplayItemLayerUserData is used as the user >+ * data pointer for ColorLayers >+ */ >+static PRUint8 gColorLayerUserData; s/gThebesDisplayItemLayerUserData/gColorLayerUserData/

Comment on attachment 451836 [details] [diff] [review] Part 14: Scroll using the layer system. Rebalancing review load by giving the primary review for this patch to Mats

Comment on attachment 451908 [details] [diff] [review] Part 23: Introduce the concept of 'inactive' layers Rebalancing review load by giving the primary review for this patch to Mats

Comment on attachment 452124 [details] [diff] [review] Part 25: Introduce a 'has active layers' flag on frames, which times out, and use it for 'opacity' Rebalancing review load by giving the primary review for this patch to Mats

Comment on attachment 452126 [details] [diff] [review] Part 26: Put background-attachment:fixed backgrounds in their own layer Rebalancing review load by giving the primary review for this patch to Mats

Comment on attachment 453563 [details] [diff] [review] Part 16 v2 Rebalancing review load by giving the primary review for this patch to Mats

Comment on attachment 451836 [details] [diff] [review] Part 14: Scroll using the layer system. Nothing further to add (you addressed my last comments). r=mats

Comment on attachment 453563 [details] [diff] [review] Part 16 v2 Nothing further; you addressed the nit I had. r=mats

Comment on attachment 451836 [details] [diff] [review] Part 14: Scroll using the layer system. sr=dbaron I hope there's another patch somewhere to remove the unused aVisibleRegionBeforeMove parameter. It also might be clearer if mActive is called mScrollingActive (to match the method name, which seems more obvious to me).

Comment on attachment 451908 [details] [diff] [review] Part 23: Introduce the concept of 'inactive' layers >+ * Note: for LAYER_INACTIVE container layers, all descendant >+ * display items must also be LAYER_INACTIVE or LAYER_NONE. Also, This comment should probably be clearer that each implementation that might return LAYER_INACTIVE is responsible for enforcing this invariant. That said, might the code end up simpler if the caller ended up enforcing this invariant instead? > /** >+ * Invalidates the layer that aItem used to belong to, if that was a >+ * ThebesLayer. Invalidates aNewLayer if that's a ThebesLayer. The area of >+ * aItem is invalidated. >+ */ >+ void InvalidateForLayerChange(nsDisplayItem* aItem, Layer* aNewLayer); I think I understand what this comment is saying, but I think it could be a little clearer in describing what invalidates happen. sr=dbaron

Comment on attachment 452124 [details] [diff] [review] Part 25: Introduce a 'has active layers' flag on frames, which times out, and use it for 'opacity' >+ // Tim Terriberry says that 300-400ms is a good timeout period >+ enum { TIMEOUT_MS = 100 }; The comment and the code seem a little contradictory. With a timeout this small, I worry about cases where a performance problem might cause a single paint to take up the whole expiration timeout, which could then cause us to throw the layers away and do the whole thing over. Could that sort of thing happen with this setup? sr=dbaron

Comment on attachment 452126 [details] [diff] [review] Part 26: Put background-attachment:fixed backgrounds in their own layer So is the idea here that scrollable areas always have layers, and they're sometimes active, and when the scrollable area's layer is active, then the fixed background layer is also active? And I think the commit message should say that it's only for background-attachment: fixed backgrounds that are propagated to the canvas. It seems like the ComputeVisibility change in this patch really belongs in the viewport-background patch under it.

Comment on attachment 453563 [details] [diff] [review] Part 16 v2 >+ // We put scrollbars in their own layers when this is the root scroll >+ // frame and we have no· ...? But shouldn't this be looking at a notion of whether the scrolling area is active? And shouldn't the declaration of |scrollParts| move into the loop so it doesn't accumulate extra stuff?

(In reply to comment #100) > s/gThebesDisplayItemLayerUserData/gColorLayerUserData/ Fixed. (In reply to comment #108) > I hope there's another patch somewhere to remove the unused > aVisibleRegionBeforeMove parameter. Not yet, but I'll do it. > It also might be clearer if mActive is called mScrollingActive (to match the > method name, which seems more obvious to me). Done. (In reply to comment #109) > (From update of attachment 451908 [details] [diff] [review]) > >+ * Note: for LAYER_INACTIVE container layers, all descendant > >+ * display items must also be LAYER_INACTIVE or LAYER_NONE. Also, > > This comment should probably be clearer that each implementation that > might return LAYER_INACTIVE is responsible for enforcing this invariant. Fixed. > That said, might the code end up simpler if the caller ended up > enforcing this invariant instead? I don't think it would be much different. I've factored out the callee code into nsDisplayWrapList::ChildrenCanBeInactive, so for each implementation of GetLayerState for containers, we only need an extra line of code. > > /** > >+ * Invalidates the layer that aItem used to belong to, if that was a > >+ * ThebesLayer. Invalidates aNewLayer if that's a ThebesLayer. The area of > >+ * aItem is invalidated. > >+ */ > >+ void InvalidateForLayerChange(nsDisplayItem* aItem, Layer* aNewLayer); > > I think I understand what this comment is saying, but I think it could > be a little clearer in describing what invalidates happen. OK, tried to fix: /** * If aItem used to belong to a ThebesLayer, invalidates the area of * aItem in that layer. If aNewLayer is a ThebesLayer, invalidates the area of * aItem in that layer. */ (In reply to comment #110) > (From update of attachment 452124 [details] [diff] [review]) > >+ // Tim Terriberry says that 300-400ms is a good timeout period > >+ enum { TIMEOUT_MS = 100 }; > > The comment and the code seem a little contradictory. Yes, I'll rename TIMEOUT_MS to GENERATION_MS. The comment is correct; nsExpirationTracker uses 4 generations of 100ms each, which gives us a timeout of 300-400ms. > With a timeout this small, I worry about cases where a performance > problem might cause a single paint to take up the whole expiration > timeout, which could then cause us to throw the layers away and do the > whole thing over. Could that sort of thing happen with this setup? Not really. If the document is quiescent, then we'll converge to a state where the layer is not active; for the layer to be active, something has to be marking it active from outside. If the document is not quiescent, so we get repeated paints, then if the update period is shorter than the activity timeout, we won't get any extra paints. (We might get into a situation where something should be marking the layer active again but it doesn't get a chance between the timer marking the layer inactive and the next paint; in that case we might be suboptimal in terms of not retaining the layer, but that's all.) If the update period is longer than the activity timeout, then we might end up doing extra paints as the layer flips between active and non-active, but only as long as the browser can keep up with the paints, otherwise they'll be coalesced, so our performance under peak load shouldn't be affected. To summarize: 1) If we're already painting as frequently as we can, flipping a layer between active and inactive is mostly free. 2) In a quiescent document, layers converge to being inactive.

(In reply to comment #111) > (From update of attachment 452126 [details] [diff] [review]) > So is the idea here that scrollable areas always have layers, and > they're sometimes active, and when the scrollable area's layer is > active, then the fixed background layer is also active? No. Inactive scrollable areas don't have their own layers. For each display item, we figure out what the nearest "active scrolled root" is --- that is, the nearest ancestor scrollframe that's "actively being scrolled". Display items consecutive in z-order with the same active scrolled root and that don't have their own layers are grouped into the same ThebesLayer. That means when we scroll the active scrolled root, we can just bump the offsets on the ThebesLayers that it's associated with. You can think of those ThebesLayers as "belonging" to the active scrolled root, but it's a loose association. What this patch is doing is associating a background-attachment:fixed display item with the viewport as its active scrolled root, instead of the viewport scroll frame. But I just realized there's a bug here: instead of setting the viewport as the active scrolled root, we should find the active scrolled root for the viewport and use that. > And I think the commit message should say that it's only for > background-attachment: fixed backgrounds that are propagated to the > canvas. Will fix. > It seems like the ComputeVisibility change in this patch really > belongs in the viewport-background patch under it. No, I actually needed it here. viewport-background functions OK without it. The bogus empty background was causing us to create an unnecessary layer somewhere, although I can't quite remember the details.