Closed Bug 539356 (dlbi) Opened 14 years ago Closed 12 years ago

Replace Invalidate() calls in reflow with display list analysis

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox18 - ---

People

(Reporter: ventnor.bugzilla, Assigned: mattwoodrow)

References

(Depends on 5 open bugs, Blocks 9 open bugs)

Details

(Keywords: perf, Whiteboard: [Snappy:P2])

Attachments

(53 files, 58 obsolete files)

1.56 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.87 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
12.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.04 KB, patch
roc
: review+
Details | Diff | Splinter Review
33.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
46.60 KB, patch
roc
: review+
Details | Diff | Splinter Review
193.76 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.32 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
6.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.53 KB, patch
roc
: review-
Details | Diff | Splinter Review
5.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
32.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
21.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
47.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.55 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
69.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.44 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.56 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.21 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.20 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
17.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
110.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.28 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
12.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
47.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.10 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
12.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
roc thinks that this will eventually be a DHTML win and assigned me to this. Whenever the geometry of a frame differs during reflow, currently we call Invalidate() on the entirety of the frame.

The aim of this bug is to remove all those calls and replace it with a comparison of display list items before/after each reflow. We can then attempt to match up the items and possibly minimise the area we need to invalidate.

By implementing a certain virtual function for each display list item type, we can calculate the minimal invalidation area case-by-case rather than doing the whole overflow rect.
Attached patch Patch 1 - Infrastructure (obsolete) — Splinter Review
This provides the comparison algorithm and the ability to match up display list items. This patch alone doesn't really cause a behavioural change overall, unlike the next patch.
Attachment #421369 - Flags: review?(roc)
Look at all those -'s!
+      for (PRInt32 n = 0; n < rem; lineFrame = lineFrame->GetNextSibling(), ++n) {
+        lineArea.UnionRect(lineArea, lineFrame->GetOverflowRect());
+      }

This is incorrect, lineFrame->GetOverflowRect() is relative to lineFrame->GetPosition().

+      line->SetCombinedArea(newFrame->GetOverflowRect());

Similar problem here.

The optimizations in nsIFrame::CheckInvalidateSizeChange will need to be reimplemented. Each of the cases in that function are trying to repaint (or not repaint) in some set of situations, and we need to ensure that we repaint (or don't repaint) in those situations.

nsDisplayText::ComputeInvalidationArea should be in the first patch.

       // Resize/move the row to its final size and position
       if (movedFrame) {
         rowFrame->InvalidateOverflowRect();
       }

You missed it!

I think we should make Invalidates assert if called during reflow (use the phase stuff in nsPresContext).
Can you pull out the code that adds types to all the display items into a separate patch?

+  if (GetStyleText()->mTextShadow) {
+    nsresult rv = aBelowTextDecorations->AppendNewToTop(new (aBuilder)
+      nsDisplayTextShadow(this, decorations, aLine));
+    NS_ENSURE_SUCCESS(rv, rv);

Put the code that puts all shadows into one display item into its own patch.

-  nsDisplayWrapList* GetStoredList() { return &mStoredList; }
-#endif
-
+  virtual nsDisplayList* GetList() { return mStoredList.GetList(); }

We don't want to do this for nsDisplayTransform, because GetList should only return a list when the items in the list are in the same coordinate system as the other items. One option here is to make GetStoredList virtual and have all the nsDisplayListWrappers implement it ... so the items returned by GetStoredList are not guaranteed to be in the same coordinate system as other items.
+      // what type it is, and its heirarchy in the display list.

hierarchy

+nsDisplayInvalidation::OnFrameDestroy(nsIFrame* aFrame)
+{
+  nsDisplayTableLinkEntry* entry = mBefore.GetEntry(aFrame);
+  if (!entry)
+    return;
+
+  for (PRUint32 i = 0; i < entry->Length(); i++) {
+    if (GetFrameForItem(entry->Item(i).Get()) == aFrame)

How could this "if" fail?

+  while (aEntry->Length() != 0) {
+    // If the frame is gone (and the frame pointer is rubbish), nothing to invalidate
+    if (aEntry->Item(0).IsFrameDestroyed()) {
+      aEntry->Items().RemoveElementAt(0);
+      continue;
+    }

Seems like it would be simpler and more efficient to just iterate through the array.

Don't we need to invalidate for destroyed frames?

Some name changes are required. IsEquivalent probably isn't a good name.

+    nsTArray<nsDisplayTableLink*> lhsEquivalentItems;
+    nsTArray<nsDisplayTableLink*> rhsEquivalentItems;

I don't think these are needed. Just iterate over the lists in the hashtable instead of building these temporary lists.
+    for (PRUint32 i = 0; i < lhsMatchedItems.Length(); i++) {
+      aEntry->Items().RemoveElement(*lhsMatchedItems[i]);
+    }

I don't think we'll need to do this, if we iterate over the lists in the hashtable directly.
+nsRegion
+nsDisplayText::ComputeInvalidationArea(nsDisplayListBuilder* aBuilder,
+                                       nsDisplayItem* aOther,
+                                       const nsRect& aOtherVisibleRect,
+                                       const nsRect& aOtherBounds) {
+  // XXX We need to unconditionally invalidate, because it's possible for
+  // two different strings to have the same geometric width.
+  // Eventually, we can change this to analyse the string itself and get
+  // more performance wins.
+  // We also use the same antialiasing hack as in Paint()

We really need to do something here. Right now it looks like when any reflow happens, all visible text in the window will have to be repainted. We probably should be invalidating in nsTextFrameThebes::CharacterDataChanged.

The optimizations from CheckInvalidateSizeChange will need to be implemented in nsDisplayBackground::ComputeInvalidationArea and nsDisplayBorder::ComputeInvalidationArea.

+    nsDisplayItem::Type mParentType;
+    nsIFrame* mParentFrame;

I'm not sure why we need to store and check these.

Overall, this looks very promising!

Some suggested renames:
nsDisplayInvalidation -> nsDisplayListInvalidator
nsDisplayTableLink -> move it into nsDisplayListInvalidator, call it Item?
nsDisplayTableLinkEntry -> move it into nsDisplayListInvalidator, call it ItemEntry?
GetLastGoodUnderlyingFrame -> GetNearestUnderlyingFrame?
IsEquivalent -> MatchesFrameAndType

Let's have some design documentation in nsDisplayListInvalidator.h

+    friend PLDHashOperator CompareItemsCallback(nsDisplayTableLinkEntry *aEntry, void *aArg);
+    friend PLDHashOperator InvalidateLeftoversCallback(nsDisplayTableLinkEntry *aEntry, void *aArg);

You can avoid 'friend' by making these static functions in the class.
When shell->IsPaintingSuppressed is true, we shouldn't do any of the work to create display lists or compare them.
You need to check all the different display types and figure out what parts of the frame geometry they depend on. For example, nsDisplayBackground background positions can depend on the content, border or padding box, and background clipping can depend on the content, border or padding box as well (I think), so if any of those boxes changes, we need to repaint. Right now you only repaint if the border-box changes. The border-box might stay the same while the content-box changes (e.g. if CSS 'width' increases and 'padding' decreases). You should write an invalidation reftest for this.
For the layout/tables changes, a number of the callers of nsTableFrame::InvalidateFrame were saving the original rect and overflow rect just so they could make those calls.  So there's more code to be removed there, possibly (in a followup if desired).
A pretty large problem with this approach is that we trigger a lot of assertions about calling GetUsedBorder/Padding on a dirty frame. This is a real issue; we can't accurately determine the "before reflow" dislay list state if style changes have already destroyed our ability to know what the pre-reflow content-box and padding-box were.

Would it be possible to get rid of these assertions once and for all, by having style changes to margin, border and padding widths store the old used-margin/used-border/used-padding in frame properties, which are then removed (if possible) when reflow happens? There's obviously a performance hit for dynamic margin/border/padding changes --- but how common are they? In exchange, we'd get guilt-free painting of dirty frame trees, plus it would make this invalidation approach work.

If we don't want to do that, we could still fix this invalidation approach by ensuring changes to border/padding widths cause repainting, and having pre-reflow-display-list construction suppress the assertions.
> but how common are they

Not very.
(In reply to comment #7)
> nsDisplayTableLink -> move it into nsDisplayListInvalidator, call it Item?

I can't use friend nsDisplayListInvalidator::Item on nsDisplayItem, which I need to access the visible rect.
D'oh, wait, it'd probably be part of the friend nsDisplayListInvalidator line, wouldn't it?
Attached patch Remove GetUsedX Assertion (obsolete) — Splinter Review
One of the requirements for this project to work is to make sure we handle post-stylechange-pre-reflow properly.

I think I got it right...
Attachment #421369 - Attachment is obsolete: true
Attachment #421370 - Attachment is obsolete: true
Attachment #423658 - Flags: review?(roc)
Attachment #421369 - Flags: review?(roc)
Comment on attachment 423658 [details] [diff] [review]
Remove GetUsedX Assertion

Roc wants this in a new bug.
Attachment #423658 - Attachment is obsolete: true
Attachment #423658 - Flags: review?(roc)
Depends on: 542361
Attached patch Patch 1 - Infrastructure (obsolete) — Splinter Review
This is the main patch, the rest is just code churning which is likely to get r+ anyway. My plan is to upload them once this gets r+ so if there are review comments I can fix them easily.
Attachment #423868 - Flags: review?(roc)
+  if (!invalFrame)
+    invalFrame = aLink.GetNearestUnderlyingFrame();

{} around the if body. There are lots of these.

+            mFrameData.mBounds = aItem->GetBounds(aBuilder);
+            if (itemFrame) {
+              mInvalidArea = GetReferenceRectForRect(aItem->mVisibleRect,
+                                                     itemFrame,
+                                                     aListFrameTree);
+              mFrameData.mPaddingRect = itemFrame->GetPaddingRect() -
+                                itemFrame->GetPosition() +
+                                aBuilder->ToReferenceFrame(itemFrame);

I think we should have a virtual method on nsDisplayItem, say nsDisplayItem::SaveGeometry(nsDisplayItemGeometry* aGeometry), which stashes the bounds in aGeometry, and for display items which are going to need more than that (e.g. borders and backgrounds), is overridden to also store the padding or content rect. This also means we get a slight optimization where we don't have to save the padding rect for most display items.

+  nsRect mPaddingRect;

So I'd make this something like "mExtraRect". This would be interpreted however the item wishes. For backgrounds this would be the background positioning area, see http://www.w3.org/TR/css3-background/#background-positioning-area

+    static void InvalidateRegion(nsDisplayListBuilder* aBuilder,
+                                 const Item& aLHS,
+                                 const Item& aRHS);
+
+    void InvalidateWholeItem(const Item& aLink);
+    nsresult FillHashtable(nsTHashtable<ItemEntry>* aTable,
+                           nsDisplayList* aList,
+                           PRBool aIsFirstPass,
+                           nsTArray<nsIFrame*>& aListFrameTree);
+

These methods need comments somewhere.
Attached patch Patch 1 - Infrastructure (obsolete) — Splinter Review
Attachment #423868 - Attachment is obsolete: true
Attachment #423938 - Flags: review?(roc)
Attachment #423868 - Flags: review?(roc)
+            mFrameData.mBounds = aItem->GetBounds(aBuilder);

You don't need this anymore.

Some renaming:
GetReferenceRectForRect => GetReferenceRectToInvalidate
GetInvalidationFrame => GetFrameToInvalidate
SetPrevious => SetPreviousDisplayList
ItemEntry => ItemList

I think there is a problem when we destroy a frame that is the mNearestUnderlyingFrame for some item. We're going to crash. Please make up a testcase for this, it should be something like
<span style="opacity:0.5">Hello Kitty</span>
with a dynamic change where we start with a linebreak between the two words, and then a reflow puts the text on one line.

+        nsDisplayListInvalidator::Item& lhsItem = aEntry->GetItem(i);

This can be outside the j loop.

+          nsDisplayListInvalidator::InvalidateRegion(inval->mBuilder,
+                                                  lhsItem, rhsItem);

Fix indent.

+          j--;

Not needed, since you break out of the loop immediately anyway.

+    inval->mAfter.RawRemoveEntry(rhsEntry);
+  }
+
+  return PL_DHASH_REMOVE;

I don't think we need to bother removing the entries. We're going to kill off the entire hashtable(s) anyway once CompareAndInvalidate is done.

+    // If this is a text frame and the frame is dirty, we need to invalidate it now

Explain why.

I think the Item constructor should be moved out of the header file, taking GetFrameForItem and GetReferenceRectForRect. Also, why mark GetFrameForItem as inline?

+  nsTArray<nsIFrame*> listFrameTree;

might as well make this nsAutoTArray<nsIFrame*,8>. It won't usually be very deep.

+        nsDisplayItem* mItem;
+        nsIFrame* mNearestUnderlyingFrame;
+        nsRect mInvalidArea;
+        nsDisplayItemGeometry mFrameData;

These need comments.

I think it would make sense to move the ComputeVisibility calls out to nsPresShell, or else move more of the display list construction into nsDisplayListInvalidator (I prefer moving into nsPresShell). They belong with display list construction.

+    // Fills a hashtable with all items in aList. aListFrameTree
+    // records the ancestry of items in aList, since this is a
+    // recursive function.

Needs to describe aListFrameTree better, as described.
(In reply to comment #20)
> I think there is a problem when we destroy a frame that is the
> mNearestUnderlyingFrame for some item. We're going to crash. Please make up a
> testcase for this, it should be something like
> <span style="opacity:0.5">Hello Kitty</span>
> with a dynamic change where we start with a linebreak between the two words,
> and then a reflow puts the text on one line.

The testcase might need to be something more like
<span style="opacity:0.5">Hello <b>Kitty</b></span>

so the frame for the <b>Kitty</b> isn't destroyed, but the second <span> frame that was its parent and its mNearestUnderlyingFrame *is* destroyed.
GetInvalidationFrame can be replaced with GetFrameForItem (if we add an assertion to nsDisplayItem::GetUnderlyingFrame that only nsDisplayClip items can have null mFrames). Then we don't need mNearestUnderlyingFrame and the crash problem would go away.
One pretty big problem is going to be table painting and nsDisplayTableBorderBackground, because that display item paints all the backgrounds for all the table parts (via nsTablePainter), and the table border, and all cell borders too if border-collapsing is in effect. So it depends on the geometry of everything in the table. To get reasonable invalidation here we probably need to rework table background painting so that individual nsDisplayBackground items are created (which would simplify other code), and possibly also do something with collapsed borders, although that may be less important.
Attached patch Patch 1 - Infrastructure (obsolete) — Splinter Review
Uploading what I've done so far.
Attachment #423938 - Attachment is obsolete: true
Attachment #423938 - Flags: review?(roc)
Blocks: 559396
(In reply to comment #24)
> Created an attachment (id=424689) [details]
> Patch 1 - Infrastructure
> 
> Uploading what I've done so far.

Oh damn, this patch doesn't add types for each display list item type. I don't have the code with me right now so you may need to scavenge some of it from the obsolete patch :(
We might want to finish this to fix issues in bug 579323.

We can address comment #23 by setting a flag on a table when any of its components change geometry during reflow, and invalidating the entire table in that case.

Things have changed a bit since these patches were done. FrameLayerBuilder now builds a display list for the entire window every time we paint. Possibly we could harvest geometry information at that time and use that as our "before-reflow state", just rebuilding a display list after reflow and compare the geometry. That would save one display list construction per reflow.

FrameLayerBuilder is also storing a per-frame list of (display item key, layer) pairs representing the visible content of the window (except for content in inactive layers, but that could be fixed), as frame properties, plus a set of pointers to the frames that have such properties. This could be used as the basis for the saved display list state.
blocking2.0: --- → ?
I have some new patches, but it's not done yet and I think it's going to be too risky for FF4. Post FF4 we can look at moving *all* invalidation to be display-list based.

My idea is that when invalidation is needed we simply ensure a paint event is queued in the refresh driver. When the refresh driver decides to paint, we build a new display list, update the layer tree to that display list, and during layer tree reconstruction, collect an invalid region based on the display list differences, and also update the layer tree contents by repainting ThebesLayer items as usual. Then and only then, force a synchronous paint of the native widget (unless we're a content process whose layer tree is being composited elsewhere). When painting a native widget, we'd just recomposite the layer tree and not do any flushing or Thebes drawing.

Doing invalidation during layer tree reconstruction has several advantages. It means we can always invalidate the right ThebesLayers. It will centralize the code to track display items and their changes. And it minimizes the amount of display list construction; we'll construct just one display list per paint (and the paint rate will be throttled of course).
Is there anything we can do about the new-tab animation in the FF4 timeframe? It's looking pretty ugly right now, and sometimes even leaves turds.
Blocks: 595574
Blocks: 554004
Blocks: 318474
Blocks: 608648
Blocks: 119311
Blocks: 612250
Blocks: 626927
Blocks: 585258
Blocks: 352367
Blocks: 454254
Blocks: 693105
Blocks: 707960
Adding [Snappy] search string. DisplayList Analysis aims to fix a number of cases that lead to excessive/redundant invalidation, which should improve perceived performance.

BTW, do we need to reassign this bug?
Whiteboard: [Snappy]
[snappy] != perf
This is a perf bug.  So far as I know, this is not a user-response-time bug.
Whiteboard: [Snappy] → [Snappy:P2]
(In reply to Randell Jesup [:jesup] from comment #34)
> [snappy] != perf
> This is a perf bug.  So far as I know, this is not a user-response-time bug.

Apparently this will affect how quickly the UI appears to respond for things like opening/closing/switching/moving tabs, since we currently redraw the whole tab-bar for those.
Roc says Matt Woodrow will be taking this bug w/ a new approach in January.
Blocks: 712883
Blocks: 546362
Blocks: 723315
I've started working on this now.

I've got a prototype working locally, fixing performance/correctness bugs at the moment. I'll try to have something demoable within the next week or so.
Assignee: ventnor.bugzilla → matt.woodrow
Depends on: 725886
I tried to write a test for bug 725580 but was unsuccessful.  There's a manual testcase that shows the problem quite clearly with paint flashing enabled, which can be checked here.
Depends on: 725580
Making good progress on this, passes almost all mochitests.

A couple of issues that I'm struggling with:

MozAfterPaint - I've got this working to generate an invalidation region for the window, but I'm not sure how this is going to work for subdocuments, IFrames etc. Invalidation is all relative to a ThebesLayer, so finding the changed area for a component of this isn't easy. Relatedly, INVALIDATE_CROSS_DOC no longer exists, so I'm not sure how to only show rects that content should be allowed to see.

Flush_Layout (from getClientBoundingRect) - Reftests are using this to determine if anything needs to be repainted. Since we never do any invalidation detection until we paint (refresh driver tick), this doesn't actually invalidate anything. Reftests thus think that nothing will be invalidated.

Ideas anyone?
Thinking about the second one, we could have invalidated frames (caused by style changes etc) setup a pending paint event - either with the entire window as the dirty region, or somehow fill this in later after a refresh driver tick.

I think we need this to implement empty transaction support, and it should work for style changes. Will reftests ever attempt to 'flush layout' with a change that would only be caught by display-list comparison?
Alternatively we could teach the reftest harness to wait until a refresh driver tick before checking for paint events (do we have events that would work for this?) and hope there aren't other things relying on the existing behaviour.
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> MozAfterPaint - I've got this working to generate an invalidation region for
> the window, but I'm not sure how this is going to work for subdocuments,
> IFrames etc. Invalidation is all relative to a ThebesLayer, so finding the
> changed area for a component of this isn't easy. Relatedly,
> INVALIDATE_CROSS_DOC no longer exists, so I'm not sure how to only show
> rects that content should be allowed to see.

How about we change MozAfterPaint so that toplevel chrome documents see everything, toplevel content documents see repainting inside them (possible because all their content is always in an nsDisplayOwnLayer), and no other documents see anything.

We could attach identifying information to userdata for the container layer generated by an nsDisplayOwnLayer in nsSubDocumentFrame to indicate which prescontext it belongs to. Then you can walk up the layer tree and find which prescontext(s) need to be notified about the repainting.

This would be mostly compatible with our tests and other uses of MozAfterPaint, and hopefully be reasonably easy and efficient to implement. There might be one or two tests that use MozAfterPaint in an <iframe> that need to have some tests removed.
(In reply to Matt Woodrow (:mattwoodrow) from comment #40)
> Thinking about the second one, we could have invalidated frames (caused by
> style changes etc) setup a pending paint event - either with the entire
> window as the dirty region, or somehow fill this in later after a refresh
> driver tick.
> 
> I think we need this to implement empty transaction support, and it should
> work for style changes. Will reftests ever attempt to 'flush layout' with a
> change that would only be caught by display-list comparison?

I'm not sure what this question is really about. Are you asking how to implement isMozAfterPaintPending?

One option for that would be to return true whenever there's a refresh driver tick pending. Then we'd need to require that every refresh driver tick fires a MozAfterPaint event (if there's a listener).

(BTW feel free to remove clearMozAfterPaintEvents if necessary, it's only used by one test.)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #42) 
> How about we change MozAfterPaint so that toplevel chrome documents see
> everything, toplevel content documents see repainting inside them (possible
> because all their content is always in an nsDisplayOwnLayer), and no other
> documents see anything.
> 
> We could attach identifying information to userdata for the container layer
> generated by an nsDisplayOwnLayer in nsSubDocumentFrame to indicate which
> prescontext it belongs to. Then you can walk up the layer tree and find
> which prescontext(s) need to be notified about the repainting.
> 
> This would be mostly compatible with our tests and other uses of
> MozAfterPaint, and hopefully be reasonably easy and efficient to implement.
> There might be one or two tests that use MozAfterPaint in an <iframe> that
> need to have some tests removed.

This has the downside that toplevel content documents can see invalidations from subdocuments (iframes, svg), which they couldn't do previously without privileges.

I can't see any easy way to avoid this without forcing all documents into their own layers, and even then it won't be easy passing the regions back up through SVG's painting code.
+cc: jwatt. He is moving the SVG scene graph to use the displayList as the render and hit-test tree.
Mochitests also run within an iframe, so we don't get any events to them. I've got most of them passing when run standalone though.

Is there a way to run the mochitest in it's own tab/window?
(In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> it won't be easy passing the regions back up through SVG's painting code.

There's a helper nsSVGUtils::TransformFrameRectToOuterSVG that should help with that.
Blocks: 270264
Depends on: 731858
Attachment #424689 - Attachment is obsolete: true
Attachment #424690 - Attachment is obsolete: true
Attachment #424691 - Attachment is obsolete: true
Attachment #424692 - Attachment is obsolete: true
(In reply to Matt Woodrow (:mattwoodrow) from comment #37)
> I've started working on this now.
> 
> I've got a prototype working locally, fixing performance/correctness bugs at
> the moment. I'll try to have something demoable within the next week or so.

Hey Matt - is there a demo up of this somewhere, it would be nice to share with Facebook so they can verify the problems reported to dev-platform are indeed solved by this.
Attachment #605172 - Flags: review?(roc)
Not sure who to get to review most of these as roc is away. I've selected him for most of them anyway, happy for someone else to take them if they're keen :)

More patches coming soon!
Attachment #605179 - Flags: review?(bzbarsky)
Comment on attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames

Why do you need this? You probably want to use views here.
(In reply to Timothy Nikkel (:tn) from comment #57)
> Comment on attachment 605174 [details] [diff] [review]
> Part 3 - Make GetParentPresContext() succeed when the current PresContext
> has no frames
> 
> Why do you need this? You probably want to use views here.

Finding the root (painting) refresh driver from any arbitrary frame. What is the advantage of using views here? Happy to change it if you want.
Part 9: All of The Things.

I've split this up into a few patches for review, they won't compile or run on their own.
Attachment #605261 - Flags: review?(roc)
Attachment #605262 - Flags: review?(roc)
Attachment #605264 - Flags: review?(bzbarsky)
Attachment #605265 - Flags: review?(jwatt)
Comment on attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +238,5 @@
>  
>        PRIntervalTime iStart = PR_IntervalNow();
>  
>        for (PRUint32 i = 0; i < aCount; i++)
> +        rootFrame->InvalidateFrame();

You can remove "r".

@@ +351,5 @@
>          rootScrollFrame ? rootScrollFrame->GetContent() : nsnull;
>        nsRect rootDisplayport;
>        bool usingDisplayport = rootContent &&
>          nsLayoutUtils::GetDisplayPort(rootContent, &rootDisplayport);
> +      rootFrame->InvalidateFrame();

You can remove rootDisplayPort and usingDisplayPort and rootContent.

::: layout/base/nsCaret.cpp
@@ +644,5 @@
>        if (block) {
>          const nsStyleTextReset* style = block->GetStyleTextReset();
>          if (style->mTextOverflow.mLeft.mType != NS_STYLE_TEXT_OVERFLOW_CLIP ||
>              style->mTextOverflow.mRight.mType != NS_STYLE_TEXT_OVERFLOW_CLIP) {
> +          block->InvalidateFrame();

Do we need nsCaret::InvalidateTextOverflowBlock or can it just go away? I would have expected it to just go away.

@@ +1163,5 @@
>                                nsIFrame *aFrame)
>  {
>    NS_ASSERTION(aFrame, "Must have a frame to invalidate");
>    nsRect rect;
>    rect.UnionRect(aRect, aHook);

Get rid of rect, aRect and aHook parameters.

::: layout/base/nsImageLoader.cpp
@@ +278,5 @@
>  
>  #endif
>  
>    if (mFrame->GetStyleVisibility()->IsVisible()) {
> +    mFrame->InvalidateFrame();

Get rid of all the code that computes "bounds".

Stopping here ... I think generally there's a lot more code that can be removed here!
(In reply to Matt Woodrow (:mattwoodrow) from comment #63)
> Created attachment 605266 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation

Just a bit of context, since this is probably the most complex patch in the series. This is fundamentally two changes (that could probably be separated, but it's a non-trivial amount of work).

1) Make inactive layers retain their LayerManagers and Layers (but not buffers).

2) Compute invalid regions of ThebesLayers using display list comparison.


Will work on removing more of the old invalidation code.
Comment on attachment 605171 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects

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

Can you just modify LayerUserDataSet to use gfx/2d/UserData? The gfx/2d code doesn't follow our naming conventions so modifying LayerUserDataSet would isolate that.
Comment on attachment 605172 [details] [diff] [review]
Part 2 - Add new API to BasicLayers

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

Looks OK, but I don't understand why you're removing #ifdef DEBUG around mPhase. And you need to document the new IsWidgetLayerManager method.
Comment on attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed

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

I not confident we can get away with this :-(.

I've forgotten exactly what we discussed about this. What about adding support for component-alpha layers to BasicLayers?
 
> I not confident we can get away with this :-(.
> 
> I've forgotten exactly what we discussed about this. What about adding
> support for component-alpha layers to BasicLayers?

Really? :(

That's a pain. I guess I can look into implementing this, not sure how to do the alpha recovery part efficiently though.

 
> Looks OK, but I don't understand why you're removing #ifdef DEBUG around
> mPhase. And you need to document the new IsWidgetLayerManager method.

Making mPhase available in opt builds is required for the InTransaction() call. It's used later on in the patch queue.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #70)
> Comment on attachment 605175 [details] [diff] [review]
> Part 4 - Disable subpixel AA with BasicLayers so that empty transactions
> always succeed
> 
> Review of attachment 605175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I not confident we can get away with this :-(.

Does this do what I think it does, i.e. permanently disable subpixel AA when not using accelerated layers? That wouldn't be acceptable, imho.
Not all subpixel AA, just when we attempt to retain text in a layer with a transparent background.

Probably the worst case will be text on top of a fixed position background.

I'm looking into other solutions.
Comment on attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames

>+++ b/layout/base/nsPresContext.cpp
>+  // Not sure if this is always strictly the parent, but it works for GetRootPresContext
>+  // where the current pres context has no frames.

This seems to be correct at first glance, now that we always unify viewmanager trees when document trees are unified.  I think.

Which raises the question of why the first block (the one using GetCrossDocParentFrame) is needed at all.  I suspect it's not, but maybe double-check with roc?  Removing it in a followup is ok.

> nsPresContext::GetRootPresContext()
>-    if (!parent)
>+    if (!parent || parent == pc)

Hrm.  Why is this needed?  |parent == pc| shouldn't ever happen, as far as I can tell.
Attachment #605174 - Flags: review?(bzbarsky) → review+
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -1619,16 +1619,23 @@ BasicLayerManager::EndTransactionInterna
>+    // TODO: We should really just set mTarget to null and make sure we can handle that further down the call chain

Please file this bug?

>+    nsRefPtr<gfxContext> ctx = new gfxContext(surf);
>+    mTarget = ctx;

You don't need the |ctx| temporary, right?

Someone who really knows how this layer stuff works should probably review the remaining changes to this file and the other gfx/layers changes.

>+++ b/layout/base/nsPresShell.cpp

The changes to this file should also be reviewed by someone who knows how views and widgets fit together nowadays (so :tn or roc, probably).

>+++ b/view/public/nsIViewManager.h
>+  virtual bool ProcessPendingUpdates()=0;

Document the return value?

>+++ b/view/src/nsViewManager.cpp

I'm not quite sure why we're calling PresShell:Paint() in ProcessPendingUpdatesForView...  At the very least, document clearly!

> nsViewManager::ProcessPendingUpdates()
>+  //if (mHasPendingUpdates) {

Why the comment?  If that line is no longer needed (why?) just remove it?
Attachment #605179 - Flags: review?(tnikkel)
Attachment #605179 - Flags: review?(bzbarsky)
Attachment #605179 - Flags: review-
Comment on attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code

>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -188,17 +187,16 @@ nsContentView::Update(const ViewConfig&

The long comment explaining why the InvalidateFrame call is the way it is can go away now that the InvalidaFrame call is going away, right?

That said, why is this InvalidateFrame call going away?

>@@ -1718,17 +1716,16 @@ nsFrameLoader::SetRenderMode(PRUint32 aR

Likewise, why can this call go away?  If it can, remove the comment too.

> +++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -8053,16 +8044,17 @@ nsCSSFrameConstructor::ContentStateChang
>+    primaryFrame->SchedulePaint();

Why is that call unconditional?  That seems pretty odd.  Why do we need that it at all?

>+++ b/layout/base/nsCaret.cpp
> void nsCaret::InvalidateRects(const nsRect &aRect, const nsRect &aHook,

The rects no longer seem to be needed here.... at least take out the UnionRect call?  But it seems like if we can invalidate just the rect, we should.

>+++ b/layout/base/nsImageLoader.cpp
>-    mFrame->Invalidate(bounds);
>+    mFrame->InvalidateFrame();

Do we not want to limit the invalidate to the actual damage area?  We certainly have bugs on making that actually work here!

>+++ b/layout/base/nsPresContext.cpp
>     // FrameLayerBuilder caches invalidation-related values that depend on the
>     // appunits-per-dev-pixel ratio, so ensure that all ThebesLayer drawing
>     // is completely flushed.
>-    FrameLayerBuilder::InvalidateThebesLayersInSubtree(rootFrame);
>+    rootFrame->InvalidateFrameSubtree();

Does the comment still makes sense?

>@@ -2046,22 +2046,19 @@ nsPresContext::FireDOMPaintEvent()
>-      notifyContent = false;
>+      notifyContent = true;

The changes to this function make no sense.  What are you trying to do here?

>+++ b/layout/generic/nsBlockReflowState.cpp
>@@ -821,17 +821,17 @@ nsBlockReflowState::FlowAndPlaceFloat(ns
>-    FrameLayerBuilder::InvalidateThebesLayersInSubtree(aFloat);
>+    aFloat->InvalidateFrameSubtree();

Why do we still need to invalidate here?

>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -1789,19 +1642,19 @@ void nsGfxScrollFrameInner::ScrollVisual
>-  mOuter->InvalidateWithFlags(invalidateRect, flags);
>+  mOuter->InvalidateFrame();

Again, this is invalidating a larger area than invalidateRect.  Do we still need to compute invalidateRect, assuming invalidating the larger area is correct?

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -613,18 +613,17 @@ nsImageFrame::OnDataAvailable(imgIReques
>-  Invalidate(r);
>+  InvalidateFrame();

Similar here.

>@@ -693,17 +690,17 @@ nsImageFrame::FrameChanged(imgIRequest *
>-  Invalidate(r);
>+  InvalidateFrame();

And here.

>+++ b/layout/generic/nsImageMap.cpp
>@@ -989,17 +989,17 @@ nsImageMap::HandleEvent(nsIDOMEvent* aEv
>-            mImageFrame->Invalidate(dmgRect);
>+            mImageFrame->InvalidateFrame();

Then why do we need dmgRect?

>+++ b/layout/generic/nsTextFrameThebes.cpp
>@@ -4293,16 +4293,17 @@ nsTextFrame::CharacterDataChanged(Charac
>+    textFrame->InvalidateFrame();

Why this change?

>@@ -7732,17 +7733,18 @@ nsTextFrame::ReflowText(nsLineLayout& aL
>+  //XXX: Can we assume this is correct?
>+  //InvalidateFrame();

I suspect no, but I'd like to understand why this is commented out first....

>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -539,18 +539,16 @@ RenderFrameParent::ShadowLayersUpdated()

The comments here can all go away, right?

>+++ b/layout/svg/base/src/nsSVGForeignObjectFrame.cpp
>@@ -661,17 +643,17 @@ nsSVGForeignObjectFrame::InvalidateDirty
>-  aOuter->InvalidateWithFlags(rect, aFlags);
>+  aOuter->InvalidateFrame();

Can we stop computing rect, then?  Alternately, we should be using it here somehow...

>@@ -679,14 +661,13 @@ nsSVGForeignObjectFrame::FlushDirtyRegio
>-  InvalidateDirtyRect(outerSVGFrame, mSubDocDirtyRegion.GetBounds(),
>-                      aFlags | INVALIDATE_CROSS_DOC);
>+  InvalidateDirtyRect(outerSVGFrame, mSubDocDirtyRegion.GetBounds(), aFlags);

Why this change?

>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -540,31 +540,23 @@ nsDisplaySVG::Paint(nsDisplayListBuilder
>+    ancestor->InvalidateFrameSubtree();

Why not InvalidateFrame()?

>+++ b/layout/tables/nsTableCellFrame.cpp
>@@ -896,17 +888,16 @@ NS_METHOD nsTableCellFrame::Reflow(nsPre

Can you stop saving origRect and/or origVisualOverflow here?

>+++ b/layout/tables/nsTableCellFrame.h
>+  virtual void InvalidateFrame()
...
>+    if (FrameHasBorderOrBackground(this)) {
>+      nsTableFrame *tableFrame = nsTableFrame::GetTableFrame(this);
>+      tableFrame->InvalidateFrame();

Did the old code end up doing this too, somehow?

>+++ b/layout/tables/nsTableColFrame.cpp
>+nsTableColFrame::InvalidateFrame()

Same question here.

>+++ b/layout/tables/nsTableColGroupFrame.cpp
>+nsTableColGroupFrame::InvalidateFrame()

And here.

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -1427,17 +1424,17 @@ nsTableFrame::ProcessRowInserted(nscoord
>-          Invalidate(damageRect);
>+          InvalidateFrame();

Do you still need damageRect, then?  What about damageY?

> void nsTableFrame::PlaceChild(nsTableReflowState&  aReflowState,
>                               nsIFrame*            aKidFrame,
>                               nsHTMLReflowMetrics& aKidDesiredSize,
>                               const nsRect&        aOriginalKidRect,
>                               const nsRect&        aOriginalKidVisualOverflow)

Some of those arguments are no longer needed, right?

>+++ b/layout/tables/nsTableOuterFrame.cpp
>@@ -948,18 +948,16 @@ NS_METHOD nsTableOuterFrame::Reflow(nsPr
>   nsRect origInnerRect = InnerTableFrame()->GetRect();
>   nsRect origInnerVisualOverflow = InnerTableFrame()->GetVisualOverflowRect();

Do you still need those?

>   nsRect origCaptionRect;
>   nsRect origCaptionVisualOverflow;
>   bool captionFirstReflow;

And those?

>+++ b/layout/tables/nsTableRowFrame.cpp
>@@ -841,18 +838,16 @@ nsTableRowFrame::ReflowChildren(nsPresCo
>     nsRect kidRect = kidFrame->GetRect();
>     nsRect kidVisualOverflow = kidFrame->GetVisualOverflowRect();

And do you need those?

>@@ -1240,33 +1221,26 @@ nsTableRowFrame::CollapseRowIfNecessary(
>-          nsTableFrame::InvalidateFrame(cellFrame, oldCellRect,
>-                                        oldCellVisualOverflow,

oldCellRect and oldCellVisualOverflow are probably unused now...

>-  nsTableFrame::InvalidateFrame(this, oldRect, oldVisualOverflow, false);

Likewise for oldRect and oldVisualOverflow.

>+nsTableRowFrame::InvalidateFrame()

The usual question here.

>+++ b/layout/tables/nsTableRowGroupFrame.cpp
> nsTableRowGroupFrame::PlaceChild(nsPresContext*         aPresContext,

This has some extraneous arguments too, now.  Followup bug to remove them?

>@@ -426,24 +420,24 @@ nsTableRowGroupFrame::ReflowChildren(nsP
>-              Invalidate(dirtyRect);
>+              InvalidateFrame();

The changes here don't make sense.  Why do we need to InvalidateFrame() twice?  Why do we compute an unused dirtyRect?

>@@ -483,17 +477,17 @@ nsTableRowGroupFrame::ReflowChildren(nsP
>       // XXX We should change CalculateRowHeights() to return the bounding
>       // rect of what changed. Or whether anything moved or changed size...
>       nsRect  dirtyRect(0, 0, mRect.width, mRect.height);
>-      Invalidate(dirtyRect);
>+      InvalidateFrame();

Fix the comment?  And do we still need dirtyRect?

>+nsTableRowGroupFrame::InvalidateFrame()

Usual question.

>+++ b/layout/xul/base/src/nsBox.cpp
>@@ -637,20 +637,19 @@ nsIFrame::Redraw(nsBoxLayoutState& aStat
>+  InvalidateFrameSubtree();

>   // nsStackLayout, at least, expects us to repaint descendants even
>   // if a damage rect is provided

We're no longer providing a damage rect...  Should this comment precede the InvalidateFrameSubtree call?

>+++ b/layout/xul/base/src/nsSliderFrame.cpp
>@@ -711,17 +711,17 @@ nsSliderFrame::CurrentPositionChanged(ns
>+  //InvalidateFrame();

Why is this commented out?

>+++ b/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
>@@ -702,17 +702,17 @@ nsTreeBodyFrame::InvalidateColumn(nsITre
>+      nsIFrame::InvalidateFrame();

Why the nsIFrame qualifier?

>@@ -723,17 +723,17 @@ nsTreeBodyFrame::InvalidateRow(PRInt32 a
>+  nsIFrame::InvalidateFrame();

Here too.

>@@ -753,17 +753,17 @@ nsTreeBodyFrame::InvalidateCell(PRInt32 
>+    nsIFrame::InvalidateFrame();

And here.

>@@ -786,17 +786,17 @@ nsTreeBodyFrame::InvalidateRange(PRInt32
>-  nsIFrame::Invalidate(rangeRect);
>+  nsIFrame::InvalidateFrame();

And here.  Plus, do we need rangeRect here and various other places in this file?

>@@ -829,17 +829,17 @@ nsTreeBodyFrame::InvalidateColumnRange(P
>+  nsIFrame::InvalidateFrame();

And here.

>@@ -2573,26 +2573,26 @@ nsTreeBodyFrame::HandleEvent(nsPresConte
>+      //if (mMouseOverRow != -1)
>+        //InvalidateRow(mMouseOverRow);

Why?

>+      //if (mMouseOverRow != -1)
>+        //InvalidateRow(mMouseOverRow);

And here.

>+      //InvalidateRow(mMouseOverRow);

And here.

It's nice to see a bunch of that ugly code go away!
Attachment #605264 - Flags: review?(bzbarsky) → review-
Comment on attachment 605177 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +721,5 @@
> +      if (groupContext->GetFlags() & gfxContext::FLAG_DISABLE_SNAPPING) {
> +        gfxUtils::ClipToRegion(aContext, toDraw);
> +      } else {
> +        gfxUtils::ClipToRegionSnapped(aContext, toDraw);
> +      }

Why do we need to add this clipping? This adds overhead that we don't need if it's just for paint flashing.

@@ +1695,5 @@
> +
> +  if (!sPaintFlashingPrefCached) {
> +    sPaintFlashingPrefCached = true;
> +    mozilla::Preferences::AddBoolVarCache(&sPaintFlashingEnabled, 
> +                                          "nglayout.debug.composite_flashing");

Can we call this something else, say nglayout.debug.widget_update_flashing?

Will need to add the pref to all.js, with a comment that it only affects BasicLayers. And call this FlashWidgetUpdateArea --- we need to try hard to make sure this doesn't get confused with the other paint flashing.
Comment on attachment 605178 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder

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

::: layout/base/FrameLayerBuilder.cpp
@@ +94,5 @@
> +  MOZ_COUNT_DTOR(LayerManagerLayerBuilder);
> +  if (mDelete) {
> +    delete mLayerBuilder;
> +  }
> +};

stray semicolon

@@ +866,5 @@
>    // ThebesLayer to ensure that snapping exactly matches the ideal transform.
>    layer->SetAllowResidualTranslation(
>        mParameters.mInTransformedSubtree && !mParameters.mInActiveTransformedSubtree);
>  
> +  GetLayerBuilderForManager(mManager)->SaveLastPaintOffset(layer);

I think we should just give ContainerState a pointer to its FrameLayerBuilder in its constructor.

::: layout/base/FrameLayerBuilder.h
@@ +93,5 @@
> +
> +static inline FrameLayerBuilder *GetLayerBuilderForManager(layers::LayerManager* aManager)
> +{
> +  LayerManagerLayerBuilder *data = static_cast<LayerManagerLayerBuilder*>(aManager->GetUserData(&gLayerManagerLayerBuilder));
> +  return data->mLayerBuilder;

data ? data->mLayerBuilder : nsnull

::: layout/base/nsDisplayList.cpp
@@ +603,5 @@
>      BuildContainerLayerFor(aBuilder, layerManager, aForFrame, nsnull, *this,
>                             containerParameters, nsnull);
> +
> +  if (!root) {
> +    layerManager->SetUserData(&gLayerManagerLayerBuilder, nsnull);

RemoveUserData?

@@ +646,5 @@
>      FrameLayerBuilder::InvalidateAllLayers(layerManager);
>    }
>  
>    nsCSSRendering::DidPaint();
> +  layerManager->SetUserData(&gLayerManagerLayerBuilder, nsnull);

RemoveUserData?
Also I think we can give FrameLayerBuilder a pointer to its layer manager, now that there's only one, and remove a bunch of aManager parameters.
Maybe it would make sense to have the FrameLayerBuilder always be heap-allocated and owned by the layer manager user data? Seems a bit simpler.
Matt, can we remove the invalidation in nsFrameManager::RemoveFrame?
Yes we can, I've removed it locally.
No longer blocks: 707960
Comment on attachment 605176 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction

This looks okay to me, but I don't feel familiar enough with layers to review it. Can you get review from someone who does?

One suggestion regarding the patch - rename LayerState::LAYER_SVG to LayerState::LAYER_SVG_EFFECTS? Otherwise it sounds like it's a layer for SVG.
Comment on attachment 605267 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget

>+  // TODO: nsSubDocumentFrame sometimes creates nsDisplayOwnLayer objects
>+  // with a child frame pointer. Do we want to catch all nsDisplayOwnLayer
>+  // objects here, or should we add a way to detect if these were created
>+  // by an nsSubDocumentFrame.
>+  //if (mFrame->GetType() == nsGkAtoms::subDocumentFrame) {
>+    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
>+    pres->mPresContext = mFrame->PresContext();
>+    layer->SetUserData(&gContainerLayerPresContext, pres);
>+  //}
>   return layer.forget();

I think you only want to do this for sub document frames. But subdocument frames create nsDisplayZoom items when the zoom is different on the child document, so I think you'll want to handle those as well.
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>-void nsViewManager::ProcessPendingUpdatesForView(nsView* aView,
>+bool nsViewManager::ProcessPendingUpdatesForView(nsView* aView,

It looks like the only way this can return true is if it's passed a null aView or somehow comes across one when recursing through child views. Does this need to return a bool?
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

> PresShell::Paint(nsIView*           aViewToPaint,
>-    if (!(frame->GetStateBits() & NS_FRAME_UPDATE_LAYER_TREE)) {
>-      if (layerManager->EndEmptyTransaction()) {
>-        frame->UpdatePaintCountForPaintedPresShells();
>-        presContext->NotifyDidPaintForSubtree();
>-        return;
>-      }
>+    if (aType == PaintType_Composite) {
>+      DebugOnly<bool> result = layerManager->EndEmptyTransaction();
>+      NS_ASSERTION(result, "Must complete empty transaction when compositing!");
>+      return;
>     }
> 
>     frame->RemoveStateBits(NS_FRAME_UPDATE_LAYER_TREE);

So NS_FRAME_UPDATE_LAYER_TREE will be obsolete with these patches? It doesn't look like it gets fully removed by sum result of all the patches here so far.
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

Joe, someone familiar with the accelerated layer backends should review the changes in that code. Could you find someone appropriate?
Attachment #605179 - Flags: review?(joe)
Depends on: 739490
Depends on: 739671
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

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

Bas, can you look at the D3D{9,10} changes here? I've reviewed BasicLayers and OpenGL layers.

::: gfx/layers/basic/BasicLayers.cpp
@@ +1662,5 @@
>  
> +    if (aFlags & END_NO_COMPOSITE) {
> +      if (IsRetained()) {
> +        mTarget->Clip(gfxRect(0, 0, 0, 0));
> +        PaintLayer(mTarget, mRoot, aCallback, aCallbackData, nsnull);

This will need a comment explaining why we paint the layer clipped to nothing.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +444,5 @@
>      mRoot->ComputeEffectiveTransforms(gfx3DMatrix());
>  
>      mThebesLayerCallback = aCallback;
>      mThebesLayerCallbackData = aCallbackData;
> +    SetCompositingDisabled(aFlags & END_NO_COMPOSITE);

Why not just return, instead of going through the rest of the mechanics of rendering?

If you do just return, you don't need the weirdness in ContainerLayerOGL.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +510,5 @@
>        mTexImage = nsnull;
>        mTexImageOnWhite = nsnull;
>        mBufferRect.SetRect(0, 0, 0, 0);
>        mBufferRotation.MoveTo(0, 0);
> +      printf("Clearing EVERYTHING\n");

debugging printf
Attachment #605179 - Flags: review?(joe)
Attachment #605179 - Flags: review?(bas.schouten)
Attachment #605179 - Flags: review-
Depends on: 739743
(In reply to Joe Drew (:JOEDREW!) from comment #88)

> >      mRoot->ComputeEffectiveTransforms(gfx3DMatrix());
> >  
> >      mThebesLayerCallback = aCallback;
> >      mThebesLayerCallbackData = aCallbackData;
> > +    SetCompositingDisabled(aFlags & END_NO_COMPOSITE);
> 
> Why not just return, instead of going through the rest of the mechanics of
> rendering?
> 
> If you do just return, you don't need the weirdness in ContainerLayerOGL.
> 

Wouldn't this skip ThebesLayer drawing? We still need to make sure that all ThebesLayers have fully valid contents.
Hm, good point. Ugh. It'd be nice if compositing and validation were explicitly separated.
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>diff --git a/layout/base/nsDisplayList.h b/layout/base/nsDisplayList.h
>--- a/layout/base/nsDisplayList.h
>+++ b/layout/base/nsDisplayList.h
>   enum {
>     PAINT_DEFAULT = 0,
>     PAINT_USE_WIDGET_LAYERS = 0x01,
>     PAINT_FLUSH_LAYERS = 0x02,
>-    PAINT_EXISTING_TRANSACTION = 0x04
>+    PAINT_EXISTING_TRANSACTION = 0x04,
>+    PAINT_NO_COMPOSITE = 0x08,
>+    PAINT_USE_CACHED_LAYER_MANAGER = 0x10

PAINT_USE_CACHED_LAYER_MANAGER doesn't seem to be used in this patch or any other patch in this bug.
Comment on attachment 605265 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model

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

If we have to use InvalidatedInSubtree() to crawl the descendants of all nsSVGForeignObjectFrame frames looking to see if they contain a dirty descendant then I guess that's what we have to do. That part will only have a perf impact on SVGs that contain foreignObjects after all. We shouldn't slow down _all_ SVGs by crawling the entire SVG frame tree looking for the nsSVGForeignObjectFrame frames though. Instead we can use the foreignObject registering code that I removed in bug 734079. I'll attach a patch for you that reinstates the parts of that code that you'll need is a minute.
Attachment #605265 - Flags: review?(jwatt) → review-
When SVG is converted to display lists, it can participate fully in this framework so we won't need to do that crawling.
Indeed, I've put comments to that affect in the patch.
Depends on: 741682
Depends on: 746421
Depends on: 746422
Blocks: 691645
Depends on: 746890
Blocks: 702485
Matt your try build shows significant less frame drops here :)
https://bugzilla.mozilla.org/show_bug.cgi?id=702485
Blocks: 747561
Depends on: 747718
Blocks: 748219
Blocks: 748220
Blocks: 748228
Depends on: 749055
Attachment #610380 - Flags: review?(matt.woodrow) → review+
Attachment #605171 - Attachment is obsolete: true
Attachment #618520 - Flags: review?(roc)
Attachment #605171 - Flags: review?(roc)
Attachment #618521 - Flags: review?(roc)
Attachment #605172 - Attachment is obsolete: true
Attachment #605172 - Flags: review?(roc)
Attachment #605176 - Attachment is obsolete: true
Attachment #605176 - Flags: review?(jwatt)
Attachment #605177 - Attachment is obsolete: true
Attachment #605177 - Flags: review?(roc)
Attachment #605179 - Attachment is obsolete: true
Attachment #618525 - Flags: review?(tnikkel)
Attachment #605179 - Flags: review?(tnikkel)
Attachment #605179 - Flags: review?(bas.schouten)
Attachment #605261 - Attachment is obsolete: true
Attachment #618526 - Flags: review?(roc)
Attachment #605261 - Flags: review?(roc)
Attachment #605262 - Attachment is obsolete: true
Attachment #618527 - Flags: review?(roc)
Attachment #605262 - Flags: review?(roc)
Attachment #605264 - Attachment is obsolete: true
Attachment #618528 - Flags: review?(bzbarsky)
Attachment #605265 - Attachment is obsolete: true
Attachment #618529 - Flags: review?(jwatt)
Attachment #605266 - Attachment is obsolete: true
Attachment #618530 - Flags: review?(roc)
Attachment #605266 - Flags: review?(roc)
Attachment #618530 - Flags: review?(tnikkel)
Attachment #605267 - Attachment is obsolete: true
Attachment #618531 - Flags: review?(roc)
Attachment #605267 - Flags: review?(roc)
Attachment #605268 - Attachment is obsolete: true
Attachment #618534 - Flags: review?(roc)
Attachment #605268 - Flags: review?(roc)
Not sure who should review something like this.

I also regret not writing down all my conclusions that lead to these changes. Can try figure them out again if anyone has specific questions.
Attachment #618536 - Flags: review?(roc)
Attachment #618537 - Flags: review?(roc)
This was required in part 7, since inactive layer transactions didn't use their own FrameLayerBuilders.

After part 9, this is no longer the case, so this cleans up the unnecessary code.
Attachment #618541 - Flags: review?(roc)
Reduce time spent in building display lists by only repainting widgets that have had changes made.
Attachment #618542 - Flags: review?(tnikkel)
In some cases (ie OSX titlebar painting) we can have multiple retained layer managers storing the same frame.

This updates the frame data storage code to be able to handle this.
Attachment #618543 - Flags: review?(roc)
The currently attached patch queue passes all tests except for Native Android crashes caused by bug 746890.

Patch queue updated here: http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi
Comment on attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

Can you just make the two or three tests that fail in test_panel.xul todo instead of not running the whole test at all on linux?
Comment on attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2

A paragraph or two outlining the changes this patch makes, or the new setup it creates, would be helpful.
(In reply to Timothy Nikkel (:tn) from comment #117)
> Comment on attachment 618530 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation v2
> 
> A paragraph or two outlining the changes this patch makes, or the new setup
> it creates, would be helpful.

Sure.

This patch removes the remainder of the old invalidation code (ThebesLayerInvalidRegionProperty, SetHasContainerLayer etc).

It also replaces this with the new invalidation code, using display list analysis. Most of this is within InvalidateForLayerChange(), but it also adds code to AddLayerDisplayItem to store the geometries for the current display list, and mark items that were reused from the last paint (mUsed).

It also adds ProcessRemovedDisplayItems to find display items from the previous paint (ones without the mUsed bit set), and invalidate them. This is because the InvalidateForLayerChange code never gets called for these.

It also modifies the RemoveFrameFromLayerManager frame property destructor to invalidate the areas once occupied by display items belonging to that frame.

The other main part of this patch is retaining display item information, layers and layer managers (but not layer buffers) for inactive layers. This is required to be able to do display list comparison for changes within inactive layers. For this we create a new FrameLayerBuilder when executing a inactive layer transaction (and call DidBeginRetainedLayerTransaction), and cache the layer manager in DisplayItemData.

This adds some complexity, since frames are no longer associated with a single layer manager, which the existing frame property setup assumed. We now store the LayerManagerData for the outer-most LayerManager, and you can walk down through inactive layer managers to find the others from there. See the comment above RemoveFrameFromLayerManager for more on how this affects things.


(In reply to Timothy Nikkel (:tn) from comment #116)
> Comment on attachment 618536 [details] [diff] [review]
> Part 10: Test modifications required for DLBI
> 
> Can you just make the two or three tests that fail in test_panel.xul todo
> instead of not running the whole test at all on linux?

Sure, will do.
More or less the same framework as layout/base/tests/test_bug450930.xhtml. Should be easy to add more tests to this as needed.

Hope this is testing the right thing, works first time with DLBI though :)
Attachment #619830 - Flags: review?(bzbarsky)
Comment on attachment 618529 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model

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

r=jwatt

::: layout/svg/base/src/nsSVGForeignObjectFrame.cpp
@@ +595,5 @@
> +nsSVGForeignObjectFrame::GetInvalidRegion()
> +{
> +  nsIFrame* kid = GetFirstPrincipalChild();
> +  if (kid->InvalidatedInSubtree()) {
> +    gfxRect r(mRect.x, mRect.y, mRect.width, mRect.height);

This will work for now, but only because there is a bug in nsSVGForeignObjectFrame::UpdateBounds() where it uses gfxRect(0.0, 0.0, w, h) when setting mRect instead of using gfxRect(x, y, w, h). I've fixed that in a patch elsewhere, so to avoid unexpected errors in the event that I land first I'd suggest that you should make that change to this patch too (and move the "// GetCanvasTM includes the x,y translation" comment down to just above the line that sets mCoveredRegion). That done, you can change the line above in this patch here to |gfxRect r(0.0, 0.0, mRect.width, mRect.height)| (and probably add a "// GetCanvasTM includes the x,y translation" there too).
Attachment #618529 - Flags: review?(jwatt) → review+
Blocks: 750417
Comment on attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2

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

::: gfx/layers/Layers.h
@@ +200,4 @@
>  
>    void Set(void* aKey, LayerUserData* aValue)
>    {
> +    mUserData.Add((gfx::UserDataKey *)aKey, aValue, LayerManagerUserDataDestroy);

Make these static_casts.

Better still, instead of casting these, why not actually use gfx::UserData in the API and in the callers?
Comment on attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +1436,5 @@
>    MOZ_LAYERS_LOG(("[----- BeginTransaction"));
>    Log();
>  #endif
>  
> +  //NS_ASSERTION(!InTransaction(), "Nested transactions not allowed");

Why is this commented out?
Comment on attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed

I believe this is obsolete now
Attachment #605175 - Attachment is obsolete: true
Attachment #605175 - Flags: review?(roc)
Comment on attachment 618526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v2

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

::: layout/base/nsDisplayList.h
@@ +700,5 @@
> +    nsDisplayItemGenericGeometry* geometry = new nsDisplayItemGenericGeometry();
> +    bool snap;
> +    geometry->mBounds = GetBounds(aBuilder, &snap);
> +    geometry->mBorderRect = GetBorderRect();
> +    return geometry;

I'd prefer it if GetXYZ() functions didn't allocate. Call this AllocateGeometry?

@@ +704,5 @@
> +    return geometry;
> +  }
> +
> +  virtual nsRegion ComputeInvalidationRegion(nsDisplayListBuilder* aBuilder,
> +                                             const nsDisplayItemGeometry* aGeometry)

Both this and GetGeometry need some major documentation.

::: layout/base/nsDisplayListInvalidation.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + *
> + * vim: set ts=2 sw=2 et tw=78:
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Update this (and other new files) to use the new license block.

@@ +43,5 @@
> +
> +class nsDisplayItemGeometry
> +{
> +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> +public:

This entire file is extremely comment-deficient.
Comment on attachment 618527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v2

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

::: layout/generic/nsIFrame.h
@@ +2225,5 @@
>    void InvalidateOverflowRect();
>  
>    /**
> +   * Marks all display items created by this frame as needing a repaint,
> +   * and schedules a view manager flush to trigger painting

Is this leaf display items or all display items (including containers such as nsDisplayTransform etc)?

@@ +2232,5 @@
> +  
> +  /**
> +   * Invalidate the entire frame subtree for this frame.
> +   */
> +  void InvalidateFrameSubtree();

What does this do exactly? Call InvalidateFrame on all descendant frames?

@@ +2237,5 @@
> +  
> +  /**
> +   * Checks if a frame has been marked as invalid.
> +   */
> +  bool Invalidated();

IsInvalid()? And what exactly does this return?

@@ +2243,5 @@
> +  /**
> +   * Check if this frame within the frame subtree (including this frame) 
> +   * are marked as invalid.
> +   */
> +  bool InvalidatedInSubtree();

I don't understand this comment at all

@@ +2248,5 @@
> +
> +  /**
> +   * Schedule a view manager flush on the next refresh driver tick.
> +   */
> +  void SchedulePaint();

Explain in more detail what this does.

@@ +2250,5 @@
> +   * Schedule a view manager flush on the next refresh driver tick.
> +   */
> +  void SchedulePaint();
> +
> +  Layer* InvalidateLayer(const nsRect& aDamageRect, PRUint32 aDisplayItemKey);

This needs a big comment too.
I think it would make sense to update those two API patches with better comments before I proceed on to the rest of the reviews.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #124)
> 
> ::: layout/base/nsDisplayListInvalidation.h
> @@ +1,5 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > + *
> > + * vim: set ts=2 sw=2 et tw=78:
> > + * ***** BEGIN LICENSE BLOCK *****
> > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> Update this (and other new files) to use the new license block.

What is the new license block? - Or which file can I copy one from.

> ::: layout/generic/nsIFrame.h
> @@ +2225,5 @@
> >    void InvalidateOverflowRect();
> >  
> >    /**
> > +   * Marks all display items created by this frame as needing a repaint,
> > +   * and schedules a view manager flush to trigger painting
> 
> Is this leaf display items or all display items (including containers such
> as nsDisplayTransform etc)?

All display items, including containers.

> 
> @@ +2232,5 @@
> > +  
> > +  /**
> > +   * Invalidate the entire frame subtree for this frame.
> > +   */
> > +  void InvalidateFrameSubtree();
> 
> What does this do exactly? Call InvalidateFrame on all descendant frames?

Yep.

> 
> @@ +2237,5 @@
> > +  
> > +  /**
> > +   * Checks if a frame has been marked as invalid.
> > +   */
> > +  bool Invalidated();
> 
> IsInvalid()? And what exactly does this return?


Returns if Invalidate() has been called on this frame since the last time we painted it.

> 
> @@ +2243,5 @@
> > +  /**
> > +   * Check if this frame within the frame subtree (including this frame) 
> > +   * are marked as invalid.
> > +   */
> > +  bool InvalidatedInSubtree();
> 
> I don't understand this comment at all

Checks if *any* frame within the frame subtree....

> 
> @@ +2248,5 @@
> > +
> > +  /**
> > +   * Schedule a view manager flush on the next refresh driver tick.
> > +   */
> > +  void SchedulePaint();
> 
> Explain in more detail what this does.

Any suggestions? Not sure what else to say about this.

Will update all the comments/naming asap.
(In reply to Matt Woodrow (:mattwoodrow) from comment #127)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #124)
> > Update this (and other new files) to use the new license block.
> 
> What is the new license block? - Or which file can I copy one from.

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp

> > Is this leaf display items or all display items (including containers such
> > as nsDisplayTransform etc)?
> 
> All display items, including containers.

Please say so.

> > What does this do exactly? Call InvalidateFrame on all descendant frames?
> 
> Yep.

Please say so.

> > IsInvalid()? And what exactly does this return?
> 
> Returns if Invalidate() has been called on this frame since the last time we
> painted it.

Including via InvalidateFrame() presumably?

> > > +  bool InvalidatedInSubtree();
> > 
> > I don't understand this comment at all
> 
> Checks if *any* frame within the frame subtree....

OK, please fix that.

Would it make sense perhaps to have a frame state bit to indicate "has invalid descendant"? like we do for reflow-dirty?

> > > +  /**
> > > +   * Schedule a view manager flush on the next refresh driver tick.
> > > +   */
> > > +  void SchedulePaint();
> > 
> > Explain in more detail what this does.
> 
> Any suggestions? Not sure what else to say about this.

Say that the view manager flush will update the layer tree and repaint any invalid areas in the layer tree (and that a layer tree composite operation will happen shortly after that to get the results onto the screen).

I presume this also ensures there will be a next refresh driver tick, so you'd better say that as well.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #128)
> Including via InvalidateFrame() presumably?

Yes, that's what I meant.

> 
> Would it make sense perhaps to have a frame state bit to indicate "has
> invalid descendant"? like we do for reflow-dirty?

I'm not sure that would be a useful piece of information, unless we want to walk up the frame tree when determining if a display item needs to be repainted.

This hasn't shown up in profiles as being a problem, we could look into faster ways of doing this if it does though.
Attachment #618526 - Attachment is obsolete: true
Attachment #620926 - Flags: review?(roc)
Attachment #618526 - Flags: review?(roc)
Attachment #618527 - Attachment is obsolete: true
Attachment #620927 - Flags: review?(roc)
Attachment #618527 - Flags: review?(roc)
Comment on attachment 620926 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v3

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

Could we avoid giving nsDisplayItemGeometry virtual methods by putting those virtual methods on the associated nsDisplayItem, and making nsDisplayItemGeometry essentially a private data blob for the display item? I think that would be a little bit simpler, avoid exposing nsDisplayItemGeometry details, and be more efficient (no vtable, hopefully no refcount).

Also, it might be a good idea to allocate nsDisplayItemGeometry objects from the presshell arena.

::: layout/base/nsDisplayList.cpp
@@ +1184,5 @@
>        *aForceTransparentSurface = disp->mAppearance == NS_THEME_WIN_BORDERLESS_GLASS ||
>                                    disp->mAppearance == NS_THEME_WIN_GLASS;
>      }
>      if (mThemeTransparency == nsITheme::eOpaque) {
> +      result = nsRect(nsPoint(0,0), mFrame->GetSize()) + ToReferenceFrame();

nsRect(ToReferenceFrame(), mFrame->GetSize())

::: layout/base/nsDisplayList.h
@@ +702,5 @@
> +   * and determine if repainting needs to happen.
> +   *
> +   * Subclasses wishing to store more information need to override both this
> +   * and ComputeInvalidationRegion, as well as implementing an nsDisplayItemGeometry
> +   * subclass.

Say something about what the default implementation does here.

@@ +724,5 @@
> +   *
> +   * @param aGeometry The geometry of the matching display item from the 
> +   * previous paint.
> +   * @return The invalidation area required to reflect the changes between 
> +   * the previous and current display items.

Say something about the default implementation here.

@@ +727,5 @@
> +   * @return The invalidation area required to reflect the changes between 
> +   * the previous and current display items.
> +   */
> +  virtual nsRegion ComputeInvalidationRegion(nsDisplayListBuilder* aBuilder,
> +                                             const nsDisplayItemGeometry* aGeometry)

Consider having this take an nsRegion* out parameter, and be defined to add the invalid area to that region. That is a bit more efficient than returning a new region.

@@ +1666,5 @@
>    NS_DISPLAY_DECL_NAME("Background", TYPE_BACKGROUND)
>    // Returns the value of GetUnderlyingFrame()->IsThemed(), but cached
>    bool IsThemed() { return mIsThemed; }
>  
> +  bool RenderingMightDependOnFrameSize();

Document this

@@ +1771,5 @@
> +    if (!geometry->mPaddingRect.IsEqualInterior(GetPaddingRect())) {
> +      // nsDisplayBoxShadowInner is based around the padding rect, but it can
> +      // touch pixels outside of this. We should invalidate the entire bounds.
> +      bool snap;
> +      invalid = invalid.Or(geometry->mBounds, GetBounds(aBuilder, &snap));

don't assign the result of the Or here.

::: layout/base/nsDisplayListInvalidation.h
@@ +10,5 @@
> + * This stores the geometry of an nsDisplayItem, and the area
> + * that will be affected when painting the item.
> + *
> + * It is used to retain information about display items so they
> + * can be compared against new display items in the next paintA

fix typo

@@ +14,5 @@
> + * can be compared against new display items in the next paintA
> + */
> +class nsDisplayItemGeometry
> +{
> +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)

Why do we need to refcount these? That seems unfortunate, and it seems to me we should be able to manually manage these.

@@ +38,5 @@
> +  PRInt32 mAppUnitsPerDevPixel;
> +
> +  /**
> +   * The offset (in pixels) of the TopLeft() of the buffer
> +   * this display item was drawn into.

I'm not quite sure what this means. Is this the display item's GetUnderlyingFrame()->ToReferenceFrame()? Or something else?

@@ +46,5 @@
> +  /**
> +   * The offset (in app units) of the TopLeft() of the buffer
> +   * this display item was drawn into.
> +   */
> +  nsPoint mAppUnitsOffset;

Why do you store both this and mPaintOffset?

::: layout/generic/nsCanvasFrame.h
@@ +246,5 @@
> +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> +    nsRegion invalid;
> +    if (ShouldFixToViewport(aBuilder)) {
> +      // This is incorrect, We definitely need to check more things here. 
> +      return invalid;

So ... what needs to be fixed here? :-)

@@ +259,5 @@
> +        !geometry->mContentRect.IsEqualInterior(GetContentRect())) {
> +      if (!RenderingMightDependOnFrameSize() && geometry->mBounds.TopLeft() == GetBounds(aBuilder, &snap).TopLeft()) {
> +        nsRect vertical, horizontal;
> +        nsLayoutUtils::GetRectDifferenceStrips(geometry->mBounds, GetBounds(aBuilder, &snap), &horizontal, &vertical);
> +        invalid = invalid.Or(vertical, horizontal);

Why not just write invalid.Xor(geometry->mBounds, GetBounds(aBuilder, &snap))?

::: layout/generic/nsTextFrame.h
@@ +542,5 @@
> +    bool operator!=(const LineDecoration& aOther) const {
> +      return mFrame != aOther.mFrame ||
> +             mStyle != aOther.mStyle ||
> +             mColor != aOther.mColor ||
> +             mBaselineOffset != aOther.mBaselineOffset;

return !(*this == aOther);

@@ +567,5 @@
> +    bool operator!=(const TextDecorations& aOther) const {
> +      return mOverlines != aOther.mOverlines ||
> +             mUnderlines != aOther.mUnderlines ||
> +             mStrikes != aOther.mStrikes;
> +    }

Define an operator== and then define operator!= in terms of that.

::: layout/generic/nsTextFrameThebes.cpp
@@ +4385,5 @@
> +  {
> +    aFrame->GetTextDecorations(aFrame->PresContext(), mDecorations);
> +  }
> +  
> +  nsTextFrame::TextDecorations mDecorations;

Add a comment explaining why decorations need to be stored and checked and why style changes aren't enough to pick it up.
Comment on attachment 620927 [details] [diff] [review]
Part 9b - Add new frame invalidation API v3

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

Looks like you're maintaining some kind of paint generation counter here. It would be much nicer if you can just keep a frame state bit. Why can't you?

::: layout/generic/nsFrame.cpp
@@ +4753,5 @@
> +void
> +nsIFrame::InvalidateFrame()
> +{
> +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");

Getting the displayRoot here is bad, it requires a walk to the root of the frame tree.

@@ +4766,5 @@
> +{ 
> +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> +  if (displayRoot) {
> +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();

This is confusing. So after a call to InvalidateFrame(), IsInvalid returns false?

::: layout/generic/nsIFrame.h
@@ +2853,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  PRUint32         mInvalidPaint;

What is this? Adding to nsFrame is to be avoided if at all possible.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #132)
 
> Could we avoid giving nsDisplayItemGeometry virtual methods by putting those
> virtual methods on the associated nsDisplayItem, and making
> nsDisplayItemGeometry essentially a private data blob for the display item?
> I think that would be a little bit simpler, avoid exposing
> nsDisplayItemGeometry details, and be more efficient (no vtable, hopefully
> no refcount).

The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when the corresponding item *isn't* added to the thebes layer (ie, content being removed).
So we don't have an nsDisplayItem instance with which to call a virtual function.

> 
> Also, it might be a good idea to allocate nsDisplayItemGeometry objects from
> the presshell arena.

Out of interest, what are the advantages of doing this? Disadvantages?

> > +{
> > +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> 
> Why do we need to refcount these? That seems unfortunate, and it seems to me
> we should be able to manually manage these.

I added these when debugging a leak, they can probably go now.

> 
> @@ +38,5 @@
> > +  PRInt32 mAppUnitsPerDevPixel;
> > +
> > +  /**
> > +   * The offset (in pixels) of the TopLeft() of the buffer
> > +   * this display item was drawn into.
> 
> I'm not quite sure what this means. Is this the display item's
> GetUnderlyingFrame()->ToReferenceFrame()? Or something else?

No, this is the ThebesLayer offset. Maybe I should put that instead of calling it the 'buffer'.

> 
> @@ +46,5 @@
> > +  /**
> > +   * The offset (in app units) of the TopLeft() of the buffer
> > +   * this display item was drawn into.
> > +   */
> > +  nsPoint mAppUnitsOffset;
> 
> Why do you store both this and mPaintOffset?

Good question, I can probably remove one of these.

> 
> ::: layout/generic/nsCanvasFrame.h
> @@ +246,5 @@
> > +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> > +    nsRegion invalid;
> > +    if (ShouldFixToViewport(aBuilder)) {
> > +      // This is incorrect, We definitely need to check more things here. 
> > +      return invalid;
> 
> So ... what needs to be fixed here? :-)

I still haven't figured that out yet, will have another think about it.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #133)
> Looks like you're maintaining some kind of paint generation counter here. It
> would be much nicer if you can just keep a frame state bit. Why can't you?

Since each frame can have multiple display items (spread amongst multiple layer manager/frame layer builder transactions), it's hard to know when to clear the bit.

The original implementation had state bits, and then walked the entire frame tree after painting clearing them. This was (unsurprisingly) really slow.

Suggestions appreciated.

> 
> ::: layout/generic/nsFrame.cpp
> @@ +4753,5 @@
> > +void
> > +nsIFrame::InvalidateFrame()
> > +{
> > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> 
> Getting the displayRoot here is bad, it requires a walk to the root of the
> frame tree.

We need to access the painting refresh driver and associated presshell/context. Is there a faster way to do this?

> 
> @@ +4766,5 @@
> > +{ 
> > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > +  if (displayRoot) {
> > +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();
> 
> This is confusing. So after a call to InvalidateFrame(), IsInvalid returns
> false?

I guess >= would make more sense here. We don't check this until we've started a paint and incremented the paint counter, so it's 'correct', but maybe not logical.

> 
> ::: layout/generic/nsIFrame.h
> @@ +2853,5 @@
> >      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
> >    };
> >    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
> >  
> > +  PRUint32         mInvalidPaint;
> 
> What is this? Adding to nsFrame is to be avoided if at all possible.

This is the aforementioned generation counter.
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when
> the corresponding item *isn't* added to the thebes layer (ie, content being
> removed).
> So we don't have an nsDisplayItem instance with which to call a virtual
> function.

When is it not mBounds? Can we make it always be mBounds?

> > Also, it might be a good idea to allocate nsDisplayItemGeometry objects from
> > the presshell arena.
> 
> Out of interest, what are the advantages of doing this? Disadvantages?

Probably none. Forget it for now.

> > I'm not quite sure what this means. Is this the display item's
> > GetUnderlyingFrame()->ToReferenceFrame()? Or something else?
> 
> No, this is the ThebesLayer offset. Maybe I should put that instead of
> calling it the 'buffer'.

Yes, just explain it better.

(In reply to Matt Woodrow (:mattwoodrow) from comment #135)
> Since each frame can have multiple display items (spread amongst multiple
> layer manager/frame layer builder transactions), it's hard to know when to
> clear the bit.
> 
> The original implementation had state bits, and then walked the entire frame
> tree after painting clearing them. This was (unsurprisingly) really slow.

If you have two state bits --- "I'm dirty" and "I have a dirty descendant" --- then walking the frame tree to clear them would be efficient since you only have to walk the has-dirty-descendant frames.

> > ::: layout/generic/nsFrame.cpp
> > @@ +4753,5 @@
> > > +void
> > > +nsIFrame::InvalidateFrame()
> > > +{
> > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > 
> > Getting the displayRoot here is bad, it requires a walk to the root of the
> > frame tree.
> 
> We need to access the painting refresh driver and associated
> presshell/context. Is there a faster way to do this?

Good question. One approach would be to use Yet Another State Bit to indicate when a frame is inside a popup (so its display root is not the root frame of the root document). Then this could be fast in the common case --- PresContext()->GetRootPresContext() etc.

> > @@ +4766,5 @@
> > > +{ 
> > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > > +  if (displayRoot) {
> > > +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();
> > 
> > This is confusing. So after a call to InvalidateFrame(), IsInvalid returns
> > false?
> 
> I guess >= would make more sense here. We don't check this until we've
> started a paint and incremented the paint counter, so it's 'correct', but
> maybe not logical.

Right.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #136)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when
> > the corresponding item *isn't* added to the thebes layer (ie, content being
> > removed).
> > So we don't have an nsDisplayItem instance with which to call a virtual
> > function.
> 
> When is it not mBounds? Can we make it always be mBounds?

Items like borders, box shadow outer etc shouldn't need to invalidate the entirety of the bounds.

I think I avoided that for now, since a few corner cases *do* require the bounds to be invalidated, but it seems like an optimizations we'd likely do in the near future.
 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #135)
> > Since each frame can have multiple display items (spread amongst multiple
> > layer manager/frame layer builder transactions), it's hard to know when to
> > clear the bit.
> > 
> > The original implementation had state bits, and then walked the entire frame
> > tree after painting clearing them. This was (unsurprisingly) really slow.
> 
> If you have two state bits --- "I'm dirty" and "I have a dirty descendant"
> --- then walking the frame tree to clear them would be efficient since you
> only have to walk the has-dirty-descendant frames.

Not sure I'm convinced that this is cleaner than a paint generation counter, but sure :)

> 
> > > ::: layout/generic/nsFrame.cpp
> > > @@ +4753,5 @@
> > > > +void
> > > > +nsIFrame::InvalidateFrame()
> > > > +{
> > > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > > 
> > > Getting the displayRoot here is bad, it requires a walk to the root of the
> > > frame tree.
> > 
> > We need to access the painting refresh driver and associated
> > presshell/context. Is there a faster way to do this?
> 
> Good question. One approach would be to use Yet Another State Bit to
> indicate when a frame is inside a popup (so its display root is not the root
> frame of the root document). Then this could be fast in the common case ---
> PresContext()->GetRootPresContext() etc.

Is there an easy way to set this bit? Or should I wait until I need it, walk up the frame tree the first time and set the bit then if it's valid?
Comment on attachment 619830 [details] [diff] [review]
Part 15 - Add table invalidation test

This looks pretty good, by why do you need that enablePrivilege call?
Attachment #619830 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #138)
> Comment on attachment 619830 [details] [diff] [review]
> Part 15 - Add table invalidation test
> 
> This looks pretty good, by why do you need that enablePrivilege call?

With DLBI, only privileged content can retrieve the invalidation rects from a MozAfterPaint event (since we no longer have a way to determine if the rect is due to an invalidation from a sub-document).
Ah.  In that case, would it work to remove the enablePrivilege bit and do this instead:

  event = SpecialPowers.wrap(event);

?  That basically puts a system-privileged proxy around the event which then makes all your gets and method calls happen with chrome privileges, as I understand.
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > > +{
> > > +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> > 
> > Why do we need to refcount these? That seems unfortunate, and it seems to me
> > we should be able to manually manage these.
> 
> I added these when debugging a leak, they can probably go now.

I take that back, nsTArray requires a copy constructor, which we can't have without refcounting.

ISTR running into issues with the hashtable too.

(In reply to Boris Zbarsky (:bz) from comment #140)
> Ah.  In that case, would it work to remove the enablePrivilege bit and do
> this instead:
> 
>   event = SpecialPowers.wrap(event);
> 
> ?  That basically puts a system-privileged proxy around the event which then
> makes all your gets and method calls happen with chrome privileges, as I
> understand.

Will try it!
(In reply to Matt Woodrow (:mattwoodrow) from comment #141)
> I take that back, nsTArray requires a copy constructor, which we can't have
> without refcounting.

Wouldn't nsTArray<nsAutoPtr<nsDisplayItemGeometry> > work?
> > Good question. One approach would be to use Yet Another State Bit to
> > indicate when a frame is inside a popup (so its display root is not the root
> > frame of the root document). Then this could be fast in the common case ---
> > PresContext()->GetRootPresContext() etc.
> 
> Is there an easy way to set this bit? Or should I wait until I need it, walk
> up the frame tree the first time and set the bit then if it's valid?

You could inherit it from parents to children in nsFrame::Init (where we do similar with other bits). Then you just need to set it when constructing root frames for popups.
And I guess you'd need to set it when constructing the root frame for a document that's inside a popup. That's not hard, ViewportFrame::Init can search the ancestors.
(In reply to Matt Woodrow (:mattwoodrow) from comment #137)
> I think I avoided that for now, since a few corner cases *do* require the
> bounds to be invalidated, but it seems like an optimizations we'd likely do
> in the near future.

How about, for now, make it nonvirtual and just return the bounds, and then later if we do that optimization you can make it virtual?
As a followup, it might be worth ripping out the call DoApplyRenderingChangeToTree behavior in UpdateViewsForTree (replace it with a call to UpdateViewsForTree or something) and making InvalidateFrameSubtree walk through placeholders.

Should IsInvalidInSubtree walk through placeholders?
Comment on attachment 618528 [details] [diff] [review]
Part 9c - Remove old invalidation code v2

Why is the SchedulePaint() call in nsCSSFrameConstructor::ContentStateChanged needed?  I guess if all that does is set up a refresh tick that's not too bad, but still, would be good to at least have a comment.

>@@ -241,44 +241,12 @@ nsImageLoader::DoRedraw(const nsRect* aD

We're going to need some other way to fix bug 698297.  :(

>@@ -611,17 +611,17 @@ nsFieldSetFrame::Reflow(nsPresContext*  

Why do you need to invalidate in there?

It's worth documenting why rows/rowgroups/cols/colgroups need to override InvalidateFrame() and invalidate the entire table.

r=me modulo these nits.
Attachment #618528 - Flags: review?(bzbarsky) → review+
Attachment #620927 - Attachment is obsolete: true
Attachment #623527 - Flags: review?(roc)
Attachment #620927 - Flags: review?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > 
> > ::: layout/generic/nsCanvasFrame.h
> > @@ +246,5 @@
> > > +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> > > +    nsRegion invalid;
> > > +    if (ShouldFixToViewport(aBuilder)) {
> > > +      // This is incorrect, We definitely need to check more things here. 
> > > +      return invalid;
> > 
> > So ... what needs to be fixed here? :-)

I'll handle this in a separate patch, working on it at the moment.
Comment on attachment 623526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v4

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

r+ with that fixed.

::: layout/base/nsDisplayList.cpp
@@ +1437,5 @@
> +  if (!hasBG)
> +    return false;
> +  const nsStyleBackground* bg = bgSC->GetStyleBackground();
> +
> +  if (!bg->IsTransparent()) {

Why this check? Probably just take it out.

::: layout/base/nsDisplayList.h
@@ +772,5 @@
> +   * @param aGeometry Geometry to be shifted.
> +   * @param aOffset Offset to shift by.
> +   */
> +  virtual void MoveGeometryBy(nsDisplayItemGeometry* aGeometry, const nsPoint& aOffset)
> +  {

Shouldn't this be a virtual method on nsDisplayItemGeometry?

::: layout/base/nsDisplayListInvalidation.h
@@ +32,5 @@
> +
> +  /**
> +   * The appunits per dev pixel for the item's frame.
> +   */
> +  PRInt32 mAppUnitsPerDevPixel;

Might as well make this an nscoord
Attachment #623526 - Flags: review?(roc) → review+
Comment on attachment 623527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v4

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

::: layout/generic/nsIFrame.h
@@ +311,5 @@
>  // context?
>  #define NS_FRAME_FONT_INFLATION_FLOW_ROOT           NS_FRAME_STATE_BIT(42)
>  
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)

how about calling this NS_FRAME_NEEDS_PAINT

@@ +313,5 @@
>  
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frames that is invalid

/frames/frame/

@@ +314,5 @@
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frames that is invalid
> +#define NS_FRAME_HAS_INVALID_CHILDREN               NS_FRAME_STATE_BIT(44)

NS_FRAME_DESCENDANT_NEEDS_PAINT

@@ +1377,5 @@
>  
>    /**
> +   * Checks if the current frame-state includes all of the listed bits
> +   */
> +  bool HasStateBits(nsFrameState aBits) { return (mState & aBits) == aBits; }

HasAllStateBits

@@ +2249,5 @@
> +   *
> +   * This includes all display items created by this frame, including
> +   * container types.
> +   */
> +  virtual void InvalidateFrame(bool aSchedulePaint = true);

Use a flags word instead of a boolean parameter.

@@ +2255,5 @@
> +  /**
> +   * Calls InvalidateFrame() on all frames descendant frames (including
> +   * this one).
> +   */
> +  void InvalidateFrameSubtree(bool aSchedulePaint = true);

Here too

@@ +2267,5 @@
> +  /**
> +   * Check if any frame within the frame subtree (including this frame) 
> +   * returns true for IsInvalid().
> +   */
> +  bool IsInvalidInSubtree();

HasInvalidFrameInSubtree

@@ +2274,5 @@
> +  /**
> +   * Removes the invalid state from the current frame and all
> +   * descendant frames.
> +   */
> +  void ClearInvalidations();

ClearInvalidationStateBits

@@ +2879,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  PRUint32         mInvalidPaint;

Shouldn't this be gone?
Attachment #623527 - Attachment is obsolete: true
Attachment #623545 - Flags: review?(roc)
Attachment #623527 - Flags: review?(roc)
Comment on attachment 623545 [details] [diff] [review]
Part 9b - Add new frame invalidation API v5

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

In nsSubDocumentFrame::BeginSwapDocShells, assert that either both frames have NS_FRAME_WITHIN_POPUP or neither do. I don't think it's worth supporting swapDocShells when one shell is in a popup and the other isn't.

Add a comment to nsLayoutUtils::GetDisplayRootFrame to indicate that it can be optimized to use this new NS_FRAME_WITHIN_POPUP bit.

::: layout/generic/nsFrame.cpp
@@ +4772,5 @@
> +      nsIFrame* root = subdocumentFrame->GetSubdocumentRootFrame();
> +      if (root) {
> +        childListArray.AppendElement(nsIFrame::ChildList(
> +          nsFrameList(root, nsLayoutUtils::GetLastSibling(root)),
> +          nsIFrame::kPrincipalList));

We need to share this code with InternalInvalidateThebesLayersInSubtree (and ClearInvalidationStateBits!). Either add a method to nsIFrame (e.g. GetCrossDocChildLists, next to GetChildLists) or add a helper to nsLayoutUtils.

@@ +4871,5 @@
> +    InvalidateFrame();
> +    return nsnull;
> +  }
> +
> +  SchedulePaint();

Does InvalidateLayer actually need to schedule a view manager flush? I would have thought it doesn't need to.

@@ +4878,5 @@
> +
> +bool 
> +nsIFrame::HasInvalidFrameInSubtree()
> +{
> +  return HasAnyStateBits(NS_FRAME_NEEDS_PAINT | NS_FRAME_DESCENDANT_NEEDS_PAINT);

Might as well make this inline in nsIFrame.h

@@ +8069,5 @@
> +         f && !f->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
> +         f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
> +      f->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
> +    }
> +  }

It's probably worth fixing NS_FRAME_WITHIN_POPUP here too.

::: layout/generic/nsIFrame.h
@@ +313,5 @@
>  
> +// Frame is marked as needing painting
> +#define NS_FRAME_NEEDS_PAINT                        NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frame that needs painting

Note that this includes cross-doc descendants.

@@ +317,5 @@
> +// Frame has a descendant frame that needs painting
> +#define NS_FRAME_DESCENDANT_NEEDS_PAINT             NS_FRAME_STATE_BIT(44)
> +
> +// Frame is a descendant of a popup
> +#define NS_FRAME_WITHIN_POPUP                       NS_FRAME_STATE_BIT(45)

just NS_FRAME_IN_POPUP I think
Comment on attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2

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

Address these and then we'll go around again. Also I expect this patch needs to be updated to account for nsDisplayItemGeometry not being refcounted anymore.

::: layout/base/FrameLayerBuilder.h
@@ +288,5 @@
>     */
>    void AddLayerDisplayItem(Layer* aLayer,
>                             nsDisplayItem* aItem,
> +                           LayerState aLayerState,
> +                           nsDisplayListBuilder* aBuilder,

Let's just give FrameLayerBuilder an mDisplayListBuilder so we don't have to keep passing it around.

@@ +289,5 @@
>    void AddLayerDisplayItem(Layer* aLayer,
>                             nsDisplayItem* aItem,
> +                           LayerState aLayerState,
> +                           nsDisplayListBuilder* aBuilder,
> +                           LayerManager* aManager = nsnull);

Clarify in the comment why aManager is not aLayer->Manager().

@@ +314,5 @@
>     * that renders many display items.
>     */
> +  Layer* GetOldLayerFor(nsIFrame* aFrame, PRUint32 aDisplayItemKey, nsDisplayItemGeometry** aOldGeometry = nsnull);
> +
> +  LayerManager* GetInactiveLayerManagerFor(nsDisplayItem* aItem);

Needs a comment.

@@ +352,5 @@
>     * into a retained layer.
>     * Returns false if it was rendered into a temporary layer manager and then
>     * into a retained layer.
>     */
> +  static bool HasRetainedLayerFor(nsIFrame* aFrame, PRUint32 aDisplayItemKey, LayerManager* aManager);

This is a bit confusing. Clarify what aManager is for.

@@ +448,5 @@
>      PRUint32        mDisplayItemKey;
>      LayerState      mLayerState;
> +    nsRefPtr<LayerManager> mInactiveManager;
> +    nsRefPtr<nsDisplayItemGeometry> mGeometry;
> +    bool            mUsed;

Put the pointer members together at the start of the class.

And document what mUsed means.

@@ +484,5 @@
>  
>    // LayerManagerData needs to see DisplayItemDataEntry.
>    friend class LayerManagerData;
>  
> +  void StoreDataForFrame(nsIFrame* aFrame, const DisplayItemData& data);

Needs a comment.

@@ +529,4 @@
>      nsDisplayItem* mItem;
>      Clip mClip;
> +    nsRefPtr<LayerManager> mInactiveLayer;
> +    bool mShouldPaint;

Document these and move the mInactiveLayer pointer below mItem

@@ +562,5 @@
>    static PLDHashOperator UpdateDisplayItemDataForFrame(DisplayItemDataEntry* aEntry,
>                                                         void* aUserArg);
> +public:
> +  static PLDHashOperator ProcessRemovedDisplayItems(DisplayItemDataEntry* aEntry,
> +                                                       void* aUserArg);

Fix indent

::: layout/base/nsDisplayList.h
@@ +694,5 @@
>    nsRect GetContentRect() {
>      return GetUnderlyingFrame()->GetContentRectRelativeToSelf() + ToReferenceFrame();
>    }
>  
> +  virtual bool Invalidated() { return mFrame ? mFrame->Invalidated() : false; }

Rename to IsInvalidated like we did with nsIFrame
Comment on attachment 618531 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v2

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

::: gfx/layers/Layers.h
@@ +974,5 @@
>    nsIntRect mClipRect;
>    nsIntRect mTileSourceRect;
>    PRUint32 mContentFlags;
> +  nsIntRegion mInvalidRegion;
> +  nsIntRegion mLocalInvalidRegion;

Yikes, that makes each layer a lot bigger.

I wonder if we could just give each layer an nsIntRect mInvalidRect, nothing more. This rect would be in the same coordinate space as the mVisibleRect. When changing various layer properties we add to mInvalidRect on the layer, or for transform and clip changes, on its ancestor. Then, when we want to compute the invalid rect for the whole tree, we walk all layers and collect their trnasformed mInvalidRects (clearing them as we go). Would that work and be simpler than this?

::: layout/base/nsDisplayList.cpp
@@ +665,5 @@
> +      nsRect rect(presContext->DevPixelsToAppUnits(r->x),
> +                  presContext->DevPixelsToAppUnits(r->y),
> +                  presContext->DevPixelsToAppUnits(r->width),
> +                  presContext->DevPixelsToAppUnits(r->height));
> +      view->GetViewManager()->InvalidateViewNoSuppression(view, rect);

Can we invalidate the widget directly here instead of going through the view system? That would let us remove a bunch of view code I think.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #156)
> I wonder if we could just give each layer an nsIntRect mInvalidRect, nothing
> more. This rect would be in the same coordinate space as the mVisibleRect.
> When changing various layer properties we add to mInvalidRect on the layer,
> or for transform and clip changes, on its ancestor. Then, when we want to
> compute the invalid rect for the whole tree, we walk all layers and collect
> their trnasformed mInvalidRects (clearing them as we go). Would that work
> and be simpler than this?

One problem with that approach would be cases where we temporarily set layer properties to their non-final values, e.g. transforms on video ImageLayers where we set a transform in BuildLayer and update it in ProcessDisplayItems. Similar when PopThebesLayerData calls ConfigureLayer and then updates the transform. But I think those can fairly easily be refactored so that the layer transform is set only once.
Comment on attachment 618543 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame.

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

Instead of doing all this, it would be simpler and probably more efficient to give each LayerManager a piece of userdata which is its own dynamically-allocated FramePropertyDescriptor. Then you can just get that and do a direct property lookup to get the LayerManagerData.

::: layout/base/FrameLayerBuilder.cpp
@@ +526,5 @@
> +  for (PRUint32 i = 0; i < array->Length(); i++) {
> +    LayerManagerDataEntry& entry = array->ElementAt(i);
> +    if (entry.mOwner == sWidgetManager) {
> +      array->RemoveElementAt(i);
> +      return;

Should do something to remove the last entry if necessary.

@@ +533,5 @@
> +}
> +
> +void
> +FrameLayerBuilder::ClearManagerData(nsIFrame* aFrame, LayerManagerData* aData)
> +{

This can share code with the sWidgetManager version, right?

::: layout/base/FrameLayerBuilder.h
@@ +295,5 @@
> +    NS_ASSERTION(!sWidgetManager, "Should not be reentering widget transactions!");
> +    sWidgetManager = aManager;
> +  }
> +
> +  static void LeaveWidgetManagerTransaction()

WidgetManager sounds odd. Let's expand this out to EnterWidgetLayerManagerTransaction etc.

@@ +626,5 @@
>  
>    PRUint32                            mContainerLayerGeneration;
>    PRUint32                            mMaxContainerLayerGeneration;
> +
> +  static LayerManager*                sWidgetManager;

Document
Attachment #618530 - Attachment is obsolete: true
Attachment #623575 - Flags: review?(roc)
Attachment #618530 - Flags: review?(tnikkel)
Attachment #618530 - Flags: review?(roc)
Attachment #623545 - Attachment is obsolete: true
Attachment #623576 - Flags: review?(roc)
Attachment #623545 - Flags: review?(roc)
Comment on attachment 623575 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v3

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

With mDisplayListBuilder now in FrameLayerBuilder, we can remove a bunch of parameters from existing methods. Followup.

r+ with the following fixed...

::: layout/base/FrameLayerBuilder.cpp
@@ +601,5 @@
> +
> +  nsIntRegionRectIterator it(r);
> +  s += "< ";
> +  while (const nsIntRect* sr = it.Next())
> +    AppendToString(s, *sr) += "; ";

{}

@@ +650,5 @@
>  /* static */ void
>  FrameLayerBuilder::RemoveFrameFromLayerManager(nsIFrame* aFrame,
>                                                 void* aPropertyValue)
>  {
> +  sDestroyedFrame = aFrame;

Move this down to just above the RemoveEntry call so it's more clear what it's protecting.

@@ +765,5 @@
>    if (!newDisplayItems) {
>      // This frame was visible, but isn't anymore.
>      bool found;
> +    if (data == managerData) {
> +      props.Remove(LayerManagerDataProperty(), &found);

drop 'found' parameter

@@ +1596,5 @@
> +   * display item
> +   * @param aOpaqueRect if non-null, a region of the display item that is opaque
> +   * @param aSolidColor if non-null, indicates that every pixel in aVisibleRect
> +   * will be painted with aSolidColor by the item
> +   */

Why is this comment here? these parameters don't even make sense for this function?

@@ +1907,3 @@
>              GetTranslationForThebesLayer(newLayer));
>        }
>      }

Just return here to avoid overindenting.

@@ +1907,5 @@
>              GetTranslationForThebesLayer(newLayer));
>        }
>      }
> +  } else if (aNewLayer) {
> +    ThebesLayer* newLayer = aNewLayer->AsThebesLayer();

newThebesLayer

Seems like we're going to do a bunch of work even in the case where aNewLayer is brand new. Worth checking if GetValidRegion() is empty here?

@@ +1921,5 @@
> +#ifdef DEBUG_INVALIDATIONS
> +        printf("Display item type %s(%p) added to layer %p!\n", aItem->Name(), f, aNewLayer);
> +#endif
> +      } else if (aItem->IsInvalid()) {
> +        combined = combined.Or(geometry->ComputeInvalidationRegion(), oldGeometry->ComputeInvalidationRegion());

Get rid of unnecessary assignment

@@ +1990,5 @@
> +    if (managerData) {
> +      DisplayItemDataEntry *entry = managerData->mFramesWithLayers.GetEntry(aItem->GetUnderlyingFrame());
> +      if (entry) {
> +        for (PRUint32 i = 0; i < entry->mData.Length(); ++i) {
> +          if (entry->mData[i].mDisplayItemKey == aItem->GetPerFrameKey()) {

Seems like it might be useful to have a helper function to get the DisplayItemDataEntry for a given item and layer manager.

::: layout/base/FrameLayerBuilder.h
@@ +474,5 @@
>     */
>    class DisplayItemData {
>    public:
>      DisplayItemData(Layer* aLayer, PRUint32 aKey, LayerState aLayerState)
> +      : mLayer(aLayer), mDisplayItemKey(aKey), mLayerState(aLayerState), mUsed(false) {}

Fix order to match new declaration order
Attachment #623575 - Flags: review?(roc) → review+
Comment on attachment 623576 [details] [diff] [review]
Part 9b - Add new frame invalidation API v6

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

::: layout/generic/nsFrame.cpp
@@ +1238,5 @@
> +void
> +nsIFrame::GetCrossDocChildLists(nsTArray<ChildList>* aLists) const
> +{
> +  if (!GetFirstPrincipalChild()) {
> +    nsSubDocumentFrame* subdocumentFrame = do_QueryFrame(this);

This doesn't actually save a virtual call. So either call GetChildLists first and then scan it for a first-principal-child, or just remove the GetFirstPrincipalChild check.

@@ +4866,5 @@
> +    InvalidateFrame();
> +    return nsnull;
> +  }
> +
> +  SchedulePaint();

> Does InvalidateLayer actually need to schedule a view
> manager flush? I would have thought it doesn't need to.

@@ +8061,5 @@
> +    }
> +  }
> +
> +  if (aParent->HasAnyStateBits(NS_FRAME_IN_POPUP)) {
> +    AddStateBits(NS_FRAME_IN_POPUP);

You need to set this in all descendants too.

And if the parent's bit is not set, you need to clear this in all descendants that have it set and aren't themselves popups.

::: layout/generic/nsSubDocumentFrame.cpp
@@ +931,5 @@
>      return NS_ERROR_NOT_IMPLEMENTED;
>    }
>  
> +  NS_ASSERTION((HasAnyStateBits(NS_FRAME_IN_POPUP) && other->HasAnyStateBits(NS_FRAME_IN_POPUP)) ||
> +               (!HasAnyStateBits(NS_FRAME_IN_POPUP) && !other->HasAnyStateBits(NS_FRAME_IN_POPUP)),

HasAnyStateBits(NS_FRAME_IN_POPUP) == other->HasAnyStateBits(NS_FRAME_IN_POPUP)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #162)

> @@ +4866,5 @@
> > +    InvalidateFrame();
> > +    return nsnull;
> > +  }
> > +
> > +  SchedulePaint();
> 
> > Does InvalidateLayer actually need to schedule a view
> > manager flush? I would have thought it doesn't need to.
> 

Yes it does. This is needed to composite the layers to the screen.
Attachment #623576 - Attachment is obsolete: true
Attachment #624286 - Flags: review?(roc)
Attachment #623576 - Flags: review?(roc)
Carrying forward r=roc
Attachment #623575 - Attachment is obsolete: true
Attachment #624287 - Flags: review+
Attachment #618531 - Attachment is obsolete: true
Attachment #624289 - Flags: review?(roc)
Attachment #618531 - Flags: review?(roc)
Comment on attachment 624286 [details] [diff] [review]
Part 9b - Add new frame invalidation API v7

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

::: layout/generic/nsFrame.cpp
@@ +8062,5 @@
> +RemoveInPopupStateBitFromDescendants(nsIFrame* aFrame)
> +{
> +  if (!aFrame->HasAnyStateBits(NS_FRAME_IN_POPUP) ||
> +      aFrame->GetType() == nsGkAtoms::listControlFrame ||
> +      aFrame->GetType() == nsGkAtoms::menuPopupFrame) {

Call nsLayoutUtils::IsPopup
Attachment #624286 - Flags: review?(roc) → review+
Comment on attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2

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

pending comments in comment #121
Attachment #618520 - Flags: review?(roc) → review-
Comment on attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2

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

Pending comments in comment #122
Attachment #618521 - Flags: review?(roc) → review-
Comment on attachment 624288 [details] [diff] [review]
Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1178,3 @@
>  {
> +  if (aItemVisible.IsEmpty()) {
> +    aLayer->SetVisibleRegion(aChildBounds);

This doesn't seem right. Why do we need to do this?

If we really don't know what the visible rect is, instead of passing an empty rect, make aItemVisible a const nsIntRect* and pass null when we don't know. But why not just have the caller pass aChildBounds in that case?
Comment on attachment 624289 [details] [diff] [review]
Part 9f-2 - Compute the invalid area of the layer tree and pass this to the widget v3

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

I think we need to invalidate in SetClipRect/IntersectClipRect. Hmm, but does that mean we need to get rid of IntersectClipRect and make sure that we only SetClipRect once on the layer?

You need to collect invalidation when SetCurrentImage is called on an image container.

This is better than the previous approach to computing the invalid area, but I'm not totally happy about it. Need to think a bit.

::: gfx/layers/ImageLayers.h
@@ +593,5 @@
> +    changed.Or(mVisibleRegion, aRegion);
> +    if (!changed.IsEmpty()) {
> +      InvalidateRectInSelf(changed.GetBounds());
> +    }
> +

As below, we shouldn't need to invalidate just for a visible region change.

Isn't this actually the same as the inherited method anyway?

::: gfx/layers/Layers.h
@@ +653,5 @@
> +    nsIntRegion changed;
> +    changed.Xor(mVisibleRegion, aRegion);
> +    if (!changed.IsEmpty()) {
> +      InvalidateRectInSelf(changed.GetBounds());
> +    }

Changing the visible region should not in itself require invalidation.

@@ +762,2 @@
>      mTransform = aMatrix;
> +    InvalidateBoundsInParent();

Don't we need to avoid doing any invalidation if mTransform == aMatrix? Otherwise we'll be repainting everything all the time?

@@ +963,5 @@
>    void SetDebugColorIndex(PRUint32 aIndex) { mDebugColorIndex = aIndex; }
>    PRUint32 GetDebugColorIndex() { return mDebugColorIndex; }
>  #endif
>  
> +  nsIntRect GetInvalidRect();

const nsIntRect&

::: layout/base/FrameLayerBuilder.cpp
@@ +1954,5 @@
> +
> +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));

Would be better to have CheckLayerInvalidRegion simply take an nsIntRegion parameter, and combine its region into that parameter.

Though, wouldn't it make more sense here to just accumulate a single rectangle instead of a region?

@@ +1955,5 @@
> +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> +    }

You need to process the mask layer (if any) to collect its invalid region.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #173)
> ::: gfx/layers/ImageLayers.h
> @@ +593,5 @@
> > +    changed.Or(mVisibleRegion, aRegion);
> > +    if (!changed.IsEmpty()) {
> > +      InvalidateRectInSelf(changed.GetBounds());
> > +    }
> > +
> 
> As below, we shouldn't need to invalidate just for a visible region change.
> 
> Isn't this actually the same as the inherited method anyway?

It's slightly different, in that it's Or instead of Xor. I needed this to pass a test, but it could have been related to not catching the SetCurrentImage call on an image container.

Either that, or we're scaling the contained image to the size of the visible region, and then a visible region change changes every pixel of the image.

> 
> ::: gfx/layers/Layers.h
> @@ +653,5 @@
> > +    nsIntRegion changed;
> > +    changed.Xor(mVisibleRegion, aRegion);
> > +    if (!changed.IsEmpty()) {
> > +      InvalidateRectInSelf(changed.GetBounds());
> > +    }
> 
> Changing the visible region should not in itself require invalidation.

Why not? If a layer expands its visible region, then the result composited to the screen will be different.

> 
> @@ +762,2 @@
> >      mTransform = aMatrix;
> > +    InvalidateBoundsInParent();
> 
> Don't we need to avoid doing any invalidation if mTransform == aMatrix?
> Otherwise we'll be repainting everything all the time?

Good catch, lost that code when rewriting this.

> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1954,5 @@
> > +
> > +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> > +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> > +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> > +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> 
> Would be better to have CheckLayerInvalidRegion simply take an nsIntRegion
> parameter, and combine its region into that parameter.
> 
> Though, wouldn't it make more sense here to just accumulate a single
> rectangle instead of a region?

I was worried about breaking some tests that expect 'accurate' MozAfterPaint rects. This might not actually matter though.

> 
> @@ +1955,5 @@
> > +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> > +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> > +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> > +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> > +    }
> 
> You need to process the mask layer (if any) to collect its invalid region.

Will do.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #172)
> Comment on attachment 624288 [details] [diff] [review]
> Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice
> 
> Review of attachment 624288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1178,3 @@
> >  {
> > +  if (aItemVisible.IsEmpty()) {
> > +    aLayer->SetVisibleRegion(aChildBounds);
> 
> This doesn't seem right. Why do we need to do this?
> 
> If we really don't know what the visible rect is, instead of passing an
> empty rect, make aItemVisible a const nsIntRect* and pass null when we don't
> know. But why not just have the caller pass aChildBounds in that case?

Because the callers that need to know don't have aChildBounds either (callers of BuildLayer).

I'll switch BuildLayer to be an nsIntRect*, that should fix it.
Comment on attachment 624290 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v2

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

r+ with that fixed.

::: layout/base/FrameLayerBuilder.cpp
@@ +62,5 @@
>  using namespace mozilla::layers;
>  
>  namespace mozilla {
>  
> +nsTArray<FramePropertyDescriptor*> gPropertyList;

We're not supposed to have toplevel constructors/destructors. So I guess just heap-allocate the nsTArray and destroy it (and set gPropertyList to null) when the array is empty.

@@ +113,5 @@
> +/* static */ void
> +FrameLayerBuilder::DestroyDisplayItemDataFor(nsIFrame* aFrame)
> +{
> +  for (PRUint32 i = 0; i < gPropertyList.Length(); i++) {
> +    aFrame->Properties().Delete(gPropertyList.ElementAt(i));

move aFrame->Properties() out of the loop.

It would actually be more efficient to make gPropertyList a hashtable and iterate through aFrame's properties, deleting the ones that are in gPropertyList. As it stands, the more windows you have open the longer DestroyDisplayItemDataFor will take, which is bad.

But I think for now this is fine.
Attachment #624290 - Flags: review?(roc) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #174)
> Either that, or we're scaling the contained image to the size of the visible
> region, and then a visible region change changes every pixel of the image.

I don't think we're doing that. We shouldn't be doing that!

> > Changing the visible region should not in itself require invalidation.
> 
> Why not? If a layer expands its visible region, then the result composited
> to the screen will be different.

Yeah but the visible region is just for optimization, in theory it doesn't change what a layer actually renders.

> > Though, wouldn't it make more sense here to just accumulate a single
> > rectangle instead of a region?
> 
> I was worried about breaking some tests that expect 'accurate' MozAfterPaint
> rects. This might not actually matter though.

We can just fix or disable those tests.
Rewrote to use new LayerTreeInvalidation code instead.

I haven't changed invalidating via nsViewManager though, the coordinates go through multiple shifts in that code and it wasn't obvious how I could avoid that.

Might be better as a followup for someone who knows the view system better.
Attachment #624288 - Attachment is obsolete: true
Attachment #624289 - Attachment is obsolete: true
Attachment #624288 - Flags: review?(roc)
Attachment #624289 - Flags: review?(roc)
Attachment #625817 - Flags: review?(roc)
Comment on attachment 625817 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v4

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

I really like this approach.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "LayerTreeInvalidation.h"
> +#include "FrameLayerBuilder.h"

We really shouldn't depend on layout here. I think you should make NotifySubDocInvalidation a callback function that gets passed as a parameter to ComputeDifference.

@@ +34,5 @@
> +
> +static void 
> +NotifySubDocInvalidation(ContainerLayer* aLayer, const nsIntRegion& aRegion)
> +{
> +  ContainerLayerPresContext *pres = static_cast<ContainerLayerPresContext*>(aLayer->GetUserData(&gContainerLayerPresContext));

80 char limit

@@ +56,5 @@
> + * If any of these are a ContainerLayer that reports invalidations to a PresShell,
> + * then report that the entire bounds have changed.
> + */
> +static void
> +NotifyBoundsSubDocInvalidationInChildren(Layer* aLayer)

Yes, this is a terrible name. How about calling this NotifySubdocumentInvalidationRecursive and the callback aNotifySubdocumentInvalidation?

@@ +109,5 @@
> +  virtual nsIntRect ComputeDifferences(Layer* aRoot, PRUint32 aFlags);
> +
> +  nsIntRect ComputeDifference(Layer* aOther, PRUint32 aFlags)
> +  {
> +    NS_ASSERTION(aOther == mLayer, "Can't compare different layers!");

Why even pass aOther as a parameter then?

Take out the aOther parameter and call this ComputeChange.

@@ +115,5 @@
> +    Layer* otherMask = aOther->GetMaskLayer();
> +    nsIntRect* otherClip = aOther->GetClipRect();
> +    nsIntRect result;
> +    if ((!mMaskLayer && otherMask) ||
> +        (mMaskLayer && mMaskLayer->mLayer != otherMask) ||

I think you could just test (mMaskLayer ? mMaskLayer->mLayer : nsnull) != otherMask

@@ +139,5 @@
> +    result = result.Union(TransformRect(aOther->GetInvalidRect(), mTransform));
> +
> +    if (mMaskLayer && otherMask) {
> +      nsIntRect maskDiff = mMaskLayer->ComputeDifference(otherMask, aFlags);
> +      result = result.Union(TransformRect(maskDiff, mTransform));

I think you can save some code here by accumulating an invalid rectangle in pre-mTransform coordinates and then transforming it once.

@@ +144,5 @@
> +    }
> +
> +    if (mUseClipRect && otherClip) {
> +      if (mClipRect != *otherClip) {
> +        nsIntRect combined = mClipRect.Union(*otherClip);

It might be worth catching the cases where only one edge of the clip rectangle moves and so the difference between the old and new rectangles is still a single rectangle. You could just use nsIntRegion tmp; tmp.Xor(mClipRect, *otherClip); tmp.GetBounds().

@@ +153,5 @@
> +    aOther->ClearInvalidation();
> +    return result;
> +  }
> +
> +  virtual nsIntRect ComputeDifferenceInternal(Layer* aOther, PRUint32 aFlags) { return nsIntRect(); }

Remove the aOther parameter and call this ComputeChangeInternal

@@ +188,5 @@
> +        // swap order.
> +        result.Or(result, TransformRect(child->GetVisibleRegion().GetBounds(), child->GetTransform()));
> +        if (i < mChildren.Length()) {
> +          result.Or(result, TransformRect(mChildren[i]->mVisibleBounds, mChildren[i]->mTransform));
> +        }

As before, let's union the pre-transform rects and then transform the results.

@@ +196,5 @@
> +          ClearInvalidations(child);
> +        }
> +      } else {
> +        // Same child, check for differences within the child
> +        result.Or(result, mChildren[i]->ComputeDifference(child, aFlags));

This rect needs to be transformed, does it not?

Actually, I think we can be more accurate if we pass down a matrix that transforms

@@ +215,5 @@
> +
> +    return TransformRect(result.GetBounds(), aOther->GetTransform());
> +  }
> +
> +  nsTArray<nsAutoPtr<LayerPropertiesBase> > mChildren;

I think nsAutoTArray<1>

@@ +230,5 @@
> +  {
> +    ColorLayer* color = static_cast<ColorLayer*>(aOther);
> +
> +    if (mColor != color->GetColor()) {
> +      return TransformRect(aOther->GetVisibleRegion().GetBounds(), aOther->GetTransform());

Probably worth having a helper function in a base class that returns the transformed visible region

@@ +253,5 @@
> +    ImageLayer* image = static_cast<ImageLayer*>(aOther);
> +
> +    if (mContainer != image->GetContainer() ||
> +        mFilter != image->GetFilter()) {
> +      return TransformRect(aOther->GetVisibleRegion().GetBounds(), aOther->GetTransform());

You want to check mScaleMode and mScaleSize here too.

What about the current image changing in the container? We should track that, but it's tricky because the image could change asynchronously. I think we need an invalid flag on ImageContainer that you can use that gets set whenever the current image changes. Gotta handle RemoteImage too.

::: gfx/layers/LayerTreeInvalidation.h
@@ +36,5 @@
> +  /**
> +   * Compares a set of existing layer tree properties to the current layer
> +   * tree and generates the changed rectangle.
> +   *
> +   * @param aProperties Cached properties data.

This parameter does not exist

::: gfx/layers/Layers.h
@@ +945,5 @@
>  
> +  const nsIntRect&  GetInvalidRect() { return mInvalidRect; }
> +  void Invalidate() { mInvalidRect = GetVisibleRegion().GetBounds(); }
> +  void InvalidateRect(const nsIntRect& aRect) { mInvalidRect = mInvalidRect.Union(aRect); }
> +  void ClearInvalidation() { mInvalidRect.SetEmpty(); }

You need to document what the invalid rect means, and document each of these methods.

I think that instead of Invalidate/InvalidateRect we should call this SetInvalidRectToVisibleRegion and AddInvalidRect. Also ClearInvalidRect. This helps distinguish from ThebesLayer::InvalidateRegion.

::: layout/base/FrameLayerBuilder.cpp
@@ +2756,5 @@
>    }
>  
>    FlashPaint(aContext);
> +  if (!aRegionToInvalidate.IsEmpty()) {
> +    aLayer->Invalidate();

might as well invalidate the bounds of aRegionToInvalidate

::: layout/base/nsDisplayList.cpp
@@ +701,5 @@
> +  nsIntRect invalid;
> +  if (props) {
> +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> +  } else {
> +    LayerProperties::ClearInvalidations(root); 

Do we even need to clear these?

@@ +2149,5 @@
> +
> +  if (mGenerateInvalidations) {
> +    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
> +    pres->mPresContext = mFrame->PresContext();
> +    layer->SetUserData(&gContainerLayerPresContext, pres);

Where does this get cleared?

Why not just set the nsPresContext* as the data directly?

::: layout/base/nsDisplayList.h
@@ +2057,5 @@
>   */
>  class nsDisplayOwnLayer : public nsDisplayWrapList {
>  public:
>    nsDisplayOwnLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
> +                    nsDisplayList* aList, bool aGenerateInvalidations = false);

Boolean parameters suck. Use a flag.

@@ +2284,5 @@
>     */
>    nsDisplayZoom(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                  nsDisplayList* aList,
> +                PRInt32 aAPD, PRInt32 aParentAPD,
> +                bool aGenerateInvalidations = false);

Use a flag here too.

::: layout/base/nsPresContext.h
@@ +860,5 @@
>    // Returns true on success and false on failure (not safe).
>    bool EnsureSafeToHandOutCSSRules();
>  
>    void NotifyInvalidation(const nsRect& aRect, PRUint32 aFlags);
> +  void NotifyInvalidation(const nsIntRect& aRect, PRUint32 aFlags);

Document that this is in device pixels

@@ +1083,5 @@
>    void DoChangeCharSet(const nsCString& aCharSet);
>  
> +  bool MayHavePaintEventListener();
> +
> +  bool MayHavePaintEventListenerInSubDocument();

Document these
Attachment #618543 - Attachment is obsolete: true
Attachment #618543 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #179)

> @@ +139,5 @@
> > +    result = result.Union(TransformRect(aOther->GetInvalidRect(), mTransform));
> > +
> > +    if (mMaskLayer && otherMask) {
> > +      nsIntRect maskDiff = mMaskLayer->ComputeDifference(otherMask, aFlags);
> > +      result = result.Union(TransformRect(maskDiff, mTransform));
> 
> I think you can save some code here by accumulating an invalid rectangle in
> pre-mTransform coordinates and then transforming it once.

This isn't easy for the transformChanged case, where we have two rects and two transforms. We can't always return early in this path either, since we might need to walk the children to issue separate nsPresContext invalidations.

> 
> You want to check mScaleMode and mScaleSize here too.
> 
> What about the current image changing in the container? We should track
> that, but it's tricky because the image could change asynchronously. I think
> we need an invalid flag on ImageContainer that you can use that gets set
> whenever the current image changes. Gotta handle RemoteImage too.

I was relying on the user calling InvalidateLayer() here. Hopefully they'll be able to know the changed region (say, for an animated image), rather than just invalidating the bounds as Layers would have to.

> ::: layout/base/nsDisplayList.cpp
> @@ +701,5 @@
> > +  nsIntRect invalid;
> > +  if (props) {
> > +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> > +  } else {
> > +    LayerProperties::ClearInvalidations(root); 
> 
> Do we even need to clear these?

I believe so, otherwise any invalid rects set on layers would be included next time we have a paint listener.

> 
> @@ +2149,5 @@
> > +
> > +  if (mGenerateInvalidations) {
> > +    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
> > +    pres->mPresContext = mFrame->PresContext();
> > +    layer->SetUserData(&gContainerLayerPresContext, pres);
> 
> Where does this get cleared?
> 
> Why not just set the nsPresContext* as the data directly?

Layer user data has to be a subclass of LayerUserData.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #179)

> @@ +188,5 @@
> > +        // swap order.
> > +        result.Or(result, TransformRect(child->GetVisibleRegion().GetBounds(), child->GetTransform()));
> > +        if (i < mChildren.Length()) {
> > +          result.Or(result, TransformRect(mChildren[i]->mVisibleBounds, mChildren[i]->mTransform));
> > +        }
> 
> As before, let's union the pre-transform rects and then transform the
> results.
> 

These are different children, with different transforms.
(In reply to Matt Woodrow (:mattwoodrow) from comment #180)
> This isn't easy for the transformChanged case, where we have two rects and
> two transforms. We can't always return early in this path either, since we
> might need to walk the children to issue separate nsPresContext
> invalidations.

Fine, just do it where it's easy...

> > What about the current image changing in the container? We should track
> > that, but it's tricky because the image could change asynchronously. I think
> > we need an invalid flag on ImageContainer that you can use that gets set
> > whenever the current image changes. Gotta handle RemoteImage too.
> 
> I was relying on the user calling InvalidateLayer() here. Hopefully they'll
> be able to know the changed region (say, for an animated image), rather than
> just invalidating the bounds as Layers would have to.

OK, better document that somewhere.

> > ::: layout/base/nsDisplayList.cpp
> > @@ +701,5 @@
> > > +  nsIntRect invalid;
> > > +  if (props) {
> > > +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> > > +  } else {
> > > +    LayerProperties::ClearInvalidations(root); 
> > 
> > Do we even need to clear these?
> 
> I believe so, otherwise any invalid rects set on layers would be included
> next time we have a paint listener.

That's probably OK though.

> > Why not just set the nsPresContext* as the data directly?
> 
> Layer user data has to be a subclass of LayerUserData.

Oh right. Thanks.

(In reply to Matt Woodrow (:mattwoodrow) from comment #181)
> These are different children, with different transforms.

Oops, right.
Attachment #625817 - Attachment is obsolete: true
Attachment #625817 - Flags: review?(roc)
Attachment #625862 - Flags: review?(roc)
Attachment #618521 - Attachment is obsolete: true
Attachment #625866 - Flags: review?(roc)
Comment on attachment 625862 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v5

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +40,5 @@
> +    static_cast<ContainerLayerPresContext*>(aLayer->GetUserData(&gContainerLayerPresContext));
> +  if (pres) {
> +    nsIntPoint topleft = aLayer->GetVisibleRegion().GetBounds().TopLeft();
> +    pres->mCallback(pres->mPresContext, aRegion, topleft);
> +  }

I was hoping this entire function would become the callback so gfx/layers doesn't have to know about ContainerLayerPresContext at all.

@@ +255,5 @@
> +    ImageLayer* image = static_cast<ImageLayer*>(mLayer);
> +
> +    if (mContainer != image->GetContainer() ||
> +        mFilter != image->GetFilter()) {
> +      return NewTransformedBounds();

I think you missed the comment about mScaleMode/mScaleSize?

::: gfx/layers/Layers.h
@@ +918,5 @@
> +  /**
> +   * Returns the current area of the layer (in layer-space coordinates)
> +   * marked as needed to be recomposited.
> +   */
> +  const nsIntRect&  GetInvalidRect() { return mInvalidRect; }

Remove surplus space

::: layout/base/nsDisplayList.cpp
@@ +701,5 @@
> +  nsIntRect invalid;
> +  if (props) {
> +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> +  } else {
> +    LayerProperties::ClearInvalidations(root); 

I still think you shouldn't clear here. Delivering a whole bunch of invalidations to the paint listener the first time it's called seems OK.

::: layout/base/nsDisplayList.h
@@ +2057,5 @@
>   */
>  class nsDisplayOwnLayer : public nsDisplayWrapList {
>  public:
>    nsDisplayOwnLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
> +                    nsDisplayList* aList, PRUint32 aFlags = 0);

document possible flag values

@@ +2284,5 @@
>     */
>    nsDisplayZoom(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                  nsDisplayList* aList,
> +                PRInt32 aAPD, PRInt32 aParentAPD,
> +                PRUint32 aFlags = 0);

Document possible flag values

::: layout/base/nsPresContext.h
@@ +1083,5 @@
>  public:
>    void DoChangeCharSet(const nsCString& aCharSet);
>  
> +  /**
> +   * Checks for MozAfterPaint listeners on the document

... and any subdocuments, except for subdocuments that are non-top-level content documents.

::: layout/generic/nsFrame.cpp
@@ +4687,5 @@
>      return nsnull;
>    }
>  
> +  // TODO: Use aDamageRect instead (need to convert to pixels)
> +  layer->SetInvalidRectToVisibleRegion();

Just do it :-)
Comment on attachment 625867 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v3

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

::: gfx/layers/Layers.h
@@ +198,5 @@
>   */
>  
> +static void LayerManagerUserDataDestroy(void *data)
> +{
> +  delete (LayerUserData*)data;

static_cast

@@ +762,5 @@
>     */
>    nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
> +  { 
> +    nsAutoPtr<LayerUserData> d(static_cast<LayerUserData*>(mUserData.Remove(static_cast<gfx::UserDataKey*>(aKey)))); 
> +    return d; 

it's clearer to write d.forget()
Attachment #625867 - Flags: review?(roc) → review+
Comment on attachment 625868 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +551,5 @@
> +  FrameProperties props = aFrame->Properties();
> +  if (secondary) {
> +    return static_cast<LayerManagerData*>(props.Get(LayerManagerSecondaryDataProperty()));
> +  } else {
> +    return static_cast<LayerManagerData*>(props.Get(LayerManagerDataProperty()));

Declare a static helper function that returns the correct FramePropertyDescriptor* for the current layer manager. That will simplify code here.

@@ +2617,5 @@
>  FrameLayerBuilder::GetDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey)
>  {
> +  //TODO: This isn't completely correct, since a frame could exist as a layer
> +  // in the normal widget manager, and as a different layer (or no layer)
> +  // in the titlebar manager.

"in the secondary manager"

::: layout/base/FrameLayerBuilder.h
@@ +738,5 @@
>    PRUint32                            mMaxContainerLayerGeneration;
> +
> +  /**
> +   * True if the current top-level LayerManager for the widget being
> +   * painted if marked as being a 'secondary' LayerManager.

"is marked"
Comment on attachment 618534 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v2

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

I'll look for a new version of this based on previous changes.
Attachment #618534 - Flags: review?(roc) → review-
Comment on attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

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

::: layout/reftests/svg/reftest.list
@@ +229,5 @@
>  == svg-in-foreignObject-02.xhtml svg-in-foreignObject-01-ref.xhtml # reuse -01-ref.xhtml
>  == switch-01.svg pass.svg
>  == suspend-01.svg pass.svg
>  == suspend-02.svg pass.svg
> +random == suspend-03.svg pass.svg # bug 724242

I don't understand why this is failing or should be marked random.
Comment on attachment 618537 [details] [diff] [review]
Part 11: Reimplement empty transactions

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

::: layout/base/nsIPresShell.h
@@ +1216,5 @@
>     * Notify that the NS_DID_PAINT event was received. Only fires on the
>     * root pres shell.
>     */
>    virtual void DidPaint() = 0;
> +  virtual void ScheduleViewManagerFlush(bool aThebesChanges) = 0;

Use a flag instead of a bool, and document it

::: layout/generic/nsFrame.cpp
@@ +4633,5 @@
> +      aDisplayItemKey == nsDisplayItem::TYPE_PLUGIN ||
> +      aDisplayItemKey == nsDisplayItem::TYPE_CANVAS) {
> +    thebesChanges = false;
> +  }
> +  SchedulePaint(thebesChanges);

I'm not sure why we'd ever want to pass true here.

::: layout/generic/nsIFrame.h
@@ +2104,5 @@
>  
>    /**
>     * Schedule a view manager flush on the next refresh driver tick.
>     */
> +  void SchedulePaint(bool aThebesChanges = true);

Use a flag instead of a bool, and document it
I've been testing http://bubblemark.com/dhtml.htm with this patch queue, and found that we wasting bunch of time in Regions processing.
After some investigation I've found that after each ProcessPendingUpdates we have region in thebesLayer containing ~125 rectangles. and for example amount of nsRegion::InsertInPlace - 1.47817e+06 calls per second....
with current trunk we have only ~20000.
This patch practically restoring behavior with dlbi queue back to trunk and basically having minimal amount of rectangles for each paint, and amount of calls for that function gets back to 11-15k
Attachment #625914 - Flags: feedback?(matt.woodrow)
and possibly it make sense to optimize Thebes layer calculations in future to avoid so huge number of region operations...
Attachment #625862 - Attachment is obsolete: true
Attachment #625862 - Flags: review?(roc)
Attachment #626253 - Flags: review?(roc)
Attachment #618537 - Attachment is obsolete: true
Attachment #618537 - Flags: review?(roc)
Attachment #626255 - Flags: review?(roc)
Comment on attachment 626253 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v6

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +39,5 @@
> +                         NotifySubDocInvalidationFunc aCallback)
> +{
> +  void* data = aLayer->GetUserData(&gNotifySubDocInvalidationData);
> +  nsIntPoint topLeft = aLayer->GetVisibleRegion().GetBounds().TopLeft();
> +  aCallback(data, aRegion, topLeft);

Why not just pass aLayer to the callback function and let it get the user data and compute the visible region top-left?

::: gfx/layers/LayerTreeInvalidation.h
@@ +16,5 @@
> +/**
> + * Callback for ContainerLayer invalidations.
> + *
> + * @param aUserData UserData stored on the ContainerLayer
> + * @param aRegion Invalidated region in the ContainerLayers coordinate

ContainerLayer's

@@ +18,5 @@
> + *
> + * @param aUserData UserData stored on the ContainerLayer
> + * @param aRegion Invalidated region in the ContainerLayers coordinate
> + * space.
> + * @param aTopLeft TopLeft() of the ContainerLayers visible region bounds.

ContainerLayer's

::: layout/generic/nsFrame.cpp
@@ +4690,5 @@
> +  nsIntRect rect(PresContext()->AppUnitsToDevPixels(aDamageRect.x),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.y),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.width),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.height));
> +  layer->AddInvalidRect(rect);

Just use aDamageRect.ToOutsidePixels
Comment on attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3

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

::: layout/base/nsPresContext.cpp
@@ +2091,5 @@
> +  if (!IsChrome() && !mSendAfterPaintToContent) {
> +    // Don't tell the window about this event, it should not know that
> +    // something happened in a subdocument. Tell only the chrome event handler.
> +    // (Events sent to the window get propagated to the chrome event handler
> +    // automatically.)

We never send paint events to content now, right? So no need to have a mSendAfterPaintToContent variable.
Comment on attachment 626256 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v4

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

::: layout/base/FrameLayerBuilder.cpp
@@ +551,5 @@
> +  if (secondary) {
> +    return LayerManagerSecondaryDataProperty();
> +  } else {
> +    return LayerManagerDataProperty();
> +  }

return secondary ? LayerManagerSecondaryDataProperty() : LayerManagerDataProperty();
Attachment #626256 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #200)
> Comment on attachment 626254 [details] [diff] [review]
> Part 9g - Modify MozAfterPaint code to work with the new invalidation model
> v3
> 
> Review of attachment 626254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresContext.cpp
> @@ +2091,5 @@
> > +  if (!IsChrome() && !mSendAfterPaintToContent) {
> > +    // Don't tell the window about this event, it should not know that
> > +    // something happened in a subdocument. Tell only the chrome event handler.
> > +    // (Events sent to the window get propagated to the chrome event handler
> > +    // automatically.)
> 
> We never send paint events to content now, right? So no need to have a
> mSendAfterPaintToContent variable.

We do if privileges are enabled. It's just non-top-level content documents that never receive them any more.
Comment on attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3

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

::: layout/base/nsPresContext.cpp
@@ +175,5 @@
> +  if (!mInvalidateRequests.mRequests.IsEmpty()) {
> +    return true;    
> +  }
> +  if (GetRootPresContext()->mRefreshDriver->ViewManagerFlushIsPending()) {
> +    NotifyInvalidation(nsRect(0, 0, 0, 0), 0);

Actually ... why is this NotifyInvalidation necessary?

If that's necessary, then that means we send different mozafterpaint events depending on whether DOMWindowUtils::IsMozAfterPaintPending was called?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #203)
> Comment on attachment 626254 [details] [diff] [review]
> Part 9g - Modify MozAfterPaint code to work with the new invalidation model
> v3
> 
> Review of attachment 626254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresContext.cpp
> @@ +175,5 @@
> > +  if (!mInvalidateRequests.mRequests.IsEmpty()) {
> > +    return true;    
> > +  }
> > +  if (GetRootPresContext()->mRefreshDriver->ViewManagerFlushIsPending()) {
> > +    NotifyInvalidation(nsRect(0, 0, 0, 0), 0);
> 
> Actually ... why is this NotifyInvalidation necessary?
> 
> If that's necessary, then that means we send different mozafterpaint events
> depending on whether DOMWindowUtils::IsMozAfterPaintPending was called?

That is true. If we return true for IsMozAfterPaintPending, then we need to guarantee that we actually send one.

This just adds an empty rect, in case DLBI comes up with no actual changes. I guess we could change this to a flag of some sort, or strip the empty rect out if there are actual rects when we send the event.

I don't think it would overly matter?
OK. Please add a comment explaining why this is OK.
Comment on attachment 626269 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v7

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

::: gfx/layers/LayerTreeInvalidation.h
@@ +55,5 @@
> +   *
> +   * @param aRoot Root layer of the layer tree to compare against.
> +   * @param aFlags GENERATE_SUBDOC_INVALIDATIONS: For ContainerLayers with
> +   * ContainerLayerPresContext UserData, call the callback with the local
> +   * invalid region.

aFlags no longer exists. Also, move GENERATE_SUBDOC_INVALIDATIONS to nsDisplayOwnLayer.

::: layout/base/nsPresContext.h
@@ +152,5 @@
> +/**
> + * Layer UserData for ContainerLayers that want to be notified
> + * of local invalidations of them and their descendant layers.
> + * Pass GENERATE_SUBDOC_INVALIDATIONS to ComputeDifferences
> + * to have these called.

Fix comment.
Attachment #626269 - Flags: review?(roc) → review+
Attachment #625914 - Attachment is obsolete: true
Attachment #625914 - Flags: feedback?(matt.woodrow)
Depends on: 758505
Depends on: 761884
Attachment #618536 - Attachment is obsolete: true
Attachment #618536 - Flags: review?(roc)
Attachment #630825 - Flags: review?(roc)
Patch originally by romaxa
Attachment #626345 - Attachment is obsolete: true
Attachment #630831 - Flags: review?(roc)
We need this because the BasicLayers component alpha flattening can fail (with LAYER_ACTIVE_FORCE items), and we still end up with component alpha layers.
Attachment #630832 - Flags: review?(roc)
The part forcing the background color to white should probably be changed here, I couldn't find which theme was causing the current color (0xF3, 0xF3, 0xF3).

Tried editing a native theme files to no avail.
Attachment #630833 - Flags: review?(roc)
Attachment #626253 - Attachment is obsolete: true
Attachment #626253 - Flags: review?(roc)
Comment on attachment 630825 [details] [diff] [review]
Part 10: Test modifications required for DLBI v2

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

::: layout/base/tests/chrome/test_transformed_scrolling_repaints.html
@@ +30,5 @@
>        t.scrollTop = 10;
>        waitForAllPaintsFlushed(function () {
>          // Clear paint state now and scroll again.
>          utils.checkAndClearPaintedState(e);
> +        t.scrollTop = 15;

Why are you changing this?

::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
@@ +10,5 @@
>  <body onload="setTimeout(startTest,0)">
>  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> +  <div style="height:40px;"></div>
> +  <div id="e" style="height:30px; background:lime"></div>
> +  <div style="height:300px; background:yellow"></div>

And why are you changing this?

@@ +30,5 @@
>        t.scrollTop = 10;
>        waitForAllPaintsFlushed(function () {
>          // Clear paint state now and scroll again.
>          utils.checkAndClearPaintedState(e);
> +        t.scrollTop = 20;

And why are you changing this?

::: layout/reftests/svg/reftest.list
@@ +155,5 @@
>  == getElementById-a-element-01.svg pass.svg
> +== gradient-live-01a.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01b.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01c.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01d.svg gradient-live-01-ref.svg # bug 696674

You can remove the bug numbers now that these pass.
Comment on attachment 630829 [details] [diff] [review]
Part 18 - Mark frames with only invalid children as an optimization to use when invalidating further frames

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

I think you should do in nsFrame::Init what you do in SetParent. It might not matter but it's very efficient and more clearly correct. r+ with that.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #217)
> Comment on attachment 630825 [details] [diff] [review]
> Part 10: Test modifications required for DLBI v2
> 
> Review of attachment 630825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/tests/chrome/test_transformed_scrolling_repaints.html
> @@ +30,5 @@
> >        t.scrollTop = 10;
> >        waitForAllPaintsFlushed(function () {
> >          // Clear paint state now and scroll again.
> >          utils.checkAndClearPaintedState(e);
> > +        t.scrollTop = 15;
> 
> Why are you changing this?

We were getting invalidations when content moved out of sight, and the display item gets removed from the ThebesLayer. Then CheckAndClearPaintedState() returns true for other items in the same ThebesLayer because it doesn't cleck the redraw rect.

This just moves the scroll amount around a bit to avoid that.

> 
> ::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
> @@ +10,5 @@
> >  <body onload="setTimeout(startTest,0)">
> >  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> > +  <div style="height:40px;"></div>
> > +  <div id="e" style="height:30px; background:lime"></div>
> > +  <div style="height:300px; background:yellow"></div>
> 
> And why are you changing this?

We were hitting component alpha problems, so I removed the text. This shouldn't affect the purpose of the test.

> 
> @@ +30,5 @@
> >        t.scrollTop = 10;
> >        waitForAllPaintsFlushed(function () {
> >          // Clear paint state now and scroll again.
> >          utils.checkAndClearPaintedState(e);
> > +        t.scrollTop = 20;
> 
> And why are you changing this?

As above.
But maybe it would slow things down because you'd need to sweep the frame tree to clear those bits after first paint, or something like that?
Please do add a comment to NS_FRAME_ALL_DESCENDANTS_NEED_PAINT explaining the issue. The invariant is a bit complicated, it's actually "NS_FRAME_ALL_DESCENDANTS_NEED_PAINT means that all frame descendants either have NS_FRAME_NEEDS_PAINT and NS_FRAME_ALL_DESCENDANTS_NEED_PAINT, or they have no display items."
(In reply to Matt Woodrow (:mattwoodrow) from comment #219)
> We were getting invalidations when content moved out of sight, and the
> display item gets removed from the ThebesLayer. Then
> CheckAndClearPaintedState() returns true for other items in the same
> ThebesLayer because it doesn't cleck the redraw rect.
> 
> This just moves the scroll amount around a bit to avoid that.

It would be much better to increase the size of the scrollport or decrease the size of the scrolled content to avoid that issue. These tests depend on the offsets of the scrolled content rounding in certain ways.

> > ::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
> > @@ +10,5 @@
> > >  <body onload="setTimeout(startTest,0)">
> > >  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> > > +  <div style="height:40px;"></div>
> > > +  <div id="e" style="height:30px; background:lime"></div>
> > > +  <div style="height:300px; background:yellow"></div>
> > 
> > And why are you changing this?
> 
> We were hitting component alpha problems, so I removed the text. This
> shouldn't affect the purpose of the test.

OK yes.
Comment on attachment 630830 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window

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

This is pretty confusing, tracking different kinds of painting state on different views and on the widget. It would be great if we can simplify it, but I don't know how...

::: layout/base/nsRefreshDriver.cpp
@@ +415,5 @@
>      printf("Starting ProcessPendingUpdates\n");
>  #endif
>      mViewManagerFlushIsPending = false;
> +    bool skippedFlush = mPresContext->GetPresShell()->GetViewManager()->ProcessPendingUpdates();
> +    mViewManagerFlushIsPending = mViewManagerFlushIsPending || skippedFlush;

mViewManagerFlushIsPending |= skippedFlush

::: view/src/nsView.h
@@ +174,5 @@
>    void DoResetWidgetBounds(bool aMoveOnly, bool aInvalidateChangedSize);
>  
>    nsRegion*    mDirtyRegion;
> +  bool mPendingRefresh;
> +  bool mNeedsPaint;

Document the difference between these flags

::: view/src/nsViewManager.cpp
@@ +378,3 @@
>    for (nsView* childView = aView->GetFirstChild(); childView;
>         childView = childView->GetNextSibling()) {
> +    skipped = skipped || ProcessPendingUpdatesForView(childView, aFlushDirtyRegion);

skipped |=

@@ +384,5 @@
>    // damage is applied based on the final widget geometry
>    if (aFlushDirtyRegion) {
>      nsIWidget *widget = aView->GetWidget();
>      if (widget && widget->NeedsPaint()) {
> +      if (aView->PendingRefresh() && !aView->NeedsPaint()) {

Why do we need to check !aView->NeedsPaint() here?

::: view/src/nsViewManager.h
@@ +104,5 @@
>  
>    /* Update the cached RootViewManager pointer on this view manager. */
>    void InvalidateHierarchy();
>  
> +  virtual bool ProcessPendingUpdates();

Document the return value

@@ +113,5 @@
>  
>  private:
>  
>    void FlushPendingInvalidates();
> +  bool ProcessPendingUpdatesForView(nsView *aView,

And this one
Comment on attachment 630832 [details] [diff] [review]
Part 21 - BasicLayers should always retain content

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

Shouldn't we just be removing MustRetainContent since it's always true now?
Comment on attachment 630833 [details] [diff] [review]
Part 22 - Force a background color when running android reftests

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

nsWindow::DrawWindowUnderlay should check that WidgetPaintsBackground() is true before actually drawing. We don't want WidgetPaintsBackground() to return false and still be painting the underlay. r+ with that.
Attachment #630833 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #223)

> @@ +384,5 @@
> >    // damage is applied based on the final widget geometry
> >    if (aFlushDirtyRegion) {
> >      nsIWidget *widget = aView->GetWidget();
> >      if (widget && widget->NeedsPaint()) {
> > +      if (aView->PendingRefresh() && !aView->NeedsPaint()) {
> 
> Why do we need to check !aView->NeedsPaint() here?

Occasionally we don't actually get an OS paint event (for reasons unknown), and we end up waiting forever.

This just makes us skip a maximum of once before just painting again.

 
> It would be much better to increase the size of the scrollport or decrease
> the size of the scrolled content to avoid that issue. These tests depend on
> the offsets of the scrolled content rounding in certain ways.

The new numbers are adjusted correctly to account for the rounding.
Attachment #630832 - Attachment is obsolete: true
Attachment #630832 - Flags: review?(roc)
Attachment #631193 - Flags: review?(roc)
Attachment #630825 - Attachment is obsolete: true
Attachment #630825 - Flags: review?(roc)
Attachment #631197 - Flags: review?(roc)
Comment on attachment 631192 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window v2

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

r+ if you do that

::: view/src/nsView.h
@@ +165,5 @@
> +  void SetPendingRefresh(bool aPending) { mPendingRefresh = aPending; }
> +  bool PendingRefresh() { return mPendingRefresh; }
> +
> +  void SetNeedsPaint(bool aNeedsPaint) { mNeedsPaint = aNeedsPaint; }
> +  bool NeedsPaint() { return mNeedsPaint; }

Call this SetNeedsToTriggerPaintAfterRefresh, NeedsToTriggerPaintAfterRefresh.

@@ +181,5 @@
> +
> +  // True if a paint event was skipped while mPendingRefresh was
> +  // true, and a new one should be scheduled once we get
> +  // nsViewManager::Refresh called.
> +  bool mNeedsPaint;

Call this mNeedsToTriggerPaintAfterRefresh?
Attachment #631192 - Flags: review?(roc) → review+
Attachment #618522 - Flags: review?(jwatt) → review?(roc)
Attachment #618542 - Flags: review?(tnikkel) → review?(roc)
Attachment #618525 - Flags: review?(tnikkel) → review?(roc)
Comment on attachment 618525 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event v2

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

r+ anyway. It's probably a good idea to split this patch up into "layer changes" and "the rest" for landing.

::: gfx/layers/basic/BasicLayers.cpp
@@ +1538,5 @@
>  
>    mTransactionIncomplete = false;
>  
> +  if (aFlags & END_NO_COMPOSITE) {
> +    // TODO: We should really just set mTarget to null and make sure we can handle that further down the call chain

Is that really so hard?
Attachment #618525 - Flags: review?(roc) → review+
Comment on attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

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

::: widget/nsIWidget.h
@@ +1603,5 @@
>       */
>      virtual bool WidgetPaintsBackground() { return false; }
>  
> +    void SetNeedsPaint(bool aNeedsPaint) { mNeedsPaint = aNeedsPaint; }
> +    bool NeedsPaint() { return mNeedsPaint; }

You really need to document what this means and why it's needed. I don't understand myself. In particular, in what situations would ProcessPendingUpdates find that NeedsPaint is false?
ProcessPendingUpdates handles (potentially) multiple widgets, and the invalidations recorded might have been for only one of them.

This stops us from having to call nsPresShell::Paint unnecessarily. Possibly a premature optimization.
Comment on attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

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

Hopefully we don't need this.
Attachment #618542 - Flags: review?(roc) → review-
Attachment #631802 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6748b5496059
https://hg.mozilla.org/integration/mozilla-inbound/rev/9824458a41e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23c3a7e7ce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6079b229d306
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f29fcd1845
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec8ef3ebe48
https://hg.mozilla.org/integration/mozilla-inbound/rev/4859876972f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d43208825c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd288a38085
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adc41ff56dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/313af486e68d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c96b4fd4da
https://hg.mozilla.org/integration/mozilla-inbound/rev/783f298401b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f943b729ac14
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef00ebcd6c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b3d41e0360
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e07b09c426
https://hg.mozilla.org/integration/mozilla-inbound/rev/2865904db9fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a284ccb25b83
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa80bd9792
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2e9cf8fd41
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8121f8d3af
https://hg.mozilla.org/integration/mozilla-inbound/rev/61fd66629c4f
This appears to have caused multiple regressions:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/ss9mtWc0Q94

Regression :( Trace Malloc Allocs increase 4.74% on CentOS release 5 (Final) Mozilla-Inbound
--------------------------------------------------------------------------------------------
    Previous: avg 463925.033 stddev 517.186 of 30 runs up to revision 828da55601e7
    New     : avg 485894.800 stddev 300.113 of 5 runs since revision 61fd66629c4f
    Change  : +21969.767 (4.74% / z=42.479)
    Graph   : http://mzl.la/OfNiGG 


Talos Regression :( SVG increase 27.5% on Linux Mozilla-Inbound-Non-PGO
Regression :( SVG increase 27.5% on Linux Mozilla-Inbound-Non-PGO
-----------------------------------------------------------------
    Previous: avg 3795.778 stddev 4.853 of 30 runs up to revision 828da55601e7
    New     : avg 4838.982 stddev 14.399 of 5 runs since revision 61fd66629c4f
    Change  : +1043.204 (27.5% / z=214.944)
    Graph   : http://mzl.la/OfNvJS 


Talos Regression :( SVG, Opacity increase 7.4% on Linux Mozilla-Inbound-Non-PGO
Regression :( SVG, Opacity increase 7.4% on Linux Mozilla-Inbound-Non-PGO
-------------------------------------------------------------------------
    Previous: avg 82.683 stddev 1.163 of 30 runs up to revision 828da55601e7
    New     : avg 88.800 stddev 0.908 of 5 runs since revision 61fd66629c4f
    Change  : +6.117 (7.4% / z=5.258)
    Graph   : http://mzl.la/OfNupe 


Talos Regression :( Trace Malloc Allocs increase 4.67% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
Regression :( Trace Malloc Allocs increase 4.67% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
-----------------------------------------------------------------------------------------------------
    Previous: avg 455195.667 stddev 1174.193 of 30 runs up to revision 828da55601e7
    New     : avg 476457.600 stddev 1083.631 of 5 runs since revision 61fd66629c4f
    Change  : +21261.933 (4.67% / z=18.108)
    Graph   : http://mzl.la/OfNxRY 


Talos Regression :( Tp5 Row Major MozAfterPaint increase 10.4% on Linux Mozilla-Inbound-Non-PGO
Regression :( Tp5 Row Major MozAfterPaint increase 10.4% on Linux Mozilla-Inbound-Non-PGO
-----------------------------------------------------------------------------------------
    Previous: avg 281.249 stddev 2.599 of 30 runs up to revision 828da55601e7
    New     : avg 310.368 stddev 2.838 of 5 runs since revision 61fd66629c4f
    Change  : +29.118 (10.4% / z=11.204)
    Graph   : http://mzl.la/OfTwGw 

(and some more)
Ok, so this is pretty sad faces, but I've had to back this out for causing mochitest-4 permaorange (as well as the talos regressions in comment 237):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61fd66629c4f

eg https://tbpl.mozilla.org/php/getParsedLog.php?id=12544443&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08886a8cf22

It's for times like these that a relanding script would be pretty useful, for automating the qimport of a lange range of changesets (or at least it would make me feel less bad about having to back things like this out). 

Sorry! :-(
I think this also possibly broke several of the native Android talos tests.

eg see here and press the down arrow once:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f08886a8cf22&jobname=Android%20Tegra%20250%20mozilla-inbound%20talos
Note that there were also some perf wins:

*  Tp5 Row Major MozAfterPaint (%CPU) decrease 4.13% on XP Firefox
*  Paint decrease 8.7% on Linux Mozilla-Inbound-Non-PGO
*  DHTML 2 MozAfterPaint decrease 29% on XP Mozilla-Inbound-Non-PGO
Blocks: 730509
Depends on: 766059
Depends on: 768766
I have no idea how we passed all tests without this until this week.
Attachment #637345 - Flags: review?(roc)
Comment on attachment 637344 [details] [diff] [review]
Part 24 - Don't paint android widgets if they aren't at the front, or compositing is disabled

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

::: widget/android/nsWindow.cpp
@@ +2345,5 @@
> +
> +  if (FindTopLevel() == nsWindow::TopWindow()) {
> +    return nsIWidget::NeedsPaint();
> +  }
> +  return false;

Do it the other way around:
  if (sCompositorPaused || FindTopLevel() != nsWindow::TopWindow())
    return false;
  return nsIWidget::NeedsPaint();
Attachment #637344 - Flags: review?(roc) → review+
Comment on attachment 637472 [details] [diff] [review]
Part 26 - Send invalidations for hidden documents.

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

::: layout/base/nsPresShell.cpp
@@ +606,5 @@
> +}
> +
> +void
> +nsIPresShell::InvalidatePresShellIfHidden() { 
> +  if (!IsVisible()) {

{ on next line
Attachment #637472 - Flags: review?(roc) → review+
Comment on attachment 637473 [details] [diff] [review]
Part 27 - Make nsImageFrame's overflow include the AltFeedback image if it will use one

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

r+ with that.

::: layout/generic/nsImageFrame.cpp
@@ +876,5 @@
> +  if (!imageOK || !haveSize) {
> +    nsRect altFeedbackSize(0, 0,
> +                           2*(nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+ICON_PADDING+ALT_BORDER_WIDTH)),
> +                           2*(nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+ICON_PADDING+ALT_BORDER_WIDTH)));
> +    aMetrics.mOverflowAreas.UnionAllWith(altFeedbackSize);

Let's just add this to the visual overflow area. Changing the scroll overflow area is unnecessary and might conceivably break something. It would be a behavior change, at least.

Also, the area painted by DisplayAltFeedback is relative to the top-left of the content box, so you need to offset the area by aReflowState.mComputedBorderPadding.left/top.
Attachment #637473 - Flags: review?(roc) → review+
Note that I ran out of time to look into replacing this code with the simpler version that you suggested. Happy to do this as a followup.
Comment on attachment 637820 [details] [diff] [review]
Part 28 - Cached nsDisplayBackground rasterizations with BasicLayers

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

r+ with that, though I think we should do that simplification to just use a surface the size of the viewport.

::: layout/generic/nsCanvasFrame.cpp
@@ +233,5 @@
> +  if (surf) {
> +    BlitSurface(dest, mDestRect, surf);
> +
> +    surf.get()->AddRef();
> +    GetUnderlyingFrame()->Properties().Set(nsIFrame::CachedBackgroundImage(), surf.get());

surf.forget()? Then you don't need to addref.
Attachment #637820 - Flags: review?(roc) → review+
Blocks: 769541
Carrying forward r=jrmuizel from irc.
Attachment #638073 - Flags: review+
CC'ing mobile folks because of part 29 being particularly horrible.
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f35d1fea6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/75419010ac02
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef47dbb6313a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a41b53f8ada
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea5704272d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c411daf6633
https://hg.mozilla.org/integration/mozilla-inbound/rev/60affaedccc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97bd4246317
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0a91621ea9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f83491fc735a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f568fc280fb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04abde1b323
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba840bf34511
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5e9fefee19
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f3358435ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c75abcb72ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09fdc691c66
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b89bbdd7ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c553c4b10e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e794d5f88e0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1ac88bedc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4557011ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7021170544
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8e99e92344
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8c5c011767
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7599b247eef
https://hg.mozilla.org/integration/mozilla-inbound/rev/17cc480ae05d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b59fde84e97f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6234134d4430
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6d52bdf2d8

Note that we are still expecting some Talos regressions with this, notably SVG and scrolling (on non-accelerated layer managers). The SVG regression is because of changed paint timing, and means that we're actually measuring the right thing now. Scrolling is necessary to support both this, and OMTC in the future, not much we can do right now.
There was a perma-orange test crash in Windows mochitest-2, in indexedDB test_ipc.html. This had been incorrectly starred on our try-server pushes as a different intermittent bug.

That test is special because it exercises e10s on Windows with D3D10. In LayerManagerD3D10::Initialize, the call to HasShadowManager was returning false in the child process where it should have returned true. This is due to changeset bd0a91621ea9, which resolved an ambiguous name by calling LayerManager::HasShadowManager, which always returns false. Instead of doing that, LayerManagerD3D10 needs the same treatment as BasicLayerManager.

In the meantime, I've fixed the orange (I hope) by just calling ShadowLayerForwarder::HasShadowManager, which is what we want here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ae09d4f1d8
This is only a partial fix; other callers of layerManager->HasShadowManager() will still get false, which is wrong. Fortunately I think there aren't any callers for D3D10 right now. Needs to be fixed in followup though.

I also notice that while that test is running in D3D10, the remote IFRAME does not repaint unless I force it to. That also needs to be fixed in followup.

Other followup we discussed already:
-- Fix -moz-element invalidation
-- Simplify part 28 if possible
Also, there appears to be a new intermittent reftest failure in 482659-1*.html. So it looks like part 27 didn't fix the bug, or something like that.
Depends on: 769922
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #255)
> Also, there appears to be a new intermittent reftest failure in
> 482659-1*.html. So it looks like part 27 didn't fix the bug, or something
> like that.

Feel free to file it next time ;-)
Depends on: 769935
Depends on: 769942
Depends on: 769944
https://hg.mozilla.org/mozilla-central/rev/89f35d1fea6c
https://hg.mozilla.org/mozilla-central/rev/75419010ac02
https://hg.mozilla.org/mozilla-central/rev/ef47dbb6313a
https://hg.mozilla.org/mozilla-central/rev/3a41b53f8ada
https://hg.mozilla.org/mozilla-central/rev/eea5704272d0
https://hg.mozilla.org/mozilla-central/rev/2c411daf6633
https://hg.mozilla.org/mozilla-central/rev/60affaedccc3
https://hg.mozilla.org/mozilla-central/rev/d97bd4246317
https://hg.mozilla.org/mozilla-central/rev/bd0a91621ea9
https://hg.mozilla.org/mozilla-central/rev/f568fc280fb0
https://hg.mozilla.org/mozilla-central/rev/e04abde1b323
https://hg.mozilla.org/mozilla-central/rev/ba840bf34511
https://hg.mozilla.org/mozilla-central/rev/ce5e9fefee19
https://hg.mozilla.org/mozilla-central/rev/d9f3358435ba
https://hg.mozilla.org/mozilla-central/rev/0c75abcb72ff
https://hg.mozilla.org/mozilla-central/rev/f09fdc691c66
https://hg.mozilla.org/mozilla-central/rev/b7b89bbdd7ab
https://hg.mozilla.org/mozilla-central/rev/3c553c4b10e9
https://hg.mozilla.org/mozilla-central/rev/e794d5f88e0c
https://hg.mozilla.org/mozilla-central/rev/cb1ac88bedc2
https://hg.mozilla.org/mozilla-central/rev/ef4557011ad3
https://hg.mozilla.org/mozilla-central/rev/ba7021170544
https://hg.mozilla.org/mozilla-central/rev/3f8e99e92344
https://hg.mozilla.org/mozilla-central/rev/6e8c5c011767
https://hg.mozilla.org/mozilla-central/rev/f7599b247eef
https://hg.mozilla.org/mozilla-central/rev/17cc480ae05d
https://hg.mozilla.org/mozilla-central/rev/b59fde84e97f
https://hg.mozilla.org/mozilla-central/rev/6234134d4430
https://hg.mozilla.org/mozilla-central/rev/cd6d52bdf2d8
https://hg.mozilla.org/mozilla-central/rev/a9ae09d4f1d8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 769975
Depends on: 769992
Depends on: 770000
Depends on: 770011
Depends on: 770021
Depends on: 770041
Depends on: 770052
Depends on: 770056
Depends on: 770058
Depends on: 770061
Depends on: 770062
Depends on: 770069
Depends on: 770081
Depends on: 770083
Depends on: 770096
Depends on: 770106
Depends on: 770144
Depends on: 770086
Depends on: 770198
Depends on: 770201
Depends on: 594407
Blocks: 770349
Depends on: 770404
Depends on: 770263
Depends on: 770424
Depends on: 770526
(In reply to Matt Woodrow (:mattwoodrow) from comment #253)
> Note that we are still expecting some Talos regressions with this, notably
> SVG and scrolling (on non-accelerated layer managers). 

Thanks for the warning. There were very significant Talos regressions for mobile associated with part 29:

Regression Robocop Checkerboarding Benchmark increase 4.07e+03% on Android 2.2 (Native) Mozilla-Inbound
----------------------------------------------------------------------------------------------------------
    Previous: avg 102.631 stddev 38.410 of 30 runs up to revision 0d9f7fb55226
    New     : avg 4276.834 stddev 275.135 of 5 runs since revision cd6d52bdf2d8
    Change  : +4174.203 (4.07e+03% / z=108.674)
    Graph   : http://mzl.la/MxoQlV


Regression Robocop Pan Benchmark increase 81.4% on Android 2.2 (Native) Mozilla-Inbound
------------------------------------------------------------------------------------------
    Previous: avg 42155.963 stddev 3641.365 of 30 runs up to revision 0d9f7fb55226
    New     : avg 76490.320 stddev 8384.926 of 5 runs since revision cd6d52bdf2d8
    Change  : +34334.357 (81.4% / z=9.429)
    Graph   : http://mzl.la/Mxm2oM
There are two interesting cases here we should handle (in FrameLayerBuilder?), not sure if we are
 - platform disabled component alpha
 - platform wants component alpha, but is drawing with a basic layer manager and compositing with a GPU layer manager

Fennec and b2g fall into the first bucket, "desktop" omtc will fall into the second.
Depends on: 770572
Depends on: 770582
Depends on: 770639
Depends on: 770652
Depends on: 770629
Depends on: 770617
Depends on: 770686
Depends on: 770707
Backed out because of the regressions:

https://hg.mozilla.org/mozilla-central/rev/f56dac9de37c
https://hg.mozilla.org/mozilla-central/rev/48755ba6bb62
https://hg.mozilla.org/mozilla-central/rev/5f34f67d178d
https://hg.mozilla.org/mozilla-central/rev/6f0754272ae0
https://hg.mozilla.org/mozilla-central/rev/6add891f904f
https://hg.mozilla.org/mozilla-central/rev/e596d4c9db2e
https://hg.mozilla.org/mozilla-central/rev/461b05b02842
https://hg.mozilla.org/mozilla-central/rev/6bfde067f700
https://hg.mozilla.org/mozilla-central/rev/6baef7642626
https://hg.mozilla.org/mozilla-central/rev/d6c612f34054
https://hg.mozilla.org/mozilla-central/rev/5bdd6a4fe0fd
https://hg.mozilla.org/mozilla-central/rev/45acaaed2f94
https://hg.mozilla.org/mozilla-central/rev/cf71b613d9e7
https://hg.mozilla.org/mozilla-central/rev/19dcfb96c0d3
https://hg.mozilla.org/mozilla-central/rev/c86e39ee8c55
https://hg.mozilla.org/mozilla-central/rev/cdbbbea09aeb
https://hg.mozilla.org/mozilla-central/rev/02ba5ab5a3f9
https://hg.mozilla.org/mozilla-central/rev/cde7abd8c9dd
https://hg.mozilla.org/mozilla-central/rev/dd21bea4fb77
https://hg.mozilla.org/mozilla-central/rev/3b932903b7b2
https://hg.mozilla.org/mozilla-central/rev/9af1b99d622b
https://hg.mozilla.org/mozilla-central/rev/2d5604335e15
https://hg.mozilla.org/mozilla-central/rev/c22083ebc853
https://hg.mozilla.org/mozilla-central/rev/32e8fc01c01f
https://hg.mozilla.org/mozilla-central/rev/a49eedd187dd
https://hg.mozilla.org/mozilla-central/rev/3c2ce59947ab
https://hg.mozilla.org/mozilla-central/rev/e9117b081b9f
https://hg.mozilla.org/mozilla-central/rev/032329e0f4cc
https://hg.mozilla.org/mozilla-central/rev/ea8c4c02abf9
https://hg.mozilla.org/mozilla-central/rev/37ace49edeb3
https://hg.mozilla.org/mozilla-central/rev/fe89d83d71be
https://hg.mozilla.org/mozilla-central/rev/a6ee1952a230
https://hg.mozilla.org/mozilla-central/rev/87db9617a885
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 770524
Blocks: 770517
No longer depends on: 770263
Depends on: 770364
Depends on: 771407
Blocks: 771822
No longer blocks: 770349
Depends on: 770349
Depends on: 775179
No longer blocks: 769541
Blocks: 774508
Depends on: 776690
Depends on: 776695
Due to this landing, this page is completely broken : 
https://dl.dropbox.com/u/28610/bookmarks/index.html

Correct me if I am wrong and this breakage is not due to this bug.
This was backed out, so it's probably not your bug.
I think it might be a regression from 741682, could you file a bug please.

A reduced test case would be really helpful too if possible.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #268)
> This was backed out, so it's probably not your bug.

I searched all the changesets after landing of this bug (https://hg.mozilla.org/mozilla-central/rev/7a376ff3ae84) and could not find a backout of this bug, can you pls provide a link of the backouts ?
Filed bug 777617 for the issue.
Depends on: 777924
Probably more complexity than needed for ContainerLayerD3D10, but it makes it the same as all the other layer types which doesn't hurt.
Attachment #646797 - Flags: review?(roc)
Comment on attachment 646797 [details] [diff] [review]
Part 30 - Make ShadowContainerLayerD3D10 hold references to child layers.

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

::: gfx/layers/d3d10/ContainerLayerD3D10.cpp
@@ +50,5 @@
> +    for (Layer *child = aContainer->GetFirstChild(); 
> +        child; child = child->GetNextSibling()) {
> +            if (aAfter == child) {
> +                Layer *oldNextSibling = child->GetNextSibling();
> +                child->SetNextSibling(aChild);

Indenting here is messed up

@@ +88,5 @@
> +    Layer *lastChild = nsnull;
> +    for (Layer *child = aContainer->GetFirstChild(); child; 
> +        child = child->GetNextSibling()) {
> +            if (child == aChild) {
> +                // We're sure this is not our first child. So lastChild != NULL.

Here too
Attachment #646797 - Flags: review?(roc) → review+
Blocks: 779269
Depends on: 780385
Depends on: 780728
Blocks: 626963
Depends on: 781362
Target Milestone: mozilla16 → ---
Depends on: 782660
No longer depends on: 782660
No longer blocks: 614732
Depends on: 783126
No longer depends on: 783363
Depends on: 565245
Blocks: 784068
No longer blocks: 784068
https://tbpl.mozilla.org/?tree=Try&rev=52a30e616f8b

Try builds with the remaining patches, fully green.

Anyone interesting in trying these out before I attempt to land again?

I think I've addressed the majority of the blocking bugs, haven't checked them all individually though.
Keywords: qawanted
I'm taking them one by one, started out with MazeSolver and noticed the perfect invalidation on the default 30x30 maze, but large areas are being invalidated on both the 20x20 and 40x40 sized mazes.
Also different input events seem to treat the screen differently. If i drag the window scrollbar to the top or bottom of any page larger than it's window, it will only redraw new parts of the page showing up, but if i use my mouse wheel, it will redraw the entire page when i hit the top or bottom.
Full page invalidation is also happening in about:config, by hovering between different entries. Moving the selected row "cursor" with arrow keys does the same.
Thanks for taking the time to check these!

MazeSolver - is this different to what we get with trunk? I can definitely reproduce the issue, but it's because the nsTextFrame being added is really big, DLBI can't really fix that.

Maybe we can more intelligently detect which areas of the overflow actually get pixels drawn. That's a bug with nsTextFrame::GetBounds()/GetComponentAlphaBounds() though, and would fix the over-invalidation with DLBI in the process.

roc/bz - any ideas?


Scrolling - again, is this new? I think that's the expected behaviour when the page isn't an integer number of pixels long.

about:config doesn't surprise me, I didn't spend all that much time getting XUL invalidation minimized, will have another look at that code.
> but it's because the nsTextFrame being added is really big

There's a textframe being added?
Sorry, the textframe being added is the timer changing.

Over-invalidation in the maze area looks like a real bug, working on it.
Happy to help, i've got more!

Keyboard navigating the links on this page, invalidates the entire container. I havent looked at the code, but there seems to be a pattern of invalidating the changing elements parent or event parents parent in certain cases (links inside tables and lists atleast).

Otherwise, most of the form input issues seem to be resolved. Can't confirm if any of the crashes have gone away, i havent had any.

Not sure if this is really an issue, but having paint flashing on also flashes the entire line when i'm typing here :)
(In reply to Dennis Jakobsen from comment #282)
> http://ie.microsoft.com/testdrive/Performance/Bubbles/ is doing full page
> invalidation.

This is because the page is using top/left to animate the images instead of using transforms. This isn't recommended because it's a lot harder to optimize.

The whole page invalidation is because we try keep the invalidation areas relatively simple and extend them outwards if we get too many rects. 1 rect for each image definitely hits this.

A quick hack to mimic using transforms to animate the motion gives me no invalidation apart from the frame rate counter though.
(In reply to Matt Woodrow (:mattwoodrow) from comment #287)
> Sorry, the textframe being added is the timer changing.
> 
> Over-invalidation in the maze area looks like a real bug, working on it.

It was, we were invalidating two rects, one with scaling applied and one without. And then extending the two rects out into a single rect that covered both. :(

Fixed now
(In reply to Dennis Jakobsen from comment #288)
> Keyboard navigating the links on this page, invalidates the entire
> container. I havent looked at the code, but there seems to be a pattern of
> invalidating the changing elements parent or event parents parent in certain
> cases (links inside tables and lists atleast).

How are you doing this exactly? Using tab only navigates between text boxes.

Latest patch queue is available at: http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi

Based on m-c revision 103014:5650196a8c7d. Apply up to patch 28 - svg-observers
Blocks: 786476
(In reply to Matt Woodrow (:mattwoodrow) from comment #291)
> (In reply to Dennis Jakobsen from comment #288)
> > Keyboard navigating the links on this page, invalidates the entire
> > container. I havent looked at the code, but there seems to be a pattern of
> > invalidating the changing elements parent or event parents parent in certain
> > cases (links inside tables and lists atleast).
> 
> How are you doing this exactly? Using tab only navigates between text boxes.

If i click in the empty space between the links and start tabbing, i get 1px dashed borders around links.
 
> Latest patch queue is available at:
> http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi
> 
> Based on m-c revision 103014:5650196a8c7d. Apply up to patch 28 -
> svg-observers

I'll give it a shot and report my findings.
On Mac (unlike Windows/Linux), tabbing by default goes to text boxes only.  

You can open System Preferences > Keyboard > Keyboard Shortcuts and change the radio button near the bottom to "All controls" on Mac to change the behavior.
Depends on: 786904
Depends on: 786970
Depends on: 788044
Depends on: 789862
No longer blocks: 786476
Depends on: 782505
Depends on: 792314
Blocks: 792413
Depends on: 793501
This has had fairly significant changes since it was last reviewed. In particular, handling multiple DisplayItemData's for a single display item when it has merged frames.
Attachment #624287 - Attachment is obsolete: true
Attachment #664708 - Flags: review?(roc)
This makes me all sorts of sad, but it's much less disruptive than trying to get each frame to paint it's own content. I'd love to see that done at some point though.
Attachment #664710 - Flags: review?(roc)
Another example of us not using per-frame display items and it messing with the assumptions DLBI makes.
Attachment #664712 - Flags: review?(roc)
Attachment #664715 - Flags: review?(roc)
Comment on attachment 664708 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v5

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

::: layout/base/FrameLayerBuilder.cpp
@@ +613,5 @@
>  }
>  
>  } // anonymous namespace
>  
> +uint8_t gContainerLayerPresContext;

static?

@@ +763,5 @@
> +      ThebesDisplayItemLayerUserData* data =
> +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> +      if (data) {
> +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);

do we have to have mAppUnitsPerDevPixel in mGeometry? Or can we have it in ThebesDisplayItemLayerUserData? The latter sounds cheaper.

@@ +764,5 @@
> +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> +      if (data) {
> +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> +        rgn.MoveBy(-entry->mData[i]->mGeometry->mPaintOffset);

Ditto, can mPaintOffset be in ThebesDisplayItemLayerUserData?

@@ +848,5 @@
> +  FrameProperties props = f->Properties();
> +  DisplayItemDataEntry* newDisplayItems =
> +    builder ? builder->mNewDisplayItemData.GetEntry(f) : nullptr;
> +
> +  // 

remove comment

@@ +922,5 @@
> +  // When a frame has multiple layer managers (main, inactive, svg), we
> +  // only need to store the outermost one since that will be enough to
> +  // invalidate the entire region covered by all the children.
> +  props.Remove(LayerManagerDataProperty());
> +  props.Set(LayerManagerDataProperty(), data);

This Remove call should be redundant; remove it.

@@ +2249,5 @@
> +    BasicLayerManager* basic = static_cast<BasicLayerManager*>(mInactiveLayer.get());
> +    if (basic->InTransaction()) {
> +      basic->EndTransaction(nullptr, nullptr);
> +    }
> +    basic->SetUserData(&gLayerManagerLayerBuilder, nullptr);

Why is this here? Shouldn't it be in the DrawThebesLayer drawing loop, so we end the transaction on one inactive item before we start drawing the next one?

::: layout/base/FrameLayerBuilder.h
@@ +125,5 @@
>     * Call this just before we end a transaction on aManager. If aManager
>     * is not the retained layer manager then it must be a temporary layer
>     * manager that will not be used again.
>     */
> +  void WillEndTransaction();

Fix comment since aManager is gone. same below.

Do you have a comment update somewhere that says FrameLayerBuilder is one per layer manager now? If not, please add one.

::: layout/base/nsDisplayListInvalidation.h
@@ +50,5 @@
>     * this display item was drawn into.
>     */
>    nsIntPoint mPaintOffset;
> +
> +  nsPoint mActiveScrolledRootOffset;

Document this
Comment on attachment 664709 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds

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

::: layout/base/nsDisplayList.h
@@ +2024,2 @@
>      }
> +    // XXX: Is ToReferenceFrame() the same for all frames here?

Yes.

::: layout/generic/nsIFrame.h
@@ +2187,5 @@
>    virtual void InvalidateFrame();
> +
> +  /**
> +   * Same as InvalidateFrame(), but only mark a fixed rect as needing
> +   * repainting.

Add that aRect is relative to the top-left of the frame's border-box.

@@ +2206,5 @@
>     * last paint.
> +   *
> +   * If true, then the invalid rect is returned in aRect, with an
> +   * empty rect meaning all pixels drawn by this frame should be
> +   * invalidated.

How about using GetMaxSizedIntRect to mean all pixels.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #301)
> Comment on attachment 664708 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation v5
> 
> Review of attachment 664708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +613,5 @@
> >  }
> >  
> >  } // anonymous namespace
> >  
> > +uint8_t gContainerLayerPresContext;
> 
> static?

No idea what this is even doing here, removed.

> 
> @@ +763,5 @@
> > +      ThebesDisplayItemLayerUserData* data =
> > +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> > +      if (data) {
> > +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> > +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> 
> do we have to have mAppUnitsPerDevPixel in mGeometry? Or can we have it in
> ThebesDisplayItemLayerUserData? The latter sounds cheaper.
> 
> @@ +764,5 @@
> > +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> > +      if (data) {
> > +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> > +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> > +        rgn.MoveBy(-entry->mData[i]->mGeometry->mPaintOffset);
> 
> Ditto, can mPaintOffset be in ThebesDisplayItemLayerUserData?

Will have a look at these, we need to be sure that they won't have been updated to the new values.


> 
> @@ +922,5 @@
> > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > +  // only need to store the outermost one since that will be enough to
> > +  // invalidate the entire region covered by all the children.
> > +  props.Remove(LayerManagerDataProperty());
> > +  props.Set(LayerManagerDataProperty(), data);
> 
> This Remove call should be redundant; remove it.

It isn't, comment above explains.

> 
> @@ +2249,5 @@
> > +    BasicLayerManager* basic = static_cast<BasicLayerManager*>(mInactiveLayer.get());
> > +    if (basic->InTransaction()) {
> > +      basic->EndTransaction(nullptr, nullptr);
> > +    }
> > +    basic->SetUserData(&gLayerManagerLayerBuilder, nullptr);
> 
> Why is this here? Shouldn't it be in the DrawThebesLayer drawing loop, so we
> end the transaction on one inactive item before we start drawing the next
> one?

Another thing that I probably should have written down at the time. We can't tell exactly which items will actually get processed (in DrawThebesLayer) when we create the ClippedDisplayItems (and start the transaction). If for some reason we didn't call EndTransaction on this item from DrawThebesLayer, then this just ends the transaction to avoid assertions.

I don't remember the exact circumstances that caused this, but I concluded that it was valid behaviour and not a bug.
Comment on attachment 664710 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation

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

::: layout/base/nsFrameManager.cpp
@@ +480,5 @@
> +  // This has to sure to invalidate the entire overflow rect; this
> +  // is important in the presence of absolute positioning
> +  if (aInvalidate) {
> +    aOldFrame->InvalidateFrameSubtree();
> +  }

I don't understand why we need this here. Normally DLBI will invalidate for the missing frame.

::: layout/base/nsFrameManager.h
@@ +95,5 @@
>                                      nsFrameList&    aFrameList);
>  
>    NS_HIDDEN_(nsresult) RemoveFrame(ChildListID     aListID,
> +                                   nsIFrame*       aOldFrame,
> +                                   bool            aInvalidate = true);

change this to a flags word and a FLAG_INVALIDATE flag

::: layout/generic/nsFrame.cpp
@@ +4818,5 @@
>  void
>  nsIFrame::InvalidateFrameWithRect(const nsRect& aRect)
>  {
>    if (!HasAnyStateBits(NS_FRAME_NEEDS_PAINT)) {
> +    InvalidateFrameInternal(this);

Can't we just return here? No point in setting the rect property if we've invalidated the entire frame.

::: layout/generic/nsPlaceholderFrame.cpp
@@ +134,5 @@
>  {
>    nsIPresShell* shell = PresContext()->GetPresShell();
>    nsIFrame* oof = mOutOfFlowFrame;
>    if (oof) {
> +    oof->InvalidateFrameSubtree();

Why is this necessary?

::: layout/tables/nsTableCellFrame.cpp
@@ +416,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  if (!FrameLayerBuilder::HasRetainedDataFor(
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {

What's the purpose of these checks?

Wouldn't it be fine to just unconditionally invalidate the parent?

I especially don't know why you're checking for TYPE_TABLE_CELL_BACKGROUND here.

@@ +418,5 @@
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {

I think we can assert the parent of a cell is never null!

@@ +419,5 @@
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrameWithRect(aRect + GetPosition());

When the cell has a filter, the damage rect might need to be increased. Except that I guess it doesn't matter because the filter induces its own inactive layer tree and DLBI handles the invalidation that way. But you might want to comment here (and elsewhere, e.g. nsTableRowFrame) that that's what happens.

@@ +977,5 @@
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {
> +    InvalidateFrame();

This is a weird comparison, if the visual overflow rect top-left is not 0,0. Maybe you should compare against mRect.Size() instead?

::: layout/tables/nsTableColFrame.cpp
@@ +206,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  nsIFrame *parent = GetParent();
> +  if (parent) {
> +    parent->InvalidateFrameWithRect(aRect + GetPosition());
> +  }

As above

::: layout/tables/nsTableColGroupFrame.cpp
@@ +480,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  nsIFrame *parent = GetParent();
> +  if (parent) {
> +    parent->InvalidateFrameWithRect(aRect + GetPosition());
> +  }

As above.

::: layout/tables/nsTableFrame.cpp
@@ +1806,5 @@
>    }
>    aDesiredSize.mOverflowAreas.UnionAllWith(tableRect);
>  
> +  if (GetStateBits() & NS_FRAME_FIRST_REFLOW ||
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

as above

@@ +7249,5 @@
> +  }
> +
> +  // The part that looks at both the rect and the overflow rect is a
> +  // bit of a hack.  See nsBlockFrame::ReflowLine for an eloquent
> +  // description of its hackishness.

Why do we still need to do this?

I would have thought that we wouldn't need to do this anymore.

Can we just check for the border-box top-left change and not check for visual overflow top-left changes?

@@ +7261,5 @@
> +    // aFrame's parent, and reposition that overflow rect to the right
> +    // place.
> +    // XXXbz this doesn't handle outlines, does it?
> +    aFrame->InvalidateFrame();
> +    parent->InvalidateFrameWithRect(aOrigVisualOverflow + aOrigRect.TopLeft());

I wonder if we can avoid passing aOrigVisualOverflow in here. It seems to me that any changes in the visual overflow will have been separately invalidated. So we can just invalidate the new overflow at the old position and new position.

@@ +7265,5 @@
> +    parent->InvalidateFrameWithRect(aOrigVisualOverflow + aOrigRect.TopLeft());
> +  } else {
> +    aFrame->InvalidateFrameWithRect(aOrigVisualOverflow);;
> +    aFrame->InvalidateFrame();
> +    parent->InvalidateFrameWithRect(aOrigRect);;

Remove extra ;

Why are we invalidating in this case? For a potential size change? Due to box-shadow that might affect things in our visual overflow area so we should invalidate our visual overflow area.

::: layout/tables/nsTableFrame.h
@@ +467,5 @@
> +  /**
> +   * To be called on a frame by its parent after setting its size/position and
> +   * calling DidReflow (possibly via FinishReflowChild()).  This can also be
> +   * used for child frames which are not being reflowed but did have their size
> +   * or position changed.

I'd like a better understanding of exactly what this function invalidates.

I think maybe it should simply check whether aOrigRect is equal to the new rect. If it is, and aIsFirstReflow is false, then do nothing, otherwise invalidate the new visual overflow area at the old and new positions.

::: layout/tables/nsTableRowFrame.cpp
@@ +1028,5 @@
>  
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

same comment as above

@@ +1391,5 @@
> +         nsDisplayItem::TYPE_TABLE_ROW_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrame();

Why not call parent->InvalidateFrameWithRect?

::: layout/tables/nsTableRowGroupFrame.cpp
@@ +1313,5 @@
>  
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

as above

@@ +1881,5 @@
> +         nsDisplayItem::TYPE_TABLE_ROW_GROUP_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrame();

why not call parent->InvalidateFrameWithRect()?
(In reply to Matt Woodrow (:mattwoodrow) from comment #303)
> > @@ +922,5 @@
> > > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > > +  // only need to store the outermost one since that will be enough to
> > > +  // invalidate the entire region covered by all the children.
> > > +  props.Remove(LayerManagerDataProperty());
> > > +  props.Set(LayerManagerDataProperty(), data);
> > 
> > This Remove call should be redundant; remove it.
> 
> It isn't, comment above explains.

I still don't get it. props.Remove(X); props.Set(X,Y) should be equivalent to props.Set(X,Y), always.

> Another thing that I probably should have written down at the time. We can't
> tell exactly which items will actually get processed (in DrawThebesLayer)
> when we create the ClippedDisplayItems (and start the transaction). If for
> some reason we didn't call EndTransaction on this item from DrawThebesLayer,
> then this just ends the transaction to avoid assertions.

OK, put in a comment about that.
Comment on attachment 664715 [details] [diff] [review]
Avoid some causes of unnecessary painting

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

::: layout/generic/nsFrame.cpp
@@ +4651,5 @@
> +
> +  if (aObject->mChangeHint & nsChangeHint_UpdateTransformLayer &&
> +      FrameLayerBuilder::GetDedicatedLayer(f, nsDisplayItem::TYPE_TRANSFORM)) {
> +    f->InvalidateFrameSubtree();
> +  }

Put () around & subexpression and use } else if { (or make a giant boolean expression)

Would it be better to call HasRetainedDataFor? Slightly simpler...

Although, isn't this going to break cases where we have a rendering observer ancestor?

::: layout/generic/nsImageFrame.cpp
@@ +593,5 @@
>           aRect->x, aRect->y, aRect->width, aRect->height);
>  #endif
> +  if (!FrameLayerBuilder::HasRetainedDataFor(this, nsDisplayItem::TYPE_IMAGE) &&
> +      !FrameLayerBuilder::HasRetainedDataFor(this, nsDisplayItem::TYPE_ALT_FEEDBACK)) {
> +    return NS_OK;

Isn't this going to break cases where we have a rendering observer ancestor?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #306)
> Isn't this going to break cases where we have a rendering observer ancestor?

How about we add a flag to InvalidateFrame that is set when we have retained data for the frame that needs to be invalidated? When that flag is not set, InvalidateInternal would still walk up through the frame's ancestors and call nsSVGEffects::InvalidateDirectRenderingObservers on them. We could still stop when we reach NS_FRAME_DESCENDANT_NEEDS_PAINT. But we wouldn't add NS_FRAME_NEEDS_PAINT or NS_FRAME_DESCENDANT_NEEDS_PAINT and we wouldn't call SchedulePaint etc.

I wonder if we could pass in the display item type(s) to InvalidateFrame and have it call HasRetainedDataFor and do the right things.
or we could spray ANCESTOR_HAS_RENDERING_OBSERVER frame state bits everywhere.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #305)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #303)
> > > @@ +922,5 @@
> > > > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > > > +  // only need to store the outermost one since that will be enough to
> > > > +  // invalidate the entire region covered by all the children.
> > > > +  props.Remove(LayerManagerDataProperty());
> > > > +  props.Set(LayerManagerDataProperty(), data);
> > > 
> > > This Remove call should be redundant; remove it.
> > 
> > It isn't, comment above explains.
> 
> I still don't get it. props.Remove(X); props.Set(X,Y) should be equivalent
> to props.Set(X,Y), always.

Props.Delete(X); props.Set(X,Y); would be equivalent. props.Remove(X) returns the pointer without invoking the destructor, which is what we want here.

> 
> > Another thing that I probably should have written down at the time. We can't
> > tell exactly which items will actually get processed (in DrawThebesLayer)
> > when we create the ClippedDisplayItems (and start the transaction). If for
> > some reason we didn't call EndTransaction on this item from DrawThebesLayer,
> > then this just ends the transaction to avoid assertions.
> 
> OK, put in a comment about that.

Sure.
Comment on attachment 664711 [details] [diff] [review]
Correctly invalidate SVG observers

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

I think you need to InvalidateDirectRenderingObservers in nsFrame::DidReflow as well.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #304)
> Comment on attachment 664710 [details] [diff] [review]
> Make the table code use rect invalidation to avoid over invalidation
> 
> Review of attachment 664710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsFrameManager.cpp
> @@ +480,5 @@
> > +  // This has to sure to invalidate the entire overflow rect; this
> > +  // is important in the presence of absolute positioning
> > +  if (aInvalidate) {
> > +    aOldFrame->InvalidateFrameSubtree();
> > +  }
> 
> I don't understand why we need this here. Normally DLBI will invalidate for
> the missing frame.
> 
This, and the other related frame manager etc changes are just reverting changes from earlier in the patch queue.

This covers the case where we remove a child frame from a table. Since the child might not have any display items, DLBI wouldn't detect it and nothing would be invalidated. But we do need to invalidate the table background display item created by the outer table frame.
(In reply to Matt Woodrow (:mattwoodrow) from comment #311)
> This covers the case where we remove a child frame from a table. Since the
> child might not have any display items, DLBI wouldn't detect it and nothing
> would be invalidated. But we do need to invalidate the table background
> display item created by the outer table frame.

OK then, can we have an nsIFrame::InvalidateFrameForRemoval() method that does nothing for most frames but calls InvalidateFrameSubtree for table parts?
Blocks: 793998
Comment on attachment 664710 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation

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

::: layout/tables/nsTableCellFrame.cpp
@@ +416,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  if (!FrameLayerBuilder::HasRetainedDataFor(
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {

I only wanted to walk up to the frame that renders the background for this frame. As soon as we hit a transformed frame (guaranteed to be a stacking context, and thus renders the background for all descendants), or a frame that drew a background, we know we're done.

This might just be a waste of time and it would be easier to just walk to the root of the table regardless.
(In reply to Matt Woodrow (:mattwoodrow) from comment #313)
> This might just be a waste of time and it would be easier to just walk to
> the root of the table regardless.

Yes, I think so.
This test needs to wait until paint unsuppression has happened. As written, it can run its tests during the onload event handler, when the content might not be visible so calls to selectAtPoint won't find anything to select.

Alternatively, we could make selectAtPoint ignore paint suppression, but I don't think that's the right thing to do. We won't want users touching the screen and selecting invisible content during paint suppression.
Attachment #665217 - Flags: review?(matt.woodrow)
This fixes the issues with pastebin I was seeing.

I also included some comment fixes and removed some dead code the compiler was complaining about.
Attachment #665217 - Flags: review?(matt.woodrow) → review+
Attachment #664708 - Attachment is obsolete: true
Attachment #664708 - Flags: review?(roc)
Attachment #665308 - Flags: review?(roc)
Attachment #664709 - Attachment is obsolete: true
Attachment #664709 - Flags: review?(roc)
Attachment #665309 - Flags: review?(roc)
Attachment #665312 - Flags: review?(roc)
Attachment #664710 - Attachment is obsolete: true
Attachment #664710 - Flags: review?(roc)
Attachment #664715 - Attachment is obsolete: true
Attachment #664715 - Flags: review?(roc)
Attachment #665313 - Flags: review?(roc)
Rebased against my other changes. r=me
Attachment #665315 - Flags: review?
Attachment #665315 - Flags: review? → review+
Comment on attachment 665308 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v6

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

::: layout/base/FrameLayerBuilder.cpp
@@ +498,5 @@
>     */
>    float mXScale, mYScale;
>  
>    /**
> +   * The appunits per dev pixel for the item in this layer.

for the items in this layer

@@ +525,5 @@
>  
> +  nsIntRegion mRegionToInvalidate;
> +
> +  // The result of GetPosition() for the active scrolled root of this layer
> +  // for the previous and current paints respectively.

This comment will need to be updated in my patch to make these relative to the reference frame.
Attachment #665308 - Flags: review?(roc) → review+
Comment on attachment 665310 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation v2

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

::: layout/base/nsFrameManager.cpp
@@ +479,5 @@
> +  // that doesn't change the size of the parent.)
> +  // This has to sure to invalidate the entire overflow rect; this
> +  // is important in the presence of absolute positioning
> +  if (!(aFlags & DONT_INVALIDATE)) {
> +    aOldFrame->InvalidateFrameSubtree();

Instead of using a flag I thought you'd just call InvalidateFrameForRemoval() here... You don't seem to have added any callers for InvalidateFrameForRemoval().

::: layout/generic/nsFrame.cpp
@@ +4823,1 @@
>    } 

Can't we just return here? No point in setting the rect property if we've invalidated the entire frame.

::: layout/generic/nsPlaceholderFrame.cpp
@@ +134,5 @@
>  {
>    nsIPresShell* shell = PresContext()->GetPresShell();
>    nsIFrame* oof = mOutOfFlowFrame;
>    if (oof) {
> +    oof->InvalidateFrameSubtree();

Why is this necessary?

::: layout/tables/nsTableFrame.cpp
@@ +1805,5 @@
>      tableRect.Inflate(bcMargin);
>    }
>    aDesiredSize.mOverflowAreas.UnionAllWith(tableRect);
>  
> +  if (GetStateBits() & NS_FRAME_FIRST_REFLOW ||

() around & subexpression

@@ +7249,5 @@
> +  }
> +
> +  // The part that looks at both the rect and the overflow rect is a
> +  // bit of a hack.  See nsBlockFrame::ReflowLine for an eloquent
> +  // description of its hackishness.

Need a note somewhere that this still doesn't make much sense and needs to be cleaned up later.
Comment on attachment 665312 [details] [diff] [review]
Add HasRetainedDataFor

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1121,5 @@
> +  }
> +  data = GetSecondaryLayerManagerDataForFrame(aFrame);
> +  if (data && GetDisplayItemDataForManager(aFrame, aDisplayItemKey, data)) {
> +    return true;
> +  }

We have to do something for non-Mac platforms to ensure that we don't have overhead for the secondary layer manager. But it can be deferred.

::: layout/base/nsPresShell.cpp
@@ +3969,5 @@
>  
>    if (aStateMask.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE)) {
>      nsIFrame* root = mFrameConstructor->GetRootFrame();
>      if (root) {
> +      FrameLayerBuilder::InvalidateAllLayersForFrame(root);

Why have you made this change?
Attachment #665312 - Flags: review?(roc) → review+
Comment on attachment 665313 [details] [diff] [review]
Avoid some causes of unnecessary painting v2

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

::: layout/generic/nsFrame.cpp
@@ +4643,5 @@
> +
> +  // if there are hints other than transform/opacity, invalidate, since we don't know what else to do.
> +  if (aObject->mChangeHint & ~(nsChangeHint_UpdateOpacityLayer|nsChangeHint_UpdateTransformLayer)) {
> +    f->InvalidateFrameSubtree();
> +  }

The remaining cases can be in an else clause, since there's no need to do any more work once we've invalidated the entire subtree.

::: layout/generic/nsIFrame.h
@@ +2216,5 @@
>     * Normally does nothing since DLBI handles removed frames.
> +   * 
> +   * @param aDisplayItemKey If specified, only issues an invalidate
> +   * if this frame painted a display item of that type during the 
> +   * previous paint. SVG rendering observers are always notified.

You didn't actually add the parameter. I think you should not be adding this comment.

::: layout/generic/nsImageFrame.cpp
@@ +599,3 @@
>    } else {
> +    InvalidateFrameWithRect(SourceRectToDest(*aRect), nsDisplayItem::TYPE_IMAGE);
> +    InvalidateFrameWithRect(SourceRectToDest(*aRect), nsDisplayItem::TYPE_ALT_FEEDBACK);

Factor out SourceRectToDest into a local variable.
Attachment #665313 - Flags: review?(roc) → review+
For stuff that's changing offscreen, having to scan frame ancestors all the time just in case there are SVG rendering observers sucks. We should fix that in followup.
Comment on attachment 665338 [details] [diff] [review]
Correctly invalidate SVG observers v2

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

::: layout/svg/base/src/nsSVGEffects.h
@@ +77,5 @@
>    nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, bool* aOK);
>  
>    Element* GetReferencedElement();
>  
> +  virtual bool ShouldObserveReflow() { return true; }

Call it ObservesReflow()
Attachment #665338 - Flags: review?(roc) → review+
Attachment #665272 - Flags: review?(matt.woodrow) → review+
> 
> ::: layout/generic/nsFrame.cpp
> @@ +4823,1 @@
> >    } 
> 
> Can't we just return here? No point in setting the rect property if we've
> invalidated the entire frame.
> 

We didn't invalidate the entire frame really. The rect is an additional property on top of the state bit, so that just makes sure the bit is set, and paints are scheduled. Then the rest of the function sets the actual rect.
> ::: layout/base/nsPresShell.cpp
> @@ +3969,5 @@
> >  
> >    if (aStateMask.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE)) {
> >      nsIFrame* root = mFrameConstructor->GetRootFrame();
> >      if (root) {
> > +      FrameLayerBuilder::InvalidateAllLayersForFrame(root);
> 
> Why have you made this change?

I decided that it made debugging easier at the time since you don't get a massive influx of InvalidateFrameSubtree() calls when switching to gdb.
Attachment #665310 - Attachment is obsolete: true
Attachment #665310 - Flags: review?(roc)
Attachment #665354 - Flags: review?(roc)
Attachment #665368 - Flags: review?(matt.woodrow) → review+
Fixed a bug where we could overwrite an already invalid frame with a partial rect.
Attachment #665309 - Attachment is obsolete: true
Attachment #665372 - Flags: review?(roc)
Comment on attachment 665372 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds v3

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

More followup: fix IsInvalid to return something other than an empty rect to mean "everything"!
Attachment #665372 - Flags: review?(roc) → review+
Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2633180110 to fix the reftest manifest on top of this landing, but there's still reftest leaks on OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef035122864a
https://hg.mozilla.org/integration/mozilla-inbound/rev/074075d10fd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bc2ff685f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/932a03fc9d44
https://hg.mozilla.org/integration/mozilla-inbound/rev/64cd1b640565
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5620eaac6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/16856e7778fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/53823eee494f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e004a9acf00d
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e70f91d826
https://hg.mozilla.org/integration/mozilla-inbound/rev/625343f34ca2
https://hg.mozilla.org/integration/mozilla-inbound/rev/226afc8b39b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b06b79fe138
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c797462d2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7676af66ca43
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc58b98cbf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/01095a6c8926
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeada32fd169
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa0868b05bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a55f82f0feca
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd22436baf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ffee0f2e7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/33be0c4f310d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1da25b8452
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12bf692dc2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/213b15f0c3c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcc52822df2
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f6deffdc70
https://hg.mozilla.org/integration/mozilla-inbound/rev/920cf04e0fe0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70defa60099
Sorry, we had to backout due to crashes on Windows debug mochitests:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e70defa60099

There were also seemingly frequent failures in browser/devtools/highlighter/test/browser_inspector_invalidate.js.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f86e3a3240
Depends on: 795208
(In reply to Ehsan Akhgari [:ehsan] from comment #336)
> Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2633180110
> to fix the reftest manifest on top of this landing, but there's still
> reftest leaks on OS X:

I don't see any reftest leaks on pushes with the reftest manifest fixed, so hopefully the leaks were related to the reftest manifest error.
(In reply to Matt Woodrow (:mattwoodrow) from comment #339)
> Created attachment 665853 [details] [diff] [review]
> Disable test_text on android since it causes crashes

More specifically, it somehow triggers JM on arv6 to generate code that's complete broken.
As we saw on the earlier landing (comment 258) there was a significant Android Talos regression:

Regression Robocop Checkerboarding Benchmark increase 3.03e+03% on Android 2.2 (Native) Mozilla-Inbound
----------------------------------------------------------------------------------------------------------
    Previous: avg 112.814 stddev 44.673 of 30 runs up to revision 9f476b4ac1e1
    New     : avg 3533.110 stddev 246.564 of 5 runs since revision 6b58397ad735
    Change  : +3420.296 (3.03e+03% / z=76.562)
    Graph   : http://mzl.la/QKmFd5

This time, trobopan does not seem to be affected.
https://hg.mozilla.org/mozilla-central/rev/9d205a349c6a
https://hg.mozilla.org/mozilla-central/rev/1556064f1c20
https://hg.mozilla.org/mozilla-central/rev/aa4652f24df9
https://hg.mozilla.org/mozilla-central/rev/e5651c513f3f
https://hg.mozilla.org/mozilla-central/rev/9c8f66d8eee4
https://hg.mozilla.org/mozilla-central/rev/04df652e5847
https://hg.mozilla.org/mozilla-central/rev/6cabf68e297b
https://hg.mozilla.org/mozilla-central/rev/93686f8add79
https://hg.mozilla.org/mozilla-central/rev/0d3c93bc5716
https://hg.mozilla.org/mozilla-central/rev/673eac4d9c55
https://hg.mozilla.org/mozilla-central/rev/cf82755175d0
https://hg.mozilla.org/mozilla-central/rev/25af0febf329
https://hg.mozilla.org/mozilla-central/rev/4372c4914958
https://hg.mozilla.org/mozilla-central/rev/b5559af3cf94
https://hg.mozilla.org/mozilla-central/rev/8d12f7ceea13
https://hg.mozilla.org/mozilla-central/rev/61672a211aac
https://hg.mozilla.org/mozilla-central/rev/ec48fbf9dd69
https://hg.mozilla.org/mozilla-central/rev/1c3434a2ef97
https://hg.mozilla.org/mozilla-central/rev/967b1ef7afdc
https://hg.mozilla.org/mozilla-central/rev/ba21437b7113
https://hg.mozilla.org/mozilla-central/rev/8fa21e95b22d
https://hg.mozilla.org/mozilla-central/rev/b55adbbe23bc
https://hg.mozilla.org/mozilla-central/rev/5947680feefb
https://hg.mozilla.org/mozilla-central/rev/b3c0862a729a
https://hg.mozilla.org/mozilla-central/rev/937aaddb295c
https://hg.mozilla.org/mozilla-central/rev/035ed3e2d9d4
https://hg.mozilla.org/mozilla-central/rev/e98e2d0aef4d
https://hg.mozilla.org/mozilla-central/rev/96752e66aaab
https://hg.mozilla.org/mozilla-central/rev/23f1b8c7d17e
https://hg.mozilla.org/mozilla-central/rev/07914b108f91
https://hg.mozilla.org/mozilla-central/rev/385d57149939
Believe the [leave open] was not intended; closing.
Status: REOPENED → RESOLVED
blocking2.0: - → ---
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P2][leave open] → [Snappy:P2]
Target Milestone: --- → mozilla18
(In reply to Geoff Brown [:gbrown] from comment #341)
> As we saw on the earlier landing (comment 258) there was a significant
> Android Talos regression:
> 
> Regression Robocop Checkerboarding Benchmark increase 3.03e+03% on Android
> 2.2 (Native) Mozilla-Inbound
> -----------------------------------------------------------------------------
> -----------------------------
>     Previous: avg 112.814 stddev 44.673 of 30 runs up to revision
> 9f476b4ac1e1
>     New     : avg 3533.110 stddev 246.564 of 5 runs since revision
> 6b58397ad735
>     Change  : +3420.296 (3.03e+03% / z=76.562)
>     Graph   : http://mzl.la/QKmFd5
> 
> This time, trobopan does not seem to be affected.

Right, so we decided to land anyway because I couldn't reproduce this regression in browser.

When scrolling timecube.com (with paint flashing), I only get invalidations for the scrolled in area and there is little to no checkerboarding (even in a debug build).

After the last landing (with a similar regression), there was obvious invalidation of the entire page when scrolling.

My conclusion was that the test suite is doing something slightly different to regular browsing (I tried various zoom levels) and is doing similar excessive invalidation.

If anyone on the mobile team has time to look into this, I'd really appreciate it. I expect there will be a fairly large to-do list after a landing of this magnitude, so I'm going to be fairly busy.
Depends on: 795591
Depends on: 795594
Depends on: 795611
Depends on: 795619
Depends on: 795626
Nightly is totally unusable atm (Bug 795591). Could you please backout.
Depends on: 795646
Depends on: 795648
Depends on: 795653
Depends on: 795679
Depends on: 795692
Depends on: 795694
Depends on: 795695
I had to give up on the blocked bugs because I'm too tired.  We should re-evaluate them tomorrow.
Nightly updates were stopped for a while.  Make sure yours was built from https://hg.mozilla.org/mozilla-central/rev/a680fd777c3b or later.
Depends on: 795672
Depends on: 795722
No longer depends on: 795672
Depends on: 795728
Depends on: 795749
Depends on: CVE-2012-5829
Depends on: 795796
Depends on: 795819
Depends on: 795826
Depends on: 795829
Depends on: 796115
Depends on: 796159
Depends on: 794103
Blocks: 796182
Depends on: 796847
Depends on: 797059
Depends on: 797254
Depends on: 797255
(In reply to Matt Woodrow (:mattwoodrow) from comment #290)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #287)
> > Sorry, the textframe being added is the timer changing.
> > 
> > Over-invalidation in the maze area looks like a real bug, working on it.
> 
> It was, we were invalidating two rects, one with scaling applied and one
> without. And then extending the two rects out into a single rect that
> covered both. :(
> 
> Fixed now
You say the over-invalidation should be fixed, but I still see it in the latest nightly? Seems the 20x20 is worst, the 40x40 is already much better.
No longer blocks: 796182
Depends on: 797355
Depends on: 797950
Depends on: 798010
Blocks: 798171
Blocks: 798182
Depends on: 798244
Depends on: 795833
Depends on: 798761
Depends on: 798964
Depends on: 799165
First off, Great work Matt!

Still alot points to parents or parents parents being invalidated.

1. Selecting few chars in a text invalidates the entire container (also on typing)
2. Opening a new tab invalidates the entire strip on each frame of the animation.
3. Firefox UI, mouse in/out focus on firefox menus invalidates the whole dropdown.

And confirming maze solver behaviour Tom mentioned in comment 349
Thanks for the report, Dennis. However, this bug being fixed and having 350 comments is not really the place to be posting follow-up information or bugs with what's landed. It muddies the water quickly. Assuming the issues you pointed out are not already filed (check the dependencies at the top here), please file them individually as new bugs and mark them as blocking this bug so that they can get the proper focus and attention they need. Thanks!
Depends on: 799506
Depends on: 799579
Depends on: 798802
Depends on: 795899
Depends on: 801365
Depends on: 800198
No longer blocks: 770517
Depends on: 770517
Depends on: 802100
Depends on: 802170
Depends on: 802176
Depends on: 802180
Depends on: 801784
Depends on: 802321
Depends on: 802440
No longer depends on: 795829
Blocks: 804323
No longer blocks: 804323
Depends on: 805487
Depends on: 795734
No longer depends on: 795734
Are we making sure we get all the regression fixes to Aurora too?
Depends on: 805696
Blocks: 511163
Adding tracking-firefox18 so that we don't forget to check all the regressions.
Depends on: 805197
Depends on: 806583
Depends on: 807934
No longer depends on: 807934
Depends on: 808204
(In reply to Olli Pettay [:smaug] from comment #353)
> Adding tracking-firefox18 so that we don't forget to check all the
> regressions.

Let's schedule a meeting to go through the regressions and track those that are critical, rather than tracking this meta bug. Please let bajaj know if you'd like to be included. The default meeting list will be matt, roc, me, and bajaj, and will happen before the next merge.
Depends on: 809178
Depends on: 810876
Depends on: 810592
Depends on: 807243
Depends on: 809334
No longer depends on: 807243
Depends on: 812897
Depends on: 786485
No longer depends on: 786485
Depends on: 812916
Depends on: 812917
No longer depends on: 812916
No longer depends on: 812917
No longer depends on: 812897
Depends on: 805507
Depends on: 808409
Depends on: 815602
Depends on: 822115
Depends on: 824085
No longer depends on: 824085
Depends on: 820839
Blocks: 807002
Depends on: 791699
Depends on: 831780
Depends on: 833033
Depends on: 838580
Depends on: 795734
Blocks: 845437
Depends on: 846181
Depends on: 848078
Depends on: 855316
Depends on: 861042
Blocks: 722888
Depends on: 885348
Depends on: 900049
Depends on: 864435
Depends on: 906405
Keywords: qawanted
Depends on: 812871
Blocks: 929484
Is there a possible relation with this?
https://bugzilla.mozilla.org/show_bug.cgi?id=834629#c4
Depends on: 991285
Depends on: 1277484
You need to log in before you can comment on or make changes to this bug.