Closed Bug 564991 Opened 14 years ago Closed 14 years ago

Retain layers and layer contents

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: roc, Assigned: roc)

References

(Depends on 2 open bugs, Blocks 4 open bugs)

Details

Attachments

(49 files, 17 obsolete files)

7.30 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
78.28 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.43 KB, patch
tnikkel
: review+
MatsPalmgren_bugz
: superreview+
Details | Diff | Splinter Review
1.12 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
36.67 KB, patch
cjones
: review+
Details | Diff | Splinter Review
8.32 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
18.37 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
34.24 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
2.42 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
5.93 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
63.02 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.73 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
17.03 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
5.15 KB, patch
vlad
: review+
Details | Diff | Splinter Review
3.93 KB, patch
vlad
: review+
Details | Diff | Splinter Review
14.58 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
30.35 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.13 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
15.28 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.70 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
983 bytes, patch
Details | Diff | Splinter Review
6.51 KB, patch
karlt
: review+
Details | Diff | Splinter Review
12.46 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
19.50 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.92 KB, patch
cjones
: review+
Details | Diff | Splinter Review
9.98 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
37.94 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
11.05 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
83.21 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
24.77 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
12.18 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
326.98 KB, image/png
Details
187.90 KB, image/png
Details
1.31 KB, patch
cjones
: review+
Details | Diff | Splinter Review
4.56 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.27 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
22.99 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
136.22 KB, image/png
Details
1.71 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
7.10 KB, patch
jaas
: review+
Details | Diff | Splinter Review
13.64 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
6.52 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
818 bytes, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.55 KB, patch
cjones
: review+
Details | Diff | Splinter Review
61.71 KB, image/png
Details
2.51 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.74 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.17 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
805.40 KB, image/jpeg
Details
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.
Depends on: 571104
Depends on: 571448
No longer blocks: 568392
Depends on: 568392
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.
Attachment #451468 - Flags: review?(tnikkel)
See comments. We need this on top of part 3 to associate frame parts with their layers.
Attachment #451469 - Flags: superreview?(matspal)
Attachment #451469 - Flags: review?(tnikkel)
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.
Attachment #451478 - Flags: superreview?(vladimir)
Attachment #451478 - Flags: review?
Attachment #451478 - Flags: review? → review?(jones.chris.g)
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.
Attachment #451480 - Flags: superreview?(dbaron)
Attachment #451480 - Flags: review?(matspal)
blocking2.0: --- → beta1+
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.
Attachment #451481 - Flags: review?(jmuizelaar) → review+
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.
Attachment #451478 - Flags: superreview?(vladimir) → superreview+
(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.
Attached patch Part 2 v2Splinter Review
Fixed to avoid reusing an old layer if the canvas was reinitialized.
Attachment #451465 - Attachment is obsolete: true
Attachment #451809 - Flags: superreview?(vladimir)
Attachment #451809 - Flags: review?(matspal)
Attachment #451465 - Flags: superreview?(vladimir)
Attachment #451465 - Flags: review?(matspal)
Attached patch Part 9: retain layer trees (obsolete) — Splinter Review
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.
Attachment #451815 - Flags: superreview?(matspal)
Attachment #451815 - Flags: review?(tnikkel)
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!
Attachment #451818 - Flags: review?(matspal)
We were getting code duplication between nsPresShell::Paint/nsLayoutUtils::PaintFrame on one hand and nsPresShell::RenderDocument on the other hand. This fixes that.
Attachment #451820 - Flags: superreview?(matspal)
Attachment #451820 - Flags: review?(tnikkel)
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!
Attachment #451828 - Flags: superreview?(matspal)
Attachment #451828 - Flags: review?(tnikkel)
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.
Attachment #451836 - Flags: superreview?(matspal)
Attachment #451836 - Flags: review?(tnikkel)
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.
Attachment #451838 - Flags: superreview?(matspal)
Attachment #451838 - Flags: review?(tnikkel)
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.
Attachment #451844 - Flags: review?(matspal)
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.
Attachment #451847 - Flags: superreview?(vladimir)
Attachment #451847 - Flags: review?(jmuizelaar)
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!)
Attachment #451848 - Flags: review?(vladimir)
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.
Attachment #451850 - Flags: review?(vladimir)
Finish the optimization described in Part 19.
Attachment #451852 - Flags: review?(vladimir)
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...
Attachment #451854 - Flags: review?(matspal)
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 :).
Attachment #451478 - Flags: review?(jones.chris.g) → review+
Blocks: 532828
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.
Attachment #451828 - Attachment is obsolete: true
Attachment #451902 - Flags: superreview?(dbaron)
Attachment #451902 - Flags: review?(matspal)
Attachment #451828 - Flags: superreview?(matspal)
Attachment #451828 - Flags: review?(tnikkel)
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).
Attachment #451908 - Flags: superreview?(matspal)
Attachment #451908 - Flags: review?(tnikkel)
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.)
Attachment #451912 - Flags: review?(chris.double)
(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 :-)
Attachment #451912 - Flags: review?(chris.double) → review+
Attachment #451481 - Flags: superreview?(vladimir) → superreview+
Attachment #451809 - Flags: superreview?(vladimir) → superreview+
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.)
Attachment #451847 - Flags: superreview?(vladimir) → superreview+
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.
Attachment #451850 - Flags: review?(vladimir) → review+
Attached patch Part 18 v2Splinter Review
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.
Attachment #452120 - Flags: superreview?(vladimir)
Attachment #452120 - Flags: review?(jmuizelaar)
Comment on attachment 452120 [details] [diff] [review]
Part 18 v2

Even better.
Attachment #452120 - Flags: superreview?(vladimir) → superreview+
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.
Attachment #452124 - Flags: superreview?(matspal)
Attachment #452124 - Flags: review?(tnikkel)
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.
Attachment #451847 - Attachment is obsolete: true
Attachment #452126 - Flags: superreview?(matspal)
Attachment #452126 - Flags: review?(tnikkel)
Attachment #451847 - Flags: review?(jmuizelaar)
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.
Attachment #452135 - Flags: superreview?(vladimir)
Attachment #452135 - Flags: review?(matspal)
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.
Attachment #452138 - Flags: review?(enndeakin)
That's all I have for now! There are still a few test failures that I'm working on.
Depends on: 572968
Attachment #452120 - Flags: review?(jmuizelaar) → review+
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?
Attachment #452135 - Flags: superreview?(vladimir) → superreview+
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.
Blocks: 573409
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?
Attachment #452138 - Flags: review?(enndeakin) → review+
I haven't. I suppose someone should. Maybe I'll just check that patch into trunk and see what happens...
blocking2.0: beta1+ → beta2+
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
Attachment #452138 - Attachment is obsolete: true
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.
Attachment #453551 - Attachment description: Bug 564991. Part 29: Sort plugins so they get moved in the best order → Part 29: Sort plugins so they get moved in the best order
Attached patch Part 9 v2 (obsolete) — Splinter Review
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.
Attachment #451815 - Attachment is obsolete: true
Attachment #453556 - Flags: superreview?(matspal)
Attachment #453556 - Flags: review?(tnikkel)
Attachment #451815 - Flags: superreview?(matspal)
Attachment #451815 - Flags: review?(tnikkel)
Attached patch Part 27 v2 (obsolete) — Splinter Review
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.
Attachment #452135 - Attachment is obsolete: true
Attachment #453559 - Flags: review?(matspal)
Attachment #452135 - Flags: review?(matspal)
Attached patch Part 16 v2Splinter Review
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.
Attachment #451838 - Attachment is obsolete: true
Attachment #453563 - Flags: superreview?(matspal)
Attachment #453563 - Flags: review?(tnikkel)
Attachment #451838 - Flags: superreview?(matspal)
Attachment #451838 - Flags: review?(tnikkel)
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
Attachment #451480 - Flags: superreview?(dbaron) → superreview+
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
Attachment #451902 - Attachment description: Part 13 v2 → Part 13 v2: Optimize invalidation to avoid repainting ThebesLayer contents sometimes
Attachment #451902 - Flags: superreview?(dbaron) → superreview+
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.
Attachment #451468 - Flags: review?(tnikkel) → review+
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.
Attachment #451469 - Flags: review?(tnikkel) → review+
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?
Attachment #451470 - Flags: review?(tnikkel) → review+
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?
Attachment #451463 - Flags: review?(matspal) → review+
Comment on attachment 451469 [details] [diff] [review]
Part 4: Create GetFrameKey API

s/the the/the/
Attachment #451469 - Flags: superreview?(matspal) → superreview+
Attachment #451809 - Flags: review?(matspal) → review+
Attachment #451480 - Flags: review?(matspal) → review+
Attachment #451816 - Flags: review?(matspal) → review+
Attachment #451818 - Flags: review?(matspal) → review+
Attachment #451820 - Flags: superreview?(matspal) → superreview+
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)" ?
Attachment #451902 - Flags: review?(matspal) → review+
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...
Attachment #453556 - Flags: superreview?(matspal) → superreview+
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)
Attachment #451836 - Flags: superreview?(matspal) → superreview+
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.
Attachment #451837 - Flags: review?(matspal) → review+
Attachment #451844 - Flags: review?(matspal) → review+
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
Attachment #453551 - Flags: review?(karlt) → review+
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/
Attachment #451854 - Flags: review?(matspal) → review+
Comment on attachment 453563 [details] [diff] [review]
Part 16 v2

I would invoke the static function as ::AppendToTop for clarity.
Attachment #453563 - Flags: superreview?(matspal) → superreview+
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.
Attachment #452124 - Flags: superreview?(matspal) → superreview+
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.
Attachment #452126 - Flags: superreview?(matspal) → superreview+
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".
Depends on: 574583
Attached patch Part 20 v2Splinter Review
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.
Attachment #453972 - Flags: review?(vladimir)
(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.
Attachment #451850 - Attachment is obsolete: true
Attachment #453975 - Flags: review?
Attachment #453975 - Flags: review? → review?(chris)
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
Attached patch Part 22 v2Splinter Review
Just need review on the trivial extension to nsExpirationTracker.
Attachment #451854 - Attachment is obsolete: true
Attachment #454001 - Flags: review?
Attachment #454001 - Flags: review? → review?(benjamin)
Comment on attachment 454001 [details] [diff] [review]
Part 22 v2

r=me on the nsExpirationTracker pieces; I didn't look at nsGfxScrollFrame.
Attachment #454001 - Flags: review?(benjamin) → review+
Attachment #451908 - Flags: superreview?(matspal) → superreview+
Comment on attachment 453972 [details] [diff] [review]
Part 20 v2

Yep, I can believe that killing perf..
Attachment #453972 - Flags: review?(vladimir) → review+
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.
Attachment #453975 - Flags: review?(chris) → review?(jones.chris.g)
Attachment #453975 - Flags: review?(jones.chris.g) → review+
(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.
Attached patch Part 27 v3Splinter Review
Updated as described above
Attachment #453559 - Attachment is obsolete: true
Attachment #454389 - Flags: review?(matspal)
Attachment #453559 - Flags: review?(matspal)
(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.
Attachment #451809 - Attachment description: Part v2 → Part 2 v2
(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
Attachment #454389 - Flags: review?(matspal) → review+
(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/
Attachment #451836 - Flags: superreview?(dbaron)
Attachment #451836 - Flags: superreview+
Attachment #451836 - Flags: review?(tnikkel)
Attachment #451836 - Flags: review?(matspal)
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
Attachment #451908 - Flags: superreview?(dbaron)
Attachment #451908 - Flags: superreview+
Attachment #451908 - Flags: review?(tnikkel)
Attachment #451908 - Flags: review?(matspal)
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
Attachment #452124 - Flags: superreview?(dbaron)
Attachment #452124 - Flags: superreview+
Attachment #452124 - Flags: review?(tnikkel)
Attachment #452124 - Flags: review?(matspal)
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
Attachment #452126 - Flags: superreview?(dbaron)
Attachment #452126 - Flags: superreview+
Attachment #452126 - Flags: review?(tnikkel)
Attachment #452126 - Flags: review?(matspal)
Comment on attachment 453563 [details] [diff] [review]
Part 16 v2

Rebalancing review load by giving the primary review for this patch to Mats
Attachment #453563 - Flags: superreview?(dbaron)
Attachment #453563 - Flags: superreview+
Attachment #453563 - Flags: review?(tnikkel)
Attachment #453563 - Flags: review?(matspal)
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
Attachment #451836 - Flags: review?(matspal) → review+
Attachment #451908 - Flags: review?(matspal) → review+
Attachment #452124 - Flags: review?(matspal) → review+
Attachment #452126 - Flags: review?(matspal) → review+
Comment on attachment 453563 [details] [diff] [review]
Part 16 v2

Nothing further; you addressed the nit I had.  r=mats
Attachment #453563 - Flags: review?(matspal) → review+
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).
Attachment #451836 - Flags: superreview?(dbaron) → superreview+
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
Attachment #451908 - Flags: superreview?(dbaron) → superreview+
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
Attachment #452124 - Flags: superreview?(dbaron) → superreview+
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.
(In reply to comment #112)
> (From update of attachment 453563 [details] [diff] [review])
> >+  // We put scrollbars in their own layers when this is the root scroll
> >+  // frame and we have no·
> 
> ...?

Oops! s/we have no/we are a toplevel content document/.

> But shouldn't this be looking at a notion of whether the scrolling area
> is active?

No, the toplevel content document's viewport scrolling area is always active.

> And shouldn't the declaration of |scrollParts| move into the loop so
> it doesn't accumulate extra stuff?

No, because all display list operations such as AppendToTop are destructive; they move items from one list to another.
So what does it mean to make a layer for something with a fixed background when the scrollable area that it's fixed to doesn't have a layer?

And likewise for scrollbars, if the corresponding scrollable area doesn't have a layer?
(In reply to comment #116)
> So what does it mean to make a layer for something with a fixed background
> when the scrollable area that it's fixed to doesn't have a layer?

A fixed background isn't fixed to a scrollable area, it's fixed to the viewport. What we're doing here is disassociating the fixed background from the scrollable area, treating it as if the canvas background was outside the scrollable area's display list.

In the common case of a fixed background for the toplevel content document, we'll typically end up with a layer tree that looks like:
ContainerLayer for the whole content document tree
--> child ThebesLayer for the fixed background
--> child ThebesLayer for the scrolled content

When we have a fixed background for an IFRAME, we could end up with layer tree like this when scrolling is not active:
ContainerLayer
--> child ThebesLayer containing the scrolled content for the toplevel document, the fixed background of the IFRAME, and the rest of the content of the IFRAME

When scrolling is active:
ContainerLayer
--> child ThebesLayer containing the scrolled content for the toplevel document and the fixed background of the IFRAME
--> child ThebesLayer for the scrolled content of the IFRAME

> And likewise for scrollbars, if the corresponding scrollable area doesn't have
> a layer?

Scrollbars aren't really analogous since they only get layers if they're the viewport scrollbars for the toplevel content document, whose scrollable area is treated as always active. The typical layer tree for a document with scrollbars showing is:
ContainerLayer
--> child ThebesLayer(s) containing scrollbars
--> child ThebesLayer containing scrolled content

(Note that even if we didn't specially put scrollbars in their own layers, they'd get their own layer since they don't moved when scrolled by the viewport. The problem is that when two scrollbars are visible, they'd be put in the same layer, which would be the size of the viewport but mostly unused ... a big space penalty, and also a penalty for compositing that large surface over and over. At some point we might want to have more intelligent buffers that support sparse allocation, e.g. by tiling, to deal with this in a more general way.)

For a document containing an IFRAME with active scrolling in its viewport:
ContainerLayer
--> child ThebesLayer containing scrolled content of toplevel document and scrollbars of the IFRAME
--> child ThebesLayer containing scrolled content of the IFRAME
Attached patch Part 26 v2Splinter Review
Fixed that bug. There's no test, since the old code does work, it would just put the fixed-background in its own layer unnecessarily when we could, and should, merge it with the layers for the content in the surrounding document.
Attachment #455351 - Flags: superreview?(dbaron)
Attachment #452126 - Attachment is obsolete: true
Attachment #452126 - Flags: superreview?(dbaron)
Comment on attachment 453563 [details] [diff] [review]
Part 16 v2

OK, then this looks good if you fix the comment that ends mid-sentence, and also fix it to explain that that condition is equivalent to a case when we'd always create a separate layer for the scrollbars anyway.  (But it's not *all* the cases where we would, right?)
Attachment #453563 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 455351 [details] [diff] [review]
Part 26 v2

So if an iframe, where the scrollable area isn't always considered active (right?), has a fixed positioned background, we still give the fixed-positioned background its own layer, and something automatically merges that layer into the ThebesLayer above it anyway?

I guess I'll mark sr=dbaron, although I'd still somewhat like to make sense of this...
Attachment #455351 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #119)
> (From update of attachment 453563 [details] [diff] [review])
> OK, then this looks good if you fix the comment that ends mid-sentence, and
> also fix it to explain that that condition is equivalent to a case when we'd
> always create a separate layer for the scrollbars anyway.

OK, done.

> (But it's not *all* the cases where we would, right?)

Yeah. For example you could have a toplevel document containing a single IFRAME and nothing else, where the IFRAME has scrollbars. That IFRAME's scrollbars would be in their own layer while the IFRAME's viewport scrolling is active.

(In reply to comment #120)
> So if an iframe, where the scrollable area isn't always considered active
> (right?),

Right

> has a fixed positioned background, we still give the fixed-positioned
> background its own layer, and something automatically merges that layer into
> the ThebesLayer above it anyway?

Not exactly, now that I've fixed the bug I mentioned.

nsDisplayBackground/CanvasBackground never asks for a dedicated layer, it always returns LAYER_NONE from GetLayerState. When FrameLayerBuilder processes the display item for the fixed background, we figure out what its "active scrolled root" should be, and in this case it would be the viewport scrolled frame for the toplevel document. If there are display items for other scrolled content of the toplevel document that are adjacent to the fixed background in z-order, FrameLayerBuilder will assign the fixed background to the same ThebesLayer as those display items, since they have the same "active scrolled root" (and other conditions are met).

> I guess I'll mark sr=dbaron, although I'd still somewhat like to make sense of
> this...

You'd have to read through at least the interface of FrameLayerBuilder, and maybe some of the implementation. Maybe we can talk it through with Timothy and Mats at the summit.
Blocks: 563887
Comment on attachment 451478 [details] [diff] [review]
Part 6: Make BasicLayers support retained ThebesLayer contents.

>   /**
>    * Function called to draw the contents of each ThebesLayer.
>    * aRegionToDraw contains the region that needs to be drawn.
>-   * This would normally be a subregion of the visible region. Drawing is
>-   * not necessarily clipped to aRegionToDraw.
>+   * This would normally be a subregion of the visible region.
>+   * The callee must draw all of aRegionToDraw. Drawing outside
>+   * aRegionToDraw will be clipped out or ignored.
>    * The callee must draw all of aRegionToDraw.

The sentence "The callee must draw all of aRegionToDraw." appears twice.
I found a bug where in certain scrolling cases, content that got scrolled into view wouldn't be painted.

The bug happened because in the first ComputeVisibility pass performed in nsLayoutUtils::PaintFrame, display items that were totally covered by other content were removed. But later when we paint ThebesLayers, a ThebesLayer might ask us to paint a region even though it's covered, and later use the bits that we painted when that area of the layer is scrolled into view so it's no longer covered.

In FrameLayerBuilder::DrawThebesLayer we recompute visibility for the items in the layer to handle exactly this situation. However, that doesn't help if the item has already been completely removed. So let's just not remove items that are invisible. We still get all our optimizations if we check visibility before we paint.
Attachment #455636 - Flags: review?(matspal)
Comment on attachment 455636 [details] [diff] [review]
Part 31: Don't cull display items in nsDisplayList::ComputeVisibility

Looks reasonable, r=mats
Maybe it doesn't matter much, but why check IsEmpty after ToNearestPixels
in one place and not the other?
Attachment #455636 - Flags: review?(matspal) → review+
Comment on attachment 453556 [details] [diff] [review]
Part 9 v2

In FrameBuilderLayer.h I'd like to see a slightly more detailed saying something like "frame layer builder takes as input a display list and it produces the frame property mapping item keys to layers and the layer manager user data listing frames that are in layers".

In FrameLayerBuilder.h the functions AddLayerDisplayItem, AddThebesDisplayItem, and GetOldLayerFor have a comment saying they are private to FrameLayerBuilder. Can you mention that they are not made private so some functions in FrameLayerBuilder.cpp that aren't in FrameLayerBuilder can access them?

DisplayItemDataEntry and ThebesLayerItemsEntry both define a copy constructor that does a deep copy of an array. Mention that it is unused but needs to be defined to work.

+typedef nsTHashtable<nsPtrHashKey<nsIFrame*> > ThebesLayerUserData;

This doesn't appear to be used anywhere, and could be wrong: the hashtable would have keys that are pointers to pointers to frames?

+/**
+ * This property represents a region that should be invalidated in every
+ * ThebesLayer child whose parent ContainerLayer is associated with the
+ * frame.
+ */
+NS_DECLARE_FRAME_PROPERTY(ThebesLayerInvalidRegionProperty, DestroyRegion)

The comment should say what type is stored in this property, and because it is a type with coordinates should say what coordinate system it is in.

The comment should also say where this region gets added to and when the region is made valid.

+  /**
+   * Invalidates the contents of the entire layer, if it's a ThebesLayer,
+   * and triggers frame invalidation of the region occupied by aLayer.
+   */
+  void InvalidateWholeLayer(Layer* aLayer);

This seems to be unused in the entire patch queue, remove it? If not, then I have comments about it.

I'd also like to see some comments in Layers.h specifying the coord systems of the visible region of Layer's and the invalid/valid region of ThebesLayer's. But that doesn't belong in this patch.


+/**
+ * The address of gThebesDisplayItemLayerUserData is used as the user
+ * data pointer for ThebesLayers that are created to contain
+ * Thebes-rendered display items.
+ */
+static PRUint8 gThebesDisplayItemLayerUserData;

This is actually used to mark thebes layers that are retained and is not used on non-retained thebes layers? Or is it just used to avoid a virtual call sometimes? It is not entirely clear why we need this and what it is used for exactly.


In FrameLayerBuilder::UpdateDisplayItemDataForFrame if there is no entry in mNewDisplayItemData for the frame we remove the frame from the layer manager's user data's mFramesWithLayers. If we remove that last entry of mFramesWithLayers from DestroyDisplayItemData then we delete the corresponding layer manager's user data, but not if we remove the last mFramesWithLayers entry in UpdateDisplayItemDataForFrame.

+  NS_DECLARE_FRAME_PROPERTY_WITH_FRAME_IN_DTOR(DisplayItemDataProperty,
+                                               DestroyDisplayItemData)

The fact that we store the nsTArray<DisplayItemData> itself in the property and not a pointer to it is a little unexpected and should be mentioned.


mNewDisplayItemData items that are processed by StoreNewDisplayItemData don't get removed from mNewDisplayItemData because the frame layer builder will be going away shortly. You might want to mention that if we ever want to keep frame layer builders around longer in the future.

+static PRUint32
+AppUnitsPerDevPixel(nsDisplayItem* aItem)
+{
+  return aItem->GetUnderlyingFrame()->PresContext()->AppUnitsPerDevPixel();
+}

It wasn't immediately obvious to me that all the items we call this on are guaranteed to have an underlying frame.


+  PRUint32 containerLayerCount = 0;
+  for (PRUint32 i = 0; i < newDisplayItems->mData.Length(); ++i) {
+    if (newDisplayItems->mData[i].mLayer->GetType() == Layer::TYPE_CONTAINER) {
+      ++containerLayerCount;
+    }
+  }
+  if (containerLayerCount == 0) {
+    f->RemoveStateBits(NS_FRAME_HAS_CONTAINER_LAYER);
+  } else {
+    NS_ASSERTION(f->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER,
+                 "This bit should have been set by GetContainerLayerFor");
+  }

Why not just use a bool and break?

+  // Reset the invalid region now so we can start collecting new dirty
+  // areas.
+  nsRegion* invalidRegion = static_cast<nsRegion*>
+    (props.Get(ThebesLayerInvalidRegionProperty()));
+  if (invalidRegion) {
+    invalidRegion->SetEmpty();
+  }

Do we want to make any attempt to remove the ThebesLayerInvalidRegionProperty property on frames that stop having a container layer?

+    NS_ASSERTION(!transform.HasNonIntegerTranslation(),
+                 "Matrix not just an integer translation?");

HasNonIntegerTranslation doesn't do what I would expect given its name. What about reversing the logic and calling it HasOnlyIntegerTranslation?


+    // This gets called on recycled ThebesLayers that are going to be in the
+    // final layer tree, so it's a convenient time to invalidate the
+    // content that changed where we don't know what ThebesLayer it belonged
+    // to, or if we need to invalidate entire layer, we can do that.
+    // This needs to be done before we update the ThebesLayer to its new
+    // transform. See nsGfxScrollFrame::InvalidateInternal, where
+    // we ensure that mInvalidThebesContent is updated according to the
+    // scroll position as of the most recent paint.

"if we need to invalidate _the_ entire layer"


+  // Set up transform so that 0,0 in the Thebes layer corresponds to the
+  // (pixel-snapped) top-left of the aActiveScrolledRoot.

This comment seems important enough to be somewhere more prominent.


+    // We do not need to add this area to mInvalidArea since we assume
+    // the caller of InvalidateThebesLayerContents or
+    // InvalidateAllThebesLayerContents has ensured
+    // the area is invalidated in the widget.

What does this comment mean? There is no mInvalidArea in m-c or in the entire patch series that I can see.






+    /**
+     * The region of visible content above the layer and below the
+     * next ThebesLayerData in the stack, if any.
+     */
+    nsIntRegion  mVisibleAboveRegion;

In the comment make it clear that not all thebes layers will be in the stack, and this region is only concerned with layers that are currently in the stack.

+  if (lastIndex > 0) {
+    ThebesLayerData* nextData = mThebesLayerDataStack[lastIndex - 1];
+    nextData->mVisibleAboveRegion.Or(nextData->mVisibleAboveRegion,
+                                     data->mVisibleAboveRegion);
+    nextData->mVisibleAboveRegion.Or(nextData->mVisibleAboveRegion,
+                                     data->mVisibleRegion);
+  }

Make a little comment here because it is a little more subtle then it first appears for the same reason.


+    /** The region of visible content in the layer. */
+    nsIntRegion  mVisibleRegion;

Say in the comment what coord system this is in.



+  if (data->mLayer->GetTransform().Is2D(&transform)) {
+    NS_ASSERTION(!transform.HasNonIntegerTranslation(),
+                 "Matrix not just an integer translation?");
+    // Compensate for that translation as we render
+    nsIntRegion rgn = data->mVisibleRegion;
+    rgn.MoveBy(-nsIntPoint(PRInt32(transform.x0), PRInt32(transform.y0)));
+    data->mLayer->SetVisibleRegion(rgn);

A more useful comment would be "convert the visible region from being relative to A to being relative to B" where A and B are the correct things.


+  data->mVisibleRegion.Sub(data->mVisibleRegion, data->mOpaqueRegion);
+  data->mLayer->SetIsOpaqueContent(data->mVisibleRegion.IsEmpty());
+
+  mThebesLayerDataStack.RemoveElementAt(lastIndex);

You're reusing data->mVisibleRegion as a temporary here because it's going away in 2 lines? Please comment that.


+  /**
+   * Find the ThebesLayer to which we should assign the next display item.
+   * Returns the layer, and also updates the ThebesLayerData. Will
+   * push a new ThebesLayerData onto the stack if necessary.
+   */
+  already_AddRefed<ThebesLayer> UpdateThebesLayerDataFor(const nsIntRect& aVisibleRect,
+                                                         nsIFrame* aActiveScrolledRoot,
+                                                         PRBool aIsOpaque);

In the comment you may as well also say that it will pop stuff from the stack if necessary too.

Indicate what the arguments actually represent.

I don't like the name of this function. UpdateThebesLayerDataFor makes it sound like it's just a function you call for its side effects. It feels like the finding and returning the layer to use is the main part of the function. So if we are going to ignore half of what the function does in its name I think FindThebesLayerFor would be better. I can't think of a good name that incorporates everything the function does.


   /**
    * Get a container layer for a display item that contains a child
    * list, either reusing an existing one or creating a new one.
    * aContainer may be null, in which case we construct a root layer.
    */
   already_AddRefed<Layer> GetContainerLayerFor(nsDisplayListBuilder* aBuilder,

This function does a lot more than either its comment or name indicates. At least beef up the comment. And maybe a better function name.


/*
 * Iterate through the non-clip items in aList and its descendants.
 * For each item we compute the effective clip rect. Each item is assigned
 * to a layer. We invalidate the areas in ThebesLayers where an item
 * has moved from one ThebesLayer to another. Also,
 * aState->mInvalidThebesContent is invalidated in every ThebesLayer.
 * We set the clip rect for items that generated their own layer.
 * We set the visible rect for all layers except for the bottommost
 * Thebes layer and the last Thebes layer.
 */

Why not set the visible rect for the bottommost thebes layer? And what is the last thebes layer exactly and how does it differ from the bottommost? And why not set the visible rect for it too?


    nsRefPtr<Layer> ownLayer = item->BuildLayer(mBuilder, mManager);
    // Assign the item to a layer
    if (ownLayer) {
      NS_ASSERTION(ownLayer->Manager() == mManager, "Wrong manager");
      // It has its own layer. Update that layer's clip and visible rects.
      if (aClipRect) {
        ownLayer->IntersectClipRect(
            aClipRect->ToNearestPixels(appUnitsPerDevPixel));
      }
      ThebesLayerData* data = GetTopThebesLayerData();
      if (data) {
        data->mVisibleAboveRegion.Or(data->mVisibleAboveRegion, itemVisibleRect);
      }

Can we get an assertion here that the ownLayer is not a thebes layer?


      nsPoint offsetToActiveScrolledRoot;
      nsIFrame* activeScrolledRoot =
        nsLayoutUtils::GetActiveScrolledRootFor(f, mBuilder->ReferenceFrame(),
                                                &offsetToActiveScrolledRoot);
      NS_ASSERTION(offsetToActiveScrolledRoot == f->GetOffsetTo(activeScrolledRoot),
                   "Wrong offset");

This assert will fail because the offset is not updated at all in GetActiveScrolledRootFor in this patch. I'm sure it must be in the entire patch series though.


        // The item has changed layers.
        // Invalidate the bounds in the old layer and new layer.
        // The bounds might have changed, but we assume that any difference
        // in the bounds will have been invalidated for all Thebes layers
        // in the container via regular frame invalidation.
        nsRect bounds = item->GetBounds(mBuilder);
        nsIntRect r = bounds.ToOutsidePixels(appUnitsPerDevPixel);
        InvalidatePostTransformRegion(oldLayer->AsThebesLayer(), r);
        InvalidatePostTransformRegion(thebesLayer, r);

        mContainerFrame->Invalidate(bounds - mBuilder->ToReferenceFrame(mContainerFrame));

So why do we need to invalidate the container frame here too?


+  /**
+   * This finalizes all the open ThebesLayers by popping every element off
+   * mThebesLayerDataStack, then sets the children of the container layer
+   * to be all the layers in mNewChildLayers.
+   */
+  void Finish();

May as well just say everything that it does and expand it to:

   * This finalizes all the open ThebesLayers by popping every element off
   * mThebesLayerDataStack, then sets the children of the container layer
   * to be all the layers in mNewChildLayers in that order and removes any
   * layers as children of the container that aren't in mNewChildLayers.


   // If there's only one layer, then in principle we can try to flatten
   // things by returning that layer here. But that adds complexity to
   // retained layer management so we don't do it. Layer backends can
   // flatten internally.

I don't understand this comment.


+  if (aManager == mRetainingManager) {
+    Layer* oldLayer = GetOldLayerFor(aContainerFrame, containerDisplayItemKey);
+    if (oldLayer) {
+      NS_ASSERTION(oldLayer->Manager() == aManager, "Wrong manager");
+      containerLayer = static_cast<ContainerLayer*>(oldLayer);
+      // Clear clip rect; we'll set it up again later.
+      containerLayer->SetClipRect(nsnull);
+    }

Can you add an assert here that the layer is indeed a container layer?

+      NS_ASSERTION(oldLayer->Manager() == aManager, "Wrong manager");
+      containerLayer = static_cast<ContainerLayer*>(oldLayer);
+      // Clear clip rect; we'll set it up again later.
+      containerLayer->SetClipRect(nsnull);

Where will we set it?


  /**
   * Get a retained layer for a leaf display item. Returns null if no
   * layer is available, in which case the caller will probably need to
   * create one.
   */
  Layer* GetLeafLayerFor(nsDisplayListBuilder* aBuilder,
                         LayerManager* aManager,
                         nsDisplayItem* aItem);

The name GetLeafLayerFor does not indicate to me that it is only used for display items that make their own layer, as thebes layers are also leaves.


+  // Clear clip rect; we'll set it up again later.
+  layer->SetClipRect(nsnull);

Where will we set it?


+  fprintf(aStream, " [%s=", aName);
+  nsIntRegionRectIterator iter(aRegion);
+  const nsIntRect* r;
+  PRBool first = PR_TRUE;
+  while ((r = iter.Next()) != nsnull) {
+    if (!first) {
+      fputs(";", aStream);
+    } else {
+      first = PR_FALSE;
+    }
+    fprintf(aStream, "%d,%d,%d,%d", r->x, r->y, r->width, r->height);
+  }
+  fputs("]", aStream);

Does adding a space after ';' make the output more readable?


+  DumpIntRegion(aStream, "visible",
+                aLayer->GetVisibleRegion().GetBounds());

This could lead to some confusion when looking at the dump output, why do it?


 FrameLayerBuilder::DrawThebesLayer
+  nsPresContext* presContext = entry->mContainerLayerFrame->PresContext();
+  nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
+  nsRect r = (aRegionToInvalidate.GetBounds() + offset).
+    ToAppUnits(appUnitsPerDevPixel);
+  entry->mContainerLayerFrame->Invalidate(r);

So this invalidate will have to be served by the _next_ paint, not this one then I guess? That's too bad if true.


+  // Compensate for that translation as we render
+  gfxContextMatrixAutoSaveRestore saveMatrix(aContext); 
+  aContext->Translate(-gfxPoint(transform.x0, transform.y0));
+  nsIntPoint offset(PRInt32(transform.x0), PRInt32(transform.y0));

The comment would be more useful if it said something like "make the origin of the context coincide with the origin of our container layer".

So all container layer's origins are at the display list builder's reference frame's origin? That should be documented somewhere.



The definition of DrawThebesLayerCallback needs more comments.
It should say that the origin of the gfxContext is at the origin of the thebes layer.
What coordinate system are the two regions in?

What aRegionToDraw and aRegionToInvalidate actually mean and what to do with them is hard to tell. It's is not clear that aRegionToInvalidate has nothing to do with the drawing that the DrawThebesLayerCallback is supposed to be doing and aRegionToInvalidate has no relation to aRegionToDraw.


 FrameLayerBuilder::DrawThebesLayer
+    presContext = cdi->mItem->GetUnderlyingFrame()->PresContext();
+    if (presContext->AppUnitsPerDevPixel() != appUnitsPerDevPixel) {

Do we have some sort of guarantee the item has a underlying frame? And again later in the same function.

+  /**
+   * Like ComputeVisibility, but does the work that nsDisplayList
+   * does per-item:
+   * -- Intersects GetBounds with aVisibleRegion and puts the result
+   * in mVisibleRect
+   * -- Subtracts bounds from aVisibleRegion if the item is opaque
+   */
+  PRBool RecomputeVisibility(nsDisplayListBuilder* aBuilder,
+                             nsRegion* aVisibleRegion);

Worth it/possible to integrate into ComputeVisibility?

+    // Don't let the visible region get too complex.
+    if (newVisible.GetNumRects() <= 15) {
+      visible = newVisible;
+    }

We don't want to use one of the simplify functions of ns(Int)Region region?


I didn't look at any of the deleted code, so I hope none of it was relevant to reviewing this patch.
Comment on attachment 451820 [details] [diff] [review]
Part 12: Refactor RenderDocument to use nsLayoutUtils::PaintFrame

I think nsIPresShell needs an iid bump.

Can you document the PAINT_FLUSH_LAYERS flag?


   if (aFlags & PAINT_WIDGET_LAYERS) {
     flags |= nsDisplayList::PAINT_USE_WIDGET_LAYERS;
+    if (aFlags & PAINT_IGNORE_VIEWPORT_SCROLLING) {
+      // We're going to display something different from what we'd normally
+      // paint in a window, so make sure we flush out any retained layer
+      // trees before *and after* we draw
+      flags |= nsDisplayList::PAINT_FLUSH_LAYERS;
+    }
     nsIWidget *widget = aFrame->GetWindow();
     PRInt32 pixelRatio = presContext->AppUnitsPerDevPixel();
     nsIntRegion visibleWindowRegion(visibleRegion.ToOutsidePixels(pixelRatio));
     nsIntRegion dirtyWindowRegion(aDirtyRegion.ToOutsidePixels(pixelRatio));
 
     // XXX we should simplify this API now that dirtyWindowRegion always
     // covers the entire window
     widget->UpdatePossiblyTransparentRegion(dirtyWindowRegion, visibleWindowRegion);


I don't understand UpdatePossiblyTransparentRegion but we never called it in RenderDocument before. Do we want to do it in nsLayoutUtils::PaintFrame if we are doing a RenderDocument call?

Also, could any of the other flags to nsLayoutUtils::PaintFrame cause us to want to flush layers? PAINT_IGNORE_SUPPRESSION or PAINT_HIDE_CARET maybe?


+      nsPoint pos = rootScrollableFrame->GetScrollPosition();
+      visibleRegion.MoveBy(-pos);
+      aRenderingContext->Translate(pos.x, pos.y);

I think adding a comment saying that you are making visibleRegion be relative to the scrolled frame, and making the origin of aRenderingContext coincide with the origin of the scrolled frame makes it clearer what is happening here.


Your changes replace NS_TIME_FUNCTION_WITH_DOCURL with NS_TIME_FUNCTION_MIN(1.0) in PresShell::RenderDocument. I don't think that was intended.

The old RenderDocument saved the context before the clip, the new one doesn't.


  gfxContext::GraphicsOperator oldOperator = aThebesContext->CurrentOperator();
  if (oldOperator == gfxContext::OPERATOR_OVER) {
    // Clip to the destination rectangle before we push the group,
    // to limit the size of the temporary surface
    aThebesContext->Clip();
  }

It seems like this would just clip to the same rect that was clipped above, but that can't be, I must be misunderstanding what's going on here.
(In reply to comment #127)
> (From update of attachment 453556 [details] [diff] [review])
> In FrameBuilderLayer.h I'd like to see a slightly more detailed saying
> something like "frame layer builder takes as input a display list and it
> produces the frame property mapping item keys to layers and the layer manager
> user data listing frames that are in layers".

Done, or at least tried!

> In FrameLayerBuilder.h the functions AddLayerDisplayItem,
> AddThebesDisplayItem, and GetOldLayerFor have a comment saying they are
> private to FrameLayerBuilder. Can you mention that they are not made private
> so some functions in FrameLayerBuilder.cpp that aren't in FrameLayerBuilder
> can access them?

Done.

> DisplayItemDataEntry and ThebesLayerItemsEntry both define a copy constructor
> that does a deep copy of an array. Mention that it is unused but needs to be
> defined to work.

Good idea. I'll stick an NS_ERROR there.

> +typedef nsTHashtable<nsPtrHashKey<nsIFrame*> > ThebesLayerUserData;
> 
> This doesn't appear to be used anywhere, and could be wrong: the hashtable
> would have keys that are pointers to pointers to frames?

Well spotted. Removed.

> +NS_DECLARE_FRAME_PROPERTY(ThebesLayerInvalidRegionProperty, DestroyRegion)
> 
> The comment should say what type is stored in this property, and because it is
> a type with coordinates should say what coordinate system it is in.
> The comment should also say where this region gets added to and when the
> region is made valid.

Done.

> +  void InvalidateWholeLayer(Layer* aLayer);
> 
> This seems to be unused in the entire patch queue, remove it? If not, then I
> have comments about it.

Removed.

> +static PRUint8 gThebesDisplayItemLayerUserData;
> 
> This is actually used to mark thebes layers that are retained and is not used
> on non-retained thebes layers? Or is it just used to avoid a virtual call
> sometimes? It is not entirely clear why we need this and what it is used for
> exactly.

It's used to identify ThebesLayers that were created by FrameLayerBuilder and are therefore eligible for recycling to draw non-layer content. (I want display items to be able to create their own dedicated ThebesLayers in BuildLayer, if necessary.) I'll improve the comment.

> In FrameLayerBuilder::UpdateDisplayItemDataForFrame if there is no entry in
> mNewDisplayItemData for the frame we remove the frame from the layer manager's
> user data's mFramesWithLayers. If we remove that last entry of
> mFramesWithLayers from DestroyDisplayItemData then we delete the corresponding
> layer manager's user data, but not if we remove the last mFramesWithLayers
> entry in UpdateDisplayItemDataForFrame.

I added a comment to UpdateDisplayItemDataForFrame:
    // Note that InternalDestroyDisplayItemData would delete the user data
    // for the retained layer manager if it removed the last entry from
    // mFramesWithLayers, but we won't. That's OK because our caller
    // is DidEndTransaction, which would recreate the user data
    // anyway.

The layer manager user data will eventually be cleaned up when we delete all the frames for the window with the retained layer manager. I guess that wouldn't happen if there were no frames with layers, but that should be impossible, so I added
  NS_ASSERTION(data->mFramesWithLayers.Count() > 0,
               "Some frame must have a layer!");
at the end of DidEndTransaction. If it turns out to be possible (I don't see how), we could add code there to remove the userdata.

> +  NS_DECLARE_FRAME_PROPERTY_WITH_FRAME_IN_DTOR(DisplayItemDataProperty,
> +                                               DestroyDisplayItemData)
> 
> The fact that we store the nsTArray<DisplayItemData> itself in the property
> and not a pointer to it is a little unexpected and should be mentioned.

Done.

> mNewDisplayItemData items that are processed by StoreNewDisplayItemData don't
> get removed from mNewDisplayItemData because the frame layer builder will be
> going away shortly. You might want to mention that if we ever want to keep
> frame layer builders around longer in the future.

Probably better just to return PL_DHASH_REMOVE from StoreNewDisplayItemData so the hashtable is emptied as we go. Done.

> +static PRUint32
> +AppUnitsPerDevPixel(nsDisplayItem* aItem)
> +{
> +  return aItem->GetUnderlyingFrame()->PresContext()->AppUnitsPerDevPixel();
> +}
> 
> It wasn't immediately obvious to me that all the items we call this on are
> guaranteed to have an underlying frame.

Added comment:
 * Returns the appunits per dev pixel for the item's frame. The item must
 * have a frame because only nsDisplayClip items don't have a frame,
 * and those items are flattened away by ProcessDisplayItems.
We might want to change display lists so every item just contains a cliprect, and we don't have nsDisplayClip items, so every item has a frame. Food for thought.

> +  PRUint32 containerLayerCount = 0;
> Why not just use a bool and break?

Better still, use a helper function DisplayItemDataEntry::HasContainerLayer. Done.

> Do we want to make any attempt to remove the ThebesLayerInvalidRegionProperty
> property on frames that stop having a container layer?

Yes, we should remove it wherever we remove the NS_FRAME_HAS_CONTAINER_LAYER property. Good catch, fixed.

> +    NS_ASSERTION(!transform.HasNonIntegerTranslation(),
> +                 "Matrix not just an integer translation?");
> 
> HasNonIntegerTranslation doesn't do what I would expect given its name. What
> about reversing the logic and calling it HasOnlyIntegerTranslation?

Sounds reasonable but perhaps as a followup bug? I didn't add it here.

> "if we need to invalidate _the_ entire layer"

Fixed.

> +  // Set up transform so that 0,0 in the Thebes layer corresponds to the
> +  // (pixel-snapped) top-left of the aActiveScrolledRoot.
> 
> This comment seems important enough to be somewhere more prominent.

Added a comment about layer coordinate systems to the main comment in FrameLayerBuilder.h.

> +    // We do not need to add this area to mInvalidArea since we assume
> +    // the caller of InvalidateThebesLayerContents or
> +    // InvalidateAllThebesLayerContents has ensured
> +    // the area is invalidated in the widget.
> 
> What does this comment mean? There is no mInvalidArea in m-c or in the entire
> patch series that I can see.

Changed to
    // We do not need to Invalidate these areas in the widget because we
    // assume the caller of InvalidateThebesLayerContents or
    // InvalidateAllThebesLayerContents has ensured
    // the area is invalidated in the widget.

> In the comment make it clear that not all thebes layers will be in the stack,
> and this region is only concerned with layers that are currently in the stack.

Done.

> +  if (lastIndex > 0) {
> +    ThebesLayerData* nextData = mThebesLayerDataStack[lastIndex - 1];
> +    nextData->mVisibleAboveRegion.Or(nextData->mVisibleAboveRegion,
> +                                     data->mVisibleAboveRegion);
> +    nextData->mVisibleAboveRegion.Or(nextData->mVisibleAboveRegion,
> +                                     data->mVisibleRegion);
> +  }
> 
> Make a little comment here because it is a little more subtle then it first
> appears for the same reason.

Done.

> +    /** The region of visible content in the layer. */
> +    nsIntRegion  mVisibleRegion;
> 
> Say in the comment what coord system this is in.

Done.

> +    // Compensate for that translation as we render
> 
> A more useful comment would be "convert the visible region from being relative
> to A to being relative to B" where A and B are the correct things.

Done.

> +  data->mVisibleRegion.Sub(data->mVisibleRegion, data->mOpaqueRegion);
> 
> You're reusing data->mVisibleRegion as a temporary here because it's going
> away in 2 lines? Please comment that.

I was just being lazy. Introduced a named temporary instead.

> +  already_AddRefed<ThebesLayer> UpdateThebesLayerDataFor(const nsIntRect&
> aVisibleRect,
> +                                                         nsIFrame*
> aActiveScrolledRoot,
> +                                                         PRBool aIsOpaque);
> 
> In the comment you may as well also say that it will pop stuff from the stack
> if necessary too.
> 
> Indicate what the arguments actually represent.
> 
> I don't like the name of this function.

I changed it to FindThebesLayerFor. and updated the comment.

>    already_AddRefed<Layer> GetContainerLayerFor(nsDisplayListBuilder* aBuilder,
> 
> This function does a lot more than either its comment or name indicates. At
> least beef up the comment. And maybe a better function name.

Changed to BuildContainerLayerFor and beefed up the comment.

>  * We set the clip rect for items that generated their own layer.
>  * We set the visible rect for all layers except for the bottommost
>  * Thebes layer and the last Thebes layer.
>  */
> 
> Why not set the visible rect for the bottommost thebes layer? And what is the
> last thebes layer exactly and how does it differ from the bottommost? And why
> not set the visible rect for it too?

The comment is wrong, we set the visible region on all layers. I replaced it with:
 * We set the visible rect for all layers, although the actual setting
 * of visible rects for some ThebesLayers is deferred until the calling
 * of ContainerState::Finish.

>     nsRefPtr<Layer> ownLayer = item->BuildLayer(mBuilder, mManager);
> 
> Can we get an assertion here that the ownLayer is not a thebes layer?

No, because I want to allow that, but I can assert that its user data is not &gThebesDisplayItemLayerUserData. Done.

>       NS_ASSERTION(offsetToActiveScrolledRoot ==
> f->GetOffsetTo(activeScrolledRoot),
>                    "Wrong offset");
> 
> This assert will fail because the offset is not updated at all in
> GetActiveScrolledRootFor in this patch. I'm sure it must be in the entire
> patch series though.

Yes. Fixed in this patch by making nsLayoutUtils::GetActiveScrolledRootFor return the expected offset.

>         InvalidatePostTransformRegion(oldLayer->AsThebesLayer(), r);
>         InvalidatePostTransformRegion(thebesLayer, r);
> 
>         mContainerFrame->Invalidate(bounds -
> mBuilder->ToReferenceFrame(mContainerFrame));
> 
> So why do we need to invalidate the container frame here too?

InvalidatePostTransformRegion invalidates layer contents only. Invalidate is needed to ensure that the relevant area of the window is repainted. Added a comment:
        // Ensure the relevant area of the window is repainted.
        // Note that the area we're currently repainting will not be
        // repainted again, thanks to the logic in nsFrame::InvalidateRoot.

> +  void Finish();
> 
> May as well just say everything that it does and expand it to:
> 
>    * This finalizes all the open ThebesLayers by popping every element off
>    * mThebesLayerDataStack, then sets the children of the container layer
>    * to be all the layers in mNewChildLayers in that order and removes any
>    * layers as children of the container that aren't in mNewChildLayers.

Done.

>    // If there's only one layer, then in principle we can try to flatten
>    // things by returning that layer here. But that adds complexity to
>    // retained layer management so we don't do it. Layer backends can
>    // flatten internally.
> 
> I don't understand this comment.

It's vestigial from the way we used to flatten the layer tree during layer construction. Removed the comment.

> +    Layer* oldLayer = GetOldLayerFor(aContainerFrame,
> containerDisplayItemKey);
> +    if (oldLayer) {
> 
> Can you add an assert here that the layer is indeed a container layer?

Done.

> +      NS_ASSERTION(oldLayer->Manager() == aManager, "Wrong manager");
> +      containerLayer = static_cast<ContainerLayer*>(oldLayer);
> +      // Clear clip rect; we'll set it up again later.
> +      containerLayer->SetClipRect(nsnull);
> 
> Where will we set it?

Actually the caller has to set it. Fixed that comment (and the other one in GetLeafLayerFor), and added to both of those methods:
   * Returns a layer with clip rect cleared; it is the
   * caller's responsibility to add any clip rect and set the visible
   * region.

>   /**
>    * Get a retained layer for a leaf display item. Returns null if no
>    * layer is available, in which case the caller will probably need to
>    * create one.
>    */
>   Layer* GetLeafLayerFor(nsDisplayListBuilder* aBuilder,
>                          LayerManager* aManager,
>                          nsDisplayItem* aItem);
> 
> The name GetLeafLayerFor does not indicate to me that it is only used for
> display items that make their own layer, as thebes layers are also leaves.

It could conceivably be used to get a ThebesLayer, if a display item wanted to use a ThebesLayer as its custom layer.

> +      fputs(";", aStream);
> 
> Does adding a space after ';' make the output more readable?

Not particularly. It makes the region easier to read but the text around the
region harder to read, IMHO.

> +  DumpIntRegion(aStream, "visible",
> +                aLayer->GetVisibleRegion().GetBounds());
> 
> This could lead to some confusion when looking at the dump output, why do it?

You mean taking the GetBounds()? Indeed. Removed the GetBounds().

> +  nsRect r = (aRegionToInvalidate.GetBounds() + offset).
> +    ToAppUnits(appUnitsPerDevPixel);
> +  entry->mContainerLayerFrame->Invalidate(r);
> 
> So this invalidate will have to be served by the _next_ paint, not this one
> then I guess? That's too bad if true.

Nope. Thanks to the logic in InvalidateRoot added in Part 7, if that area of the window is being updated by the current paint, we won't invalidate it again.

> +  // Compensate for that translation as we render
> +  gfxContextMatrixAutoSaveRestore saveMatrix(aContext); 
> +  aContext->Translate(-gfxPoint(transform.x0, transform.y0));
> +  nsIntPoint offset(PRInt32(transform.x0), PRInt32(transform.y0));
> 
> The comment would be more useful if it said something like "make the origin of
> the context coincide with the origin of our container layer".

Done. Well, actually it's
  // make the origin of the context coincide with the origin of the
  // ThebesLayer

> So all container layer's origins are at the display list builder's reference
> frame's origin? That should be documented somewhere.

Yes, added to FrameLayerBuilder.h.

> The definition of DrawThebesLayerCallback needs more comments.
> It should say that the origin of the gfxContext is at the origin of the thebes
> layer.

Done.

> What coordinate system are the two regions in?

That of the ThebesLayer. Added comment.

> What aRegionToDraw and aRegionToInvalidate actually mean and what to do with
> them is hard to tell. It's is not clear that aRegionToInvalidate has nothing to
> do with the drawing that the DrawThebesLayerCallback is supposed to be doing
> and aRegionToInvalidate has no relation to aRegionToDraw.

OK, I've tried to improve the comment.

>  FrameLayerBuilder::DrawThebesLayer
> +    presContext = cdi->mItem->GetUnderlyingFrame()->PresContext();
> +    if (presContext->AppUnitsPerDevPixel() != appUnitsPerDevPixel) {
> 
> Do we have some sort of guarantee the item has a underlying frame? And again
> later in the same function.

Yes, all display items that actually render (i.e. not nsDisplayClip) have an underlying frame. Added a comment to ClippedDisplayItem and assertion to AddThebesDisplayItem.

> +  /**
> +   * Like ComputeVisibility, but does the work that nsDisplayList
> +   * does per-item:
> +   * -- Intersects GetBounds with aVisibleRegion and puts the result
> +   * in mVisibleRect
> +   * -- Subtracts bounds from aVisibleRegion if the item is opaque
> +   */
> +  PRBool RecomputeVisibility(nsDisplayListBuilder* aBuilder,
> +                             nsRegion* aVisibleRegion);
> 
> Worth it/possible to integrate into ComputeVisibility?

The thing is, nsDisplayItem::ComputeVisiblity is virtual and currently does only the processing that's item-specific. RecomputeVisibility has to do generic work both before and after calling the item-specific code. So if we combine ComputeVisibility with RecomputeVisibility we'd have to duplicate code or have two helper functions which every implementation of ComputeVisibility calls before and after its custom processing.

Or if you mean calling RecomputeVisiblity from nsDisplayList::ComputeVisibility, we can probably do that, but first we need to disentangle the old display list scroll analysis code. It'll be much easier to do that after all the retained layers stuff has landed.

> +    // Don't let the visible region get too complex.
> +    if (newVisible.GetNumRects() <= 15) {
> +      visible = newVisible;
> +    }
> 
> We don't want to use one of the simplify functions of ns(Int)Region region?

No. Adding a rectangle and then simplifying inward would be no better, and it wouldn't perform nearly as well.
(In reply to comment #128)
> (From update of attachment 451820 [details] [diff] [review])
> I think nsIPresShell needs an iid bump.

Done.

> Can you document the PAINT_FLUSH_LAYERS flag?

Done.

>      // XXX we should simplify this API now that dirtyWindowRegion always
>      // covers the entire window
>      widget->UpdatePossiblyTransparentRegion(dirtyWindowRegion,
> visibleWindowRegion);
> 
> I don't understand UpdatePossiblyTransparentRegion but we never called it in
> RenderDocument before. Do we want to do it in nsLayoutUtils::PaintFrame if we
> are doing a RenderDocument call?

Good catch. We should skip it when we aren't painting what the window would normally paint, in particular when PAINT_IGNORE_VIEWPORT_SCROLLING is set. Fixed.

> Also, could any of the other flags to nsLayoutUtils::PaintFrame cause us to
> want to flush layers? PAINT_IGNORE_SUPPRESSION or PAINT_HIDE_CARET maybe?

Yes we should. Fixed!

> +      nsPoint pos = rootScrollableFrame->GetScrollPosition();
> +      visibleRegion.MoveBy(-pos);
> +      aRenderingContext->Translate(pos.x, pos.y);
> 
> I think adding a comment saying that you are making visibleRegion be relative
> to the scrolled frame, and making the origin of aRenderingContext coincide
> with the origin of the scrolled frame makes it clearer what is happening here.

Done.

> Your changes replace NS_TIME_FUNCTION_WITH_DOCURL with
> NS_TIME_FUNCTION_MIN(1.0) in PresShell::RenderDocument. I don't think that was
> intended.

Fixed.

> The old RenderDocument saved the context before the clip, the new one doesn't.

Oops, great catch. That Clip should simply not be there!!! Removed.
Attached patch Part 9 v3Splinter Review
Updated according to comments.
Attachment #453556 - Attachment is obsolete: true
Attachment #456071 - Flags: review?(tnikkel)
Attachment #453556 - Flags: review?(tnikkel)
Attached patch Part 12 v2Splinter Review
Updated according to comments.
Attachment #451820 - Attachment is obsolete: true
Attachment #456073 - Flags: review?(tnikkel)
Attachment #451820 - Flags: review?(tnikkel)
Comment on attachment 456071 [details] [diff] [review]
Part 9 v3

+ * responsible for converting display lists into a layer trees.

"a layer trees" -> "a layer tree" or "layer trees"

+    // Note that InternalDestroyDisplayItemData would delete the user data
+    // for the retained layer manager if it removed the last entry from
+    // mFramesWithLayers, but we won't. That's OK because our caller
+    // is DidEndTransaction, which would recreate the user data
+    // anyway.

I think this would be clearer with s/InternalDestroyDisplayItemData/DestroyDisplayItemData/

>> The name GetLeafLayerFor does not indicate to me that it is only used for
>> display items that make their own layer, as thebes layers are also leaves.
>
> It could conceivably be used to get a ThebesLayer, if a display item wanted to
> use a ThebesLayer as its custom layer.

What I was getting at is that the name GetLeafLayerFor does not indicate that the function is only used for display items that make their own frame and there are leaf layers that this function does not deal with.
Attachment #456071 - Flags: review?(tnikkel) → review+
Attachment #456073 - Flags: review?(tnikkel) → review+
(In reply to comment #134)
> All done.

mark as resolved?
(In reply to comment #135)
> (In reply to comment #134)
> > All done.
> 
> mark as resolved?

I believe roc means he is done submitting patches.  These still need to land, of course ;-)
(In reply to comment #136)
> (In reply to comment #135)
> > (In reply to comment #134)
> > > All done.
> > 
> > mark as resolved?
> 
> I believe roc means he is done submitting patches.  These still need to land,
> of course ;-)

I tested the most recent try build this am, I mean some of things I see now, are the browser doesn't repaint when there are mouseovers like over tabs/switching tabs, menu items, and other things like putting the caret in awesomebar, typing didn't show up there.  The scrollbars didn't update when I used the mouse wheel or drag the scroll thumb.  Some of it I saw before, but some of it I think is more recent.
Afternoon try build 4b0436a4faf8 test update:

Looks like the try build from this afternoon cleaned up a lot.  The only thing left I see is any place where there is glass background showing, either with the menu bar enabled or toolbars (default tabs on top, or with tabs on bottom - see nav bar) with having opacity or transparency the widgets or tabs are still not painting correctly, but the rest of the issues mentioned in comment 137 seems to have disappeared.
Attached image Aero glass painting problem (obsolete) —
Aero glass painting problem described in comment 138. Other than this issue the latest try build seems perfectly fine. Sorry if this isnt the proper place to discuss this.
I have no problems so far with the try server build.  Scrolling is insanely fast and I haven't come across any painting problems as mentioned above.  Using Aero btw.  Can't wait till it lands on M-C.

Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100706 Minefield/4.0b2pre - Build ID: 20100706114733
(In reply to comment #140)
> I have no problems so far with the try server build.  Scrolling is insanely
> fast and I haven't come across any painting problems as mentioned above.  Using
> Aero btw.  Can't wait till it lands on M-C.

I think we're seeing the aero glass issues only in normal rendering mode.  With D2D/DW enabled the aero glass issues don't seem to be a problem.  Installing a persona also hides the the problem with glass.
OK.  With D2D disabled I see all kinds of painting problems on the tabs and even the navigation bar.  Isn't D2D enabled going to be the default eventually?
(In reply to comment #142)
> OK.  With D2D disabled I see all kinds of painting problems on the tabs and
> even the navigation bar.  Isn't D2D enabled going to be the default eventually?

Eventually, yes, but probably not for beta2, and retained layers is (as far as I know) supposed to be fully working for beta2. Not to mention the fact that scrolling performance is noticeably worse with d2d turned on for some reason, but I have no idea if thats a d2d issue or retained layers issue.
With the afternoon tryserver build, a lot of painting issues on MSNBC video player on Windows XP.  Play a video and, while the video is still playing, hover your mouse over various entries in the playlist and also the video thumbnails.  Soon video gets blocking, choppy, slow, with frame skipping.  Also the progress bar does not paint properly (this part I also see with abcnews.go.com video player.

Use a new profile to test or disable any and all the Direct X-related acceleration.
Attached patch Part 31 v2Splinter Review
We're getting assertion failures about not having called ComputeVisibility before we call IsOpaque on a display item (and possibly other methods), e.g. from ContainerState::ProcessDisplayItems. The problem is that, well, we haven't called ComputeVisibility on those display items and because of Part 31 we haven't removed them from the display list.

Mats asked why we skip non-visible items that build their own layers, but not ThebesLayer items. The reason is because the ThebesLayer items may end up needing to be drawn because they're not *currently* visible, but the ThebesLayer wants to draw them anyway. See comment #125.

So we fix these assertions by ensuring that ComputeVisibility is called on all descendent items whenever we call it on a list. The main thing is that nsDisplayList::ComputeVisibility does not check mVisibleRect.IsEmpty() before calling ComputeVisibility on the child item. Instead each leaf child item checks mVisibleRect.IsEmpty() before doing more work.
Attachment #455636 - Attachment is obsolete: true
Attachment #456223 - Flags: review?(matspal)
Depends on: 577200
Today's tryserver build is still really bad with flash videos on msnbc and abcnews.  On msnbc, you still get what I described in comment 144; and on abcnews, from the lower quarter-inch of the video often does not get updated at all, and the progress bar is very rarely updated.

Again, you'll need to test with a clean profile with absolutely not D2D stuff enabled.  If you've got an XP machine to test on, even better.
Here's what a youtube video looks like with they July 7 tryserver build.
Aero glass painting problem is fixed in latest try build. And I cant reproduce the bug reported in comment 146 either, but I'm not sure if its fixed because that only happened to me like 2 times in several hours of use.
(In reply to comment #148)
> And I cant reproduce
> the bug reported in comment 146 either, but I'm not sure if its fixed because
> that only happened to me like 2 times in several hours of use.

It still happens with the latest build.  See the attached screenshot for abcnews.go.com.  

Of course, you're using Windows 7, whereas I mentioned that I see this with Windows XP.

The specific URL in the screenshot is: http://abcnews.go.com/Entertainment/video/levi-johnston-apologizes-to-palin-family-11105009
IU, that screenshot shows the same issues on the nightly also on Windows 7, and it happens on IE8 also.  Its the videos and their webpage player designed progress bars are hard to see all the pieces to it.

Roc, i can also confirm the aero glass issues were gone with today's try build, though I did see a youtube case paint issue as shown and commented already.  Otherwise it looks fine.
Hmm.  Turns out you're right, except with regards to the progress bar.  For me, that is definitely not getting painted properly with the tryserver build but does with everything else.

As for the youtube issue (previous screenshot) and msnbc issues, those definitely remain.

I realize many people are eager to see this land (myself included), but these glitches will need to be fixed sometime -- especially the youtube one, which is hard to miss.
From what I can tell, the new tab text is already there when a new tab is opened, and the tab animation draws on top of it, causing a small text distortion sometimes.
Comment on attachment 456390 [details]
New tab text problem with latest try-server build

This happens on trunk now and isn't caused by these patches.
Attachment #456390 - Attachment is obsolete: true
Can we get a bug filed for that then?
On Windows XP, scrolling on this website (http://www.300mbmovie.com/) is better than 3.6.6, (not so much) but it's still not good - it's still slow. Configuration is Athlon 64 2800+, Ati Radeon 9550. 
On Windows 7 and Athlon X2 4800+, GeForce 8400GS it's hella fast.
This was a bit of a nasty one. We were getting a few strange reftest failures on Windows which I tracked down to the fact that usually our ThebesLayers would be backed by a Win32 DDB surface but sometimes they'd be backed by a Win32 DIB surface. The former happened when the first draw was to the window, in which case the create-similar-surface would create a DDB, but sometimes the first draw would be via drawWindow in which case the destination surface is a DIB so the create-similar-surface would create a DIB.

The fix is slightly involved. We need to pass in the widget to the BasicLayerManager when we have one, so that if we don't have a default target context, we can use the widget to get a surface to be similar to. See the comments in BasicLayerManager.

It turns out that we really only need two kinds of BasicLayerManagers:
1) Retained managers that have an associated nsIWidget
2) Non-retained managers that will have their target set by SetDefaultTarget or BeginTransactionWithTarget
so I refactored the code to make that clear.

To avoid calling widget->GetThebesSurface too often, I use a bit of a trick ... if the ThebesLayer already has a buffer, we can use *that* as the surface to be similar to.

This should fix the last of the known test failures...
Attachment #456552 - Flags: review?(jones.chris.g)
Attachment #456550 - Flags: review?(jones.chris.g) → review+
Blocks: 361754
Attachment #456551 - Flags: review?(tnikkel) → review+
Comment on attachment 456551 [details] [diff] [review]
Part 33: Copy data out of hashtable in DrawThebesLayer in case hashtable is modified during the method.

The question that comes to mind is: do we hold pointers to entries in hashtables anywhere else in frame layer builder where the hashtable can be modified?
(In reply to comment #4)
> Created an attachment (id=451468) [details]
> Part 3: Create unique nsDisplayItem types for every single display item.
> 
> 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.

There may be more than one text-decoration item for a frame, eg one for underline and one for strikethrough. Would this affect things?
(In reply to comment #160)
> There may be more than one text-decoration item for a frame, eg one for
> underline and one for strikethrough. Would this affect things?

Doesn't the patch in this bug address that? Not sure if it was in your original patch or not.
(In reply to comment #159)
> (From update of attachment 456551 [details] [diff] [review])
> The question that comes to mind is: do we hold pointers to entries in
> hashtables anywhere else in frame layer builder where the hashtable can be
> modified?

Doesn't look like it.
Comment on attachment 451468 [details] [diff] [review]
Part 3: Create unique nsDisplayItem types for every single display item.

>diff --git a/layout/base/nsLayoutDebugger.cpp b/layout/base/nsLayoutDebugger.cpp
>--- a/layout/base/nsLayoutDebugger.cpp
>+++ b/layout/base/nsLayoutDebugger.cpp
>@@ -180,20 +180,16 @@ PrintDisplayListTo(nsDisplayListBuilder*
>             i->IsUniform(aBuilder, &color) ? " uniform" : "",
>             f && aBuilder->IsMovingFrame(f) ? " moving" : "",
>             f && aBuilder->IsMovingFrame(f) && !i->GetList() &&
>               i->IsVaryingRelativeToMovingFrame(aBuilder) ? " varying" : "");
>     nsDisplayList* list = i->GetList();
>     if (list) {
>       PrintDisplayListTo(aBuilder, *list, aIndent + 4, aOutput);
>     }
>-    if (i->GetType() == nsDisplayItem::TYPE_TRANSFORM) {
>-      nsDisplayTransform* t = static_cast<nsDisplayTransform*>(i);
>-      PrintDisplayListTo(aBuilder, *(t->GetStoredList()->GetList()), aIndent + 4, aOutput);
>-    }
>   }
> }

I don't think this should be part of the patch. This was part of a review comment in the original bug.
That's not in the latest version of my patch queue.
Comment on attachment 456591 [details] [diff] [review]
Part 35: Fix text decorations to have a unique GetPerFrameKey

Good catch, Michael? We need to make nsDisplayTextDecoration's GetPerFrameKey unique by adding the mDecoration to the key.
Attachment #456591 - Flags: review? → review?(tnikkel)
(In reply to comment #161)
> (In reply to comment #160)
> > There may be more than one text-decoration item for a frame, eg one for
> > underline and one for strikethrough. Would this affect things?
> 
> Doesn't the patch in this bug address that? Not sure if it was in your original
> patch or not.

It was in the original patch, I solved it with an extra virtual function allowing items to compare themselves after matching pointer and type, but bitwise shift and OR is sexier :)
Attachment #456552 - Flags: review?(jones.chris.g) → review+
Comment on attachment 456552 [details] [diff] [review]
Part 34: Make sure that ThebesLayerBuffers are always allocated as similar surfaces to the widget surface, whenever possible

>diff --git a/gfx/layers/basic/BasicLayers.h b/gfx/layers/basic/BasicLayers.h
>--- a/gfx/layers/basic/BasicLayers.h
>+++ b/gfx/layers/basic/BasicLayers.h
>@@ -40,62 +40,76 @@
>+  /**
>+   * Construct a BasicLayerManager which will have no default
>+   * target context. SetDefaultTarget or BeginTransactionWithTarget
>+   * must be called for any rendering to happen. ThebesLayers will be
>+   * retained; that is, we will try to retain the visible contents of
>+   * ThebesLayers as cairo surfaces. We creating ThebesLayer buffers by

s/creating/create/

>diff --git a/gfx/layers/basic/ThebesLayerBuffer.h b/gfx/layers/basic/ThebesLayerBuffer.h
>--- a/gfx/layers/basic/ThebesLayerBuffer.h
>+++ b/gfx/layers/basic/ThebesLayerBuffer.h
>@@ -102,31 +102,40 @@ public:
>+  /**
>+   * Get the underlying buffer, if any. This is useful because we can pass
>+   * in the buffer as the default "reference surface" if there is one.
>+   * Don't use it for anything else!
>+   */
>+  gfxASurface* GetBuffer() { return mBuffer; }

It would be nice to return |const gfxASurface*|, but gfxASurface
methods aren't properly const-ified, so meh.

>diff --git a/widget/src/windows/nsWindow.cpp b/widget/src/windows/nsWindow.cpp
>--- a/widget/src/windows/nsWindow.cpp
>+++ b/widget/src/windows/nsWindow.cpp
>@@ -3255,17 +3255,22 @@ gfxASurface *nsWindow::GetThebesSurface(
> #if defined(MOZ_XUL)
>     if (mTransparencyMode != eTransparencyOpaque) {
>       content = gfxASurface::CONTENT_COLOR_ALPHA;
>     }
> #endif
>     return (new gfxD2DSurface(mWnd, content));
>   } else {
> #endif
>-    return (new gfxWindowsSurface(mWnd));
>+    PRUint32 flags = gfxWindowsSurface::FLAG_TAKE_DC;
>+    if (mTransparencyMode != eTransparencyOpaque) {
>+        gfxWindowsSurface::FLAG_IS_TRANSPARENT;
>+    }
>+    HDC dc = ::GetDC(mWnd);
>+    return (new gfxWindowsSurface(dc, flags));

Not my area of expertise, but I'm not sure this code is correct.
gfxWindowsSurface will call DeleteDC() if it thinks it owns the DC and
isn't created with a window, but MSDN says that ReleaseDC() must be
called for GetDC() contexts.

r+ conditional on someone else chiming in on the HDC issue.  Looks good
otherwise.
(In reply to comment #161)
> (In reply to comment #160)
> > There may be more than one text-decoration item for a frame, eg one for
> > underline and one for strikethrough. Would this affect things?
> 
> Doesn't the patch in this bug address that? Not sure if it was in your original
> patch or not.

Whoops, I was thinking text shadow.
(In reply to comment #168)
> s/creating/create/

Done

> Not my area of expertise, but I'm not sure this code is correct.
> gfxWindowsSurface will call DeleteDC() if it thinks it owns the DC and
> isn't created with a window, but MSDN says that ReleaseDC() must be
> called for GetDC() contexts.

Very good catch. I'll attach a new patch.
Comment on attachment 456591 [details] [diff] [review]
Part 35: Fix text decorations to have a unique GetPerFrameKey

Add an assert that the display item types fit in 8 bits somewhere?
Attachment #456591 - Flags: review?(tnikkel) → review+
(In reply to comment #171)
> (From update of attachment 456591 [details] [diff] [review])
> Add an assert that the display item types fit in 8 bits somewhere?

OK.
Attached patch Part 34 v2Splinter Review
As noted by Chris, we need to use the HWND constructor for gfxWindowsSurface here.
Attachment #456552 - Attachment is obsolete: true
Attachment #456607 - Flags: review?(jmuizelaar)
Attachment #456607 - Flags: review?(jmuizelaar) → review+
Comment on attachment 456607 [details] [diff] [review]
Part 34 v2

I don't really like the implicit parameter passing that happens with InitDC but nothing substantially better jumps to mind.
Another quick update: testing Win7 try build aac11301a874 from this am.

Msnbc on mouseover of video pics-drawing the popups didn't paint properly, and as noted before leaving the mouse sitting on the video while its playing doesn't really paint the progress bar meter continually, have to move the mouse off the video. 

Abcgo's mouseover the progress meter is a little better, the popup for the time draws more often properly when doing a mouseover while playing video.  

Youtube playing video, I had some clipping or invalidation issue there when load, and then scrolling up and down quickly and repeat with the progress bar not continually drawing properly. Didn't appear to be large regions of the video not showing though.
(In reply to comment #149)
> The specific URL in the screenshot is:
> http://abcnews.go.com/Entertainment/video/levi-johnston-apologizes-to-palin-family-11105009

I'm not seeing any problem here on Windows XP, nor on Youtube, with a debug build that *should* be equivalent to the build here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/rocallahan@mozilla.com-ae65611a6069/tryserver-win32-debug/
(In reply to comment #176)
> I'm not seeing any problem here on Windows XP, nor on Youtube, with a debug
> build that *should* be equivalent to the build here:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/rocallahan@mozilla.com-ae65611a6069/tryserver-win32-debug/

Youtube is hit and miss; however, I can't see how you're unable to reproduce the abcnews issue -- especially since I can reproduced it both on real hardware and through VirtualBox.  When you're trying to reproduce, for the abcnews link, make sure your mouse pointer is nowhere near the video.  You will notice that the video progress bar does not update.  If you're moving your mouse pointer over the video or frame, the progress bar will update.  Compare this with a non-retained layers build and you will see that this is not normal.  In this screenshot, I highlight the problem region.

Unless debug builds render very differently, you should be able to reproduce this, if the build you linked to (32-bit I presume) is equivalent to the non-debug build (I've never figured out how to use the debug builds) available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/rocallahan@mozilla.com-ae65611a6069/tryserver-win32/

Maybe you are using an older Flash Player build and you need to update to 10.1?  Does your browser profile have default settings?  Barring that, maybe try with the non-debug build.
I can confirm the progress bar issue on the ABC video.

Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100711 Minefield/4.0b2pre - Build ID: 20100712011651
http://www.msnbc.msn.com/id/21134540/vp/38201212#38201212

Move the pointer up and down the category list on the left and see that the highlighting sometimes remains when you move to a different category. Also the painting is not 'clean'
roc, your latest try-server build (Built from http://hg.mozilla.org/try/rev/ae65611a6069) has considerably lower scrolling performance on several pages (for instance http://www.youtube.com/user/programagalofrito), is this performance regression intended/necessary or could this be fixed?
A few people on Mozillazine have noted some performance issues while scrolling, specifically the unpredictable change in scrolling speeds.

This will be much easier to pin down when this as landed since there will be more testers.

If we can pin something down should we file a bug or add a comment here?
Ryan, a comment here with the affected pages would be good.

I see the painting problems on ABC, I'm looking into it.
(In reply to comment #182)
> I see the painting problems on ABC, I'm looking into it.

Er, I mean the MSNBC problem in comment #179.
OK, I've figured out the plugin painting problem. In FrameLayerBuilder::BuildContainerLayerFor we retrieve the ThebesLayerInvalidRegionProperty for the frame and invalidate that region in the ThebesLayers before painting. After painting, we clear the ThebesLayerInvalidRegionProperty for the frame, in FrameLayerBuilder::UpdateDisplayItemDataForFrame. The problem is that during painting we can get an invalidation message from the plugin that adds to the ThebesLayerInvalidRegionProperty! So this added area can be lost.
I see a rendering regression on many wikipedia pages using this try server build and D2D off

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/rocallahan@mozilla.com-ae65611a6069/

try this page http://it.wikipedia.org/wiki/Estate , scroll up and down changing the scroll speed , a black rectangle will appear on top with many other rendering artifacts
(In reply to comment #185)
> try this page http://it.wikipedia.org/wiki/Estate , scroll up and down changing
> the scroll speed , a black rectangle will appear on top with many other
> rendering artifacts

I see a black flash when scrolling near the top, which is definitely a bug I'll look into, but nothing else.

(In reply to comment #180)
> roc, your latest try-server build (Built from
> http://hg.mozilla.org/try/rev/ae65611a6069) has considerably lower scrolling
> performance on several pages (for instance
> http://www.youtube.com/user/programagalofrito), is this performance regression
> intended/necessary or could this be fixed?

I'm looking into this.
While testing some plugin stuff I noticed that we sometimes show a windowed plugin at the top-left of the content window on Windows. This is because when we instantiate a plugin, we give it an initial position of (0,0) and now that plugin geometry changes are asynchronous (as we try to coincide with paints), the plugin is actually visible there briefly.

The solution is to clip the plugin away completely initially. When the plugin geometry is eventually calculated, the clip region will be reset to the correct value.
Attachment #457051 - Flags: review?(matspal)
I need this to test plugins invalidating themselves during a paint.
Attachment #457052 - Flags: review?(joshmoz)
This patch addresses the problem described in comment #184.

We move the setup of frame state bits and ThebesLayerInvalidRegion regions from DidEndTransaction to WillEndTransaction so that as we paint during the transaction's drawing phase, Invalidate will work without losing regions from ThebesLayerInvalidRegionProperty values.

We also clarify the behaviour of invalidation during the paint phase. Now, by default any area invalidated during paint will be painted again ... this is what we want when we receive a plugin invalidation IPC message during painting, since we don't know whether what the plugin painted was up to date in the invalid area or not. (Indeed, we might be paint plugin A, then move on to paint plugin B and receive an invalidation request for plugin A.) However, in FrameLayerBuilder due to layer changes we need to invalidate to ensure the layer area is repainted, but if that area is already being painted in the current paint we don't need to repaint it again. So we add an extra invalidation flag nsIFrame::INVALIDATE_EXCLUDE_CURRENT_PAINT to represent that. (In the previous patches in this bug, this behaviour was the default for invalidation during painting, but in retrospect that's a bad default.)
Attachment #457053 - Flags: superreview?(dbaron)
Attachment #457053 - Flags: review?(matspal)
about http://it.wikipedia.org/wiki/Estate if you scroll slow enough like pixel by pixel near the top, the fast black flash ends up like this:

http://yfrog.com/0dss1qwuj

http://yfrog.com/45ss2ej

i can reproduce that on almost every wikipedia page
Comment on attachment 456223 [details] [diff] [review]
Part 31 v2

Use @return and move it to the end of the comment?
Attachment #456223 - Flags: review?(matspal) → review+
Sorry, the last comment got truncated.  Here's what it should say:
In layout/base/nsDisplayList.h
> +   * Returns true if any item in the list is visible.

Use @return and move it to the end of the comment?
Attachment #457051 - Flags: review?(matspal) → review+
Comment on attachment 457053 [details] [diff] [review]
Part 38: Make invalidation-during-paint work reliably

In layout/generic/nsIFrame.h
>    enum {
>    	INVALIDATE_IMMEDIATE = 0x01,
>    	INVALIDATE_CROSS_DOC = 0x02,
>    	INVALIDATE_REASON_SCROLL_BLIT = 0x04,
>    	INVALIDATE_REASON_SCROLL_REPAINT = 0x08,
>      INVALIDATE_REASON_MASK = INVALIDATE_REASON_SCROLL_BLIT |
>                               INVALIDATE_REASON_SCROLL_REPAINT,
> -    INVALIDATE_NO_THEBES_LAYERS = 0x10
> +    INVALIDATE_NO_THEBES_LAYERS = 0x10,
> +    INVALIDATE_EXCLUDE_CURRENT_PAINT = 0x20
>    };

Please untabify this region.
Attachment #457053 - Flags: review?(matspal) → review+
Attachment #457052 - Flags: review?(joshmoz) → review+
We were having performance problems because we'd sometimes fail to mark content ThebesLayers as opaque when we should. The problem is that even when the union of visible opaque regions covers the entire content window, we can get into situations where mOpaqueRegion gets too complex and we empty it. This can happen even if the canvas background is opaque, since we only pass in the *visible* part. In this Wikipedia pages the visible part of the canvas background is a lot smaller than the entire window because there are opaque images covering it.

So this patch makes us a) pass in the full opaque bounds of the item and b) never discard opaque areas that we found lower in z-order. The latter is a heuristic because we know the canvas background is likely to be the most important opaque area.
Attachment #457170 - Flags: review?(tnikkel)
There is no need to check for opacity==1.0 here. It's perfectly fine to have an OPAQUE_CONTENT ThebesLayerBuffer and then paint it with partial opacity.
Attachment #457171 - Flags: review?(bas.schouten)
This was contributing to the Wikipedia problems. Sometimes we'd call Clear() on our ThebesLayerBuffer(), but we'd carry on and exclude the ThebesLayer's ValidRegion from our mRegionToDraw. We can't do that, we need to redraw the entire visible region because the ValidRegion is going to be flushed.
Attachment #457173 - Flags: review?(jones.chris.g)
Attachment #457171 - Flags: review?(bas.schouten) → review+
Comment on attachment 457173 [details] [diff] [review]
Bug 564991. Part 41: When we clear our buffer, we need to redraw the entire visible region

Oops sorry, should have caught this first time around.
Attachment #457173 - Flags: review?(jones.chris.g) → review+
(other than the two known random ones)
Latest tryserver build seem to have fixed painting problems with plugins and also improved performance on cases where the previous build was struggling. However performance is still quite bad on some cases, usually with pages where the background image resolution is higher than the browser's web content resolution/area (not quite sure whats the name of this but I think you'll understand). 
For instance, on https://bug558293.bugzilla.mozilla.org/attachment.cgi?id=438067 this build beats any other browser, but on http://www.youtube.com/user/programagalofrito we are as slow as 3.6 and much slower than chrome/opera. But since this is quite uncommon and very hard to reproduce this shouldnt delay retained layers from landing, after all in worst case scenarios we will still be faster than 3.6 and you could fix this regression later.
Btw I'm still seeing https://bug564991.bugzilla.mozilla.org/attachment.cgi?id=456390 on retained layers build and not on trunk, I think khuey might have confused this by some other bug, are you sure this isn't caused by your patches?
Comment on attachment 457170 [details] [diff] [review]
Part 39: Accumulate the full opaque rect of opaque display  items, and don't simplify away opaque areas of display items low in z-order

>   already_AddRefed<ThebesLayer> FindThebesLayerFor(const nsIntRect& aVisibleRect,
>                                                    nsIFrame* aActiveScrolledRoot,
>-                                                   PRBool aIsOpaque,
>+                                                   const nsIntRect* aOpaqueRect,
>                                                    nscolor* aSolidColor);

Can you update the comment above this function?
Attachment #457170 - Flags: review?(tnikkel) → review+
Another testcase for poor scrolling performance (and for some reason weird background painting too): http://majornelson.com/default.aspx
This happens on latest tryserver build, in any web page, as long as you have D2D/DW enabled and scroll up and down quickly.
Attachment #456203 - Attachment is obsolete: true
With the latest try server build I'm seeing some quite bad repainting problems on the css Zen Garden page MS made for IE7.

http://samples.msdn.microsoft.com/workshop/samples/author/css/cssZenGarden.htm

Scrolling up and down causes "bleeding", and zooming in and out doesn't repaint properly.
Plugin-related painting issues are definitely resolved with the rocallahan@mozilla.com-d5784e62bb98 build, based on my testing.  Thanks, ROC.
(In reply to comment #203)
> weird background painting too): http://majornelson.com/default.aspx

Seeing this with d5784e62bb98 on XP (without D2D), too.
(In reply to comment #205)
> With the latest try server build I'm seeing some quite bad repainting problems
> on the css Zen Garden page MS made for IE7.
> 
> http://samples.msdn.microsoft.com/workshop/samples/author/css/cssZenGarden.htm
> 
> Scrolling up and down causes "bleeding", and zooming in and out doesn't repaint
> properly.

Seeing this with d5784e62bb98 on XP (without D2D), too.
(In reply to comment #203)
> Another testcase for poor scrolling performance (and for some reason weird
> background painting too): http://majornelson.com/default.aspx

I can also see this, with D2D and without D2D enabled. I am also seeing some strange issues with links. Scrolling usually fixes it.
Comment on attachment 457053 [details] [diff] [review]
Part 38: Make invalidation-during-paint work reliably

>+   * areas that invalidated while currently being painted will be repainted

s/that invalidated/that are invalidated/
s/currently //


sr=dbaron, although I'm curious how you decided which callers should pass this flag and which shouldn't.  (Would it be good to say that in the code comments?)
Attachment #457053 - Flags: superreview?(dbaron) → superreview+
er, comment 189 sort of answers that, but it might be good to reflect in in comments (i.e., say that the flag is largely for the layer manager's own use, or some such)
There's a bug when zooming. For example, load http://majornelson.com/Themes/Blogs/majornelson/style/images/NXE.png, zoom out a few times, then zoom in. When zooming in, only the top-left portion of the window is updated.

The problem is simply that nothing triggers invalidation when the size of the root frame changes. We need to invalidate in the widget and any ThebesLayer children of the root layer. This patch does that.

The patch doesn't work unless I also remove the logic in nsFrame::InvalidateRoot that clips the invalid area to the frame bounds, since here the area we want to invalidate is actually outside the current frame bounds. That clipping was added in Part 7, but is actually not needed; it's perfectly OK, indeed desirable to invalidate the area of ThebesLayers that's outside the root frame bounds, and it's also safe to invalidate that area of the widget, since the root frame's widget should be the size of the root frame anyway.
Attachment #457438 - Flags: review?(tnikkel)
Comment on attachment 457438 [details] [diff] [review]
Part 42: When the root frame changes size, invalidate the changed area

>+     // nsFrame::InvalidateRoot clips the invalid area to the root frame
>+     // bounds so we can't use it here.

Don't you make this comment false later in this patch?
yes, sorry. Consider that comment removed!
Comment on attachment 457438 [details] [diff] [review]
Part 42: When the root frame changes size, invalidate the changed area

>+     target->InvalidateRectDifference(mPresContext->GetVisibleArea(),
>+                                      target->GetRect());

Can you add a comment here saying target->GetRect() is the old size and mPresContext->GetVisibleArea() will be the new size of the root frame?
Attachment #457438 - Flags: review?(tnikkel) → review+
The update rect here was wrong. We should be using the scrollport, not the bounds of the scrolled frame (which are generally the size of the scrollport, but offset by the scroll position).

We also need to fix a few assertions:
1) don't call IsVaryingRelativeToMovingFrame without calling IsMovingFrame first
2) delete display items from list in InvalidateFixedBackgroundFrames
Attachment #457481 - Flags: review?(tnikkel)
In some of the pages people have been looking at, there are background-attachment:fixed backgrounds on elements that don't propagate to the canvas background, but still cover the viewport scrollport. We can accelerate those with layers as well.

I'm also slipping in a bugfix:
+    if (layer.mClip != NS_STYLE_BG_CLIP_BORDER)
+      return PR_FALSE;
nsDisplayBackground::GetBounds always returns the border-box, so this optimization isn't safe if we're clipping to within the bounds ... the display item might cover the viewport scrollport, but need to be invalidated after scrolling anyway since the content-box or padding-box edge might be visible in the scrollport.

I've also added:
+  if (nsLayoutUtils::HasNonZeroCorner(mFrame->GetStyleBorder()->mBorderRadius))
+    return PR_FALSE;

Since rounded borders can also interfere with the assumption that the movement of the frame during scrolling doesn't affect rendering.
Attachment #457483 - Flags: review?(tnikkel)
I've just done a new tryserver push c11c3f73a0a2, with the latest patches, updated for review comments, and everything updated to trunk (rev 4dd7bbd2ebeb). The problems scrolling pages with background-attachment:fixed backgrounds not propagated to the viewport (IE7 Zen Garden, majornelson.com) are fixed, and those sites should also scroll faster now. Repainting on zooming out should also be fixed.

I know of one visual regression on Mac (titlebar doesn't paint unified toolbar correctly), and maybe we have one on Windows (comment #204) but Bas couldn't reproduce it. There is definitely some performance and quality tweaking to do after this lands, too. But if the tryserver build looks good I'll try to land this tonight.
Attachment #457481 - Flags: review?(tnikkel) → review+
Attachment #457483 - Flags: review?(tnikkel) → review+
With c11c3f73a0a2 tryserver linux32 build, now
http://majornelson.com/default.aspx
and
http://samples.msdn.microsoft.com/workshop/samples/author/css/cssZenGarden.htm
both scroll quickly, and no the fixed backgrounds no longer fracture during the scrolling.

(Ubuntu Lucid Lynx on 7 y/o PC.)
(In reply to comment #219)
> With c11c3f73a0a2 tryserver linux32 build, now
> http://majornelson.com/default.aspx
> and
> http://samples.msdn.microsoft.com/workshop/samples/author/css/cssZenGarden.htm
> both scroll quickly, and now the fixed backgrounds no longer fracture during the
> scrolling.
> 
> (Ubuntu Lucid Lynx on 7 y/o PC.)

Same with f40a0d62b601 tryserver win32 build (on same PC)
Confirming that background painting works fine now and performance is also much better. Build looks good enough for landing on m-c, I'll file the D2D bug later when this lands and mark as blocking this bug.
(In reply to comment #204)
> Created attachment 457241 [details]
> Painting problem with D2D enabled
> 
> This happens on latest tryserver build, in any web page, as long as you have
> D2D/DW enabled and scroll up and down quickly.

I can confirm the d2d/dw issue on the tryserver build from July 15. It looks like it only happens when scrolling up and down quickly and there are links hovered during scrolling.
(In reply to comment #222)

It's fairly straightforward to reproduce by scrolling both horizontally and vertically at the same time. Shrink this window and middle-click scroll slowly in both ways to verify this immediately. It looks like only one of the bars is applying its offset to the content properly.
Depends on: 579215
Checked in, seems to have stuck!

http://hg.mozilla.org/mozilla-central/rev/e4381b348943
http://hg.mozilla.org/mozilla-central/rev/469c18b0c36d
http://hg.mozilla.org/mozilla-central/rev/85bbe15e63bd
http://hg.mozilla.org/mozilla-central/rev/ab624c68983e
http://hg.mozilla.org/mozilla-central/rev/e4213cc1138f
http://hg.mozilla.org/mozilla-central/rev/213c2dc6c88a
http://hg.mozilla.org/mozilla-central/rev/e284964e5bcf
http://hg.mozilla.org/mozilla-central/rev/e7aa691d74e4
http://hg.mozilla.org/mozilla-central/rev/c6ecff6b8a91
http://hg.mozilla.org/mozilla-central/rev/209d286abd0c
http://hg.mozilla.org/mozilla-central/rev/d98f8a21727e
http://hg.mozilla.org/mozilla-central/rev/177b05c97d84
http://hg.mozilla.org/mozilla-central/rev/a78221e8bde4
http://hg.mozilla.org/mozilla-central/rev/a3bc79f9b141
http://hg.mozilla.org/mozilla-central/rev/856678526417
http://hg.mozilla.org/mozilla-central/rev/88a10532e555
http://hg.mozilla.org/mozilla-central/rev/0c696d2a81a2
http://hg.mozilla.org/mozilla-central/rev/0615ff4efbdc
http://hg.mozilla.org/mozilla-central/rev/1fa38a238c90
http://hg.mozilla.org/mozilla-central/rev/9cc11c490ec2
http://hg.mozilla.org/mozilla-central/rev/e724a4858e54
http://hg.mozilla.org/mozilla-central/rev/047895d6fda3
http://hg.mozilla.org/mozilla-central/rev/d290d2b97416
http://hg.mozilla.org/mozilla-central/rev/83728aa66312
http://hg.mozilla.org/mozilla-central/rev/fb34f3f684df
http://hg.mozilla.org/mozilla-central/rev/41143b03b5a1
http://hg.mozilla.org/mozilla-central/rev/df85fba385ca
http://hg.mozilla.org/mozilla-central/rev/67284975e710
http://hg.mozilla.org/mozilla-central/rev/d0fb245dcb34
http://hg.mozilla.org/mozilla-central/rev/fa2e4fe69310
http://hg.mozilla.org/mozilla-central/rev/7ca67c5abe05
http://hg.mozilla.org/mozilla-central/rev/1e2f6e38cd98
http://hg.mozilla.org/mozilla-central/rev/082b84bc2d3b
http://hg.mozilla.org/mozilla-central/rev/626c70cb1db5
http://hg.mozilla.org/mozilla-central/rev/813dd6f10747
http://hg.mozilla.org/mozilla-central/rev/8183174f5527
http://hg.mozilla.org/mozilla-central/rev/284cab759b39
http://hg.mozilla.org/mozilla-central/rev/3d058bbc0838
http://hg.mozilla.org/mozilla-central/rev/26c583cbcf53
http://hg.mozilla.org/mozilla-central/rev/aceb5233289a
http://hg.mozilla.org/mozilla-central/rev/5b371eb4fa53
http://hg.mozilla.org/mozilla-central/rev/e1d7fd5255fd
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 579262
Blocks: 579277
Depends on: 579276
Depends on: 579323
Depends on: 579358
Comment on attachment 454389 [details] [diff] [review]
Part 27 v3

> NS_IMETHODIMP
> PresShell::Paint(nsIView*        aDisplayRoot,
>                  nsIView*        aViewToPaint,
>                  nsIWidget*      aWidgetToPaint,
>                  const nsRegion& aDirtyRegion,
>-                 PRBool          aPaintDefaultBackground)
>+                 PRBool          aPaintDefaultBackground,
>+                 PRBool          aWillSendDidPaint)

Is there a reason we need the aWillSendDidPaint argument to the view observer interface? It doesn't seem to be used in PresShell::Paint.
Depends on: 578616
Maybe we don't need it, but it could be useful later and churning the interface is undesirable.
No longer depends on: 578616
No longer blocks: 579260
Depends on: 579260
Scrolling on http://thebubble.msn.com/ is quite slow and browser responsiveness is sluggish for me as compared to Firefox 3.6.7

I'm using 20100716 Minefield
(In reply to comment #227)
> Scrolling on http://thebubble.msn.com/ is quite slow and browser responsiveness
> is sluggish for me as compared to Firefox 3.6.7
> 
> I'm using 20100716 Minefield

Its not bad once flash loads there in a normal window, but when I use maximized mode (with almost the exact same size plugin dimensions) it almost looks like bug 558339 [w D2D off].
Not sure if this has anything to do with this bug, but in the 7-16-2010 nightly, when opening a PDF file in a new browser window via a page link (for instance, viewing one's Best Buy credit card statement, which I'm sure some can relate to) and then trying to maximize the window, the window maximizes, but the contents do not (contents of window do not regen for the new window size).  Scrolling resizes the document's text (as if it *thinks* it is maximized), but not the document display dimensions.
(In reply to comment #229)
> Not sure if this has anything to do with this bug, but in the 7-16-2010
> nightly, when opening a PDF file in a new browser window via a page link (for
> instance, viewing one's Best Buy credit card statement, which I'm sure some can
> relate to) and then trying to maximize the window, the window maximizes, but
> the contents do not (contents of window do not regen for the new window size). 
> Scrolling resizes the document's text (as if it *thinks* it is maximized), but
> not the document display dimensions.

Probably bug 579262, resizing plugin area doesn't trigger a complete re-draw for some reason.
Depends on: 579621
While I was marking some dependent bugs as fixed I noticed that this testcase: https://bugzilla.mozilla.org/attachment.cgi?id=308629 (which is from 124150) is still scrolling quite slowly and it doesn't seem very fast.
(In reply to comment #231)
> While I was marking some dependent bugs as fixed I noticed that this testcase:
> https://bugzilla.mozilla.org/attachment.cgi?id=308629 (which is from 124150) is
> still scrolling quite slowly and it doesn't seem very fast.

Scrolls fine for me on Win7.
Instead of posting comments here, it might be better to file new bugs for these issues that you are seeing and make them blocking this bug.
Depends on: 579597
Depends on: 579663
Depends on: 579767
(In reply to comment #231)
> While I was marking some dependent bugs as fixed I noticed that this testcase:
> https://bugzilla.mozilla.org/attachment.cgi?id=308629 (which is from 124150) is
> still scrolling quite slowly and it doesn't seem very fast.

Yes. How about filing a new tracker bug for "slow scrolling with some fixed background pages" and making 124150 block it. Thanks!
(In reply to comment #234)
> Yes. How about filing a new tracker bug for "slow scrolling with some fixed
> background pages" and making 124150 block it. Thanks!

Done. Filled bug 579794.
Depends on: 579808
(In reply to comment #228)
> (In reply to comment #227)
> > Scrolling on http://thebubble.msn.com/ is quite slow and browser responsiveness
> > is sluggish for me as compared to Firefox 3.6.7
> > 
> > I'm using 20100716 Minefield
> 
> Its not bad once flash loads there in a normal window, but when I use maximized
> mode (with almost the exact same size plugin dimensions) it almost looks like
> bug 558339 [w D2D off].

I spun this off to bug 579597.
Depends on: 579924
Depends on: 580043
Depends on: 579676
Depends on: 580383
Depends on: 580473
Depends on: 580494
Depends on: 580497
When I scroll the page fast upward it looks like this: The top part of the page is duplicated acroos the viewport. I can't PrtSC it, since that will "fix" the problem, so the photo of the screen is from my n900. It's also "fixed" as soon as I do anything on the page.

When scrolling textareas similar problems occur, i.e. all one sees is the same line of text duplicated. I can not take a photo of that though, because a few milliseconds after I stop scrolling the text is OK again.

This is on Mozilla/5.0 (X11; Linux i686; rv:2.0b2pre) Gecko/20100720 Minefield/4.0b2pre running Fedora 12 on a Thinkpad z61p

Should I file a bug?
(In reply to comment #237)
> Created attachment 459000 [details]
> Scrolling is weird on quirksmode.com
> 
> When I scroll the page fast upward it looks like this: The top part of the page
> is duplicated acroos the viewport. I can't PrtSC it, since that will "fix" the
> problem, so the photo of the screen is from my n900. It's also "fixed" as soon
> as I do anything on the page.
> 
> When scrolling textareas similar problems occur, i.e. all one sees is the same
> line of text duplicated. I can not take a photo of that though, because a few
> milliseconds after I stop scrolling the text is OK again.
> 
> This is on Mozilla/5.0 (X11; Linux i686; rv:2.0b2pre) Gecko/20100720
> Minefield/4.0b2pre running Fedora 12 on a Thinkpad z61p
> 
> Should I file a bug?

That's probably bug 579736.
OK. But please not that my problem is totally unrelated to Flash and bug 579736 seems to be a Flash bug.
(In reply to comment #237)
> Created attachment 459000 [details]
> Scrolling is weird on quirksmode.com
> 
> When I scroll the page fast upward it looks like this: The top part of the page
> is duplicated acroos the viewport. I can't PrtSC it, since that will "fix" the
> problem, so the photo of the screen is from my n900. It's also "fixed" as soon
> as I do anything on the page.
> 
> When scrolling textareas similar problems occur, i.e. all one sees is the same
> line of text duplicated. I can not take a photo of that though, because a few
> milliseconds after I stop scrolling the text is OK again.
> 
> This is on Mozilla/5.0 (X11; Linux i686; rv:2.0b2pre) Gecko/20100720
> Minefield/4.0b2pre running Fedora 12 on a Thinkpad z61p
> 
> Should I file a bug?

I confirm.  I am seeing the same phenomenon on my Linux installation.

Incidentally, it seems that with older hardware such has my Geforce 3 card, the effect is even more obvious and on some sites the browser appears to almost lock up.

this needs some attention as there are plenty of folks using older hardware.
Ok, if you guys are seeing bug 579736, please comment there, else please file another bug if your not sure, or bug 579736 doesn't fix your issue.
Depends on: 580734
Depends on: 580621
Depends on: 581725
Depends on: 582223
Depends on: 582476
Depends on: 583115
Depends on: 583207
Depends on: 583548
Depends on: 583878
Depends on: 583859
Depends on: 580757
Depends on: 584617
Depends on: 585258
Depends on: 586162
Depends on: 586627
Depends on: 586683
Depends on: 587483
Depends on: 587442
Depends on: 589305
Depends on: 590568
Depends on: 588591
Depends on: 591209
Depends on: 593627
Depends on: 594333
Depends on: 595671
Depends on: 583475
Depends on: 607213
Depends on: 618722
Depends on: 621495
No longer depends on: 588591
Depends on: 588591
No longer depends on: 579767
Depends on: 613931
Depends on: 650228
Depends on: 656948
Depends on: 686673
No longer depends on: 686673
Depends on: 686608
Depends on: 708155
Depends on: 715368
Depends on: 886875
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: