Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 539356 (dlbi)

Replace Invalidate() calls in reflow with display list analysis

RESOLVED FIXED in mozilla18

Status

()

Core
Layout
RESOLVED FIXED
8 years ago
2 months ago

People

(Reporter: Michael Ventnor, Assigned: mattwoodrow)

Tracking

(Depends on: 8 bugs, Blocks: 17 bugs, {perf})

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18-)

Details

(Whiteboard: [Snappy:P2])

Attachments

(53 attachments, 58 obsolete attachments)

1.56 KB, patch
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
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
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
(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 421369 [details] [diff] [review]
Patch 1 - Infrastructure

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)
(Reporter)

Comment 2

8 years ago
Created attachment 421370 [details] [diff] [review]
Patch 2 - Remove Invalidate() calls

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.

Comment 10

8 years ago
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.

Comment 12

8 years ago
> but how common are they

Not very.
(Reporter)

Comment 13

8 years ago
(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.
(Reporter)

Comment 14

8 years ago
D'oh, wait, it'd probably be part of the friend nsDisplayListInvalidator line, wouldn't it?
(Reporter)

Comment 15

8 years ago
Created attachment 423658 [details] [diff] [review]
Remove GetUsedX Assertion

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)
(Reporter)

Comment 16

8 years ago
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)
(Reporter)

Updated

8 years ago
Depends on: 542361
Blocks: 519339
(Reporter)

Comment 17

8 years ago
Created attachment 423868 [details] [diff] [review]
Patch 1 - Infrastructure

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.
(Reporter)

Comment 19

8 years ago
Created attachment 423938 [details] [diff] [review]
Patch 1 - Infrastructure
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.
(Reporter)

Comment 24

8 years ago
Created attachment 424689 [details] [diff] [review]
Patch 1 - Infrastructure

Uploading what I've done so far.
Attachment #423938 - Attachment is obsolete: true
Attachment #423938 - Flags: review?(roc)
(Reporter)

Comment 25

8 years ago
Created attachment 424690 [details] [diff] [review]
Patch 2 - Remove Invalidate() calls
(Reporter)

Comment 26

8 years ago
Created attachment 424691 [details] [diff] [review]
Patch 3 - Manage text-shadow display items better
(Reporter)

Comment 27

8 years ago
Created attachment 424692 [details] [diff] [review]
Patch 4 - Optimise border invalidation

Updated

7 years ago
Blocks: 559396
(Reporter)

Comment 28

7 years ago
(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.

Updated

7 years ago
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.
Yes, there is a patch in bug 579323.
blocking2.0: ? → -

Updated

7 years ago
Blocks: 595574

Updated

7 years ago
Blocks: 554004

Updated

7 years ago
Blocks: 318474

Updated

7 years ago
Blocks: 608648

Updated

7 years ago
Blocks: 119311

Updated

7 years ago
Blocks: 612250
Blocks: 614732
No longer blocks: 612250
No longer blocks: 554004

Updated

7 years ago
Blocks: 626927

Updated

7 years ago
Blocks: 585258

Updated

6 years ago
Blocks: 352367

Updated

6 years ago
Keywords: perf

Updated

6 years ago
Blocks: 454254

Updated

6 years ago
Blocks: 693105
Blocks: 707960

Comment 33

6 years ago
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.

Updated

6 years ago
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

Updated

6 years ago
Blocks: 723315
(Assignee)

Comment 37

6 years ago
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
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 39

6 years ago
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?
(Assignee)

Comment 40

6 years ago
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?
(Assignee)

Comment 41

6 years ago
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.)
(Assignee)

Comment 44

6 years ago
(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.

Comment 45

6 years ago
+cc: jwatt. He is moving the SVG scene graph to use the displayList as the render and hit-test tree.
(Assignee)

Comment 46

6 years ago
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
(Assignee)

Updated

6 years ago
Depends on: 731858
(Assignee)

Updated

5 years ago
Attachment #424689 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #424690 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #424691 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #424692 - Attachment is obsolete: true

Comment 48

5 years ago
(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.
(Assignee)

Comment 49

5 years ago
Created attachment 605171 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects
Attachment #605171 - Flags: review?(roc)
(Assignee)

Comment 50

5 years ago
Created attachment 605172 [details] [diff] [review]
Part 2 - Add new API to BasicLayers
Attachment #605172 - Flags: review?(roc)
(Assignee)

Comment 51

5 years ago
Created attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames
Attachment #605174 - Flags: review?(bzbarsky)
(Assignee)

Comment 52

5 years ago
Created attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed
Attachment #605175 - Flags: review?(roc)
(Assignee)

Comment 53

5 years ago
Created attachment 605176 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction
Attachment #605176 - Flags: review?(jwatt)
(Assignee)

Comment 54

5 years ago
Created attachment 605177 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers
Attachment #605177 - Flags: review?(roc)
(Assignee)

Comment 55

5 years ago
Created attachment 605178 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder
Attachment #605178 - Flags: review?(roc)
(Assignee)

Comment 56

5 years ago
Created attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

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.
(Assignee)

Comment 58

5 years ago
(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.
(Assignee)

Comment 59

5 years ago
Created attachment 605261 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it

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)
(Assignee)

Comment 60

5 years ago
Created attachment 605262 [details] [diff] [review]
Part 9b - Add new frame invalidation API
Attachment #605262 - Flags: review?(roc)
(Assignee)

Comment 61

5 years ago
Created attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code
Attachment #605264 - Flags: review?(bzbarsky)
(Assignee)

Comment 62

5 years ago
Created attachment 605265 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model
Attachment #605265 - Flags: review?(jwatt)
(Assignee)

Comment 63

5 years ago
Created attachment 605266 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation
Attachment #605266 - Flags: review?(roc)
(Assignee)

Comment 64

5 years ago
Created attachment 605267 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget
Attachment #605267 - Flags: review?(roc)
(Assignee)

Comment 65

5 years ago
Created attachment 605268 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model
Attachment #605268 - Flags: review?(roc)
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!
(Assignee)

Comment 67

5 years ago
(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?
(Assignee)

Comment 71

5 years ago
 
> 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.
(Assignee)

Comment 73

5 years ago
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 74

5 years ago
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 75

5 years ago
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 76

5 years ago
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?
(Assignee)

Comment 82

5 years ago
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)
(Assignee)

Updated

5 years ago
Depends on: 739490
(Assignee)

Updated

5 years ago
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-
(Assignee)

Updated

5 years ago
Depends on: 739743
(Assignee)

Comment 89

5 years ago
(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-
Created attachment 610380 [details] [diff] [review]
patch to reinstate the foreignObject registering code removed in bug 734079
Attachment #610380 - Flags: review?(matt.woodrow)
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.
(Assignee)

Updated

5 years ago
Depends on: 741682
(Assignee)

Updated

5 years ago
Depends on: 746421
(Assignee)

Updated

5 years ago
Depends on: 746422

Updated

5 years ago
Blocks: 691645
(Assignee)

Updated

5 years ago
Depends on: 746890

Updated

5 years ago
Blocks: 702485

Comment 96

5 years ago
Matt your try build shows significant less frame drops here :)
https://bugzilla.mozilla.org/show_bug.cgi?id=702485

Updated

5 years ago
Blocks: 747561
(Assignee)

Updated

5 years ago
Depends on: 747718
(Assignee)

Updated

5 years ago
Blocks: 748219
(Assignee)

Updated

5 years ago
Blocks: 748220
(Assignee)

Updated

5 years ago
Blocks: 748228
(Assignee)

Updated

5 years ago
Depends on: 749055
(Assignee)

Updated

5 years ago
Attachment #610380 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 97

5 years ago
Created attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2
Attachment #605171 - Attachment is obsolete: true
Attachment #618520 - Flags: review?(roc)
Attachment #605171 - Flags: review?(roc)
(Assignee)

Comment 98

5 years ago
Created attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2
Attachment #618521 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #605172 - Attachment is obsolete: true
Attachment #605172 - Flags: review?(roc)
(Assignee)

Comment 99

5 years ago
Created attachment 618522 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction v2
Attachment #618522 - Flags: review?(jwatt)
(Assignee)

Updated

5 years ago
Attachment #605176 - Attachment is obsolete: true
Attachment #605176 - Flags: review?(jwatt)
(Assignee)

Comment 100

5 years ago
Created attachment 618523 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers v2
Attachment #618523 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #605177 - Attachment is obsolete: true
Attachment #605177 - Flags: review?(roc)
(Assignee)

Comment 101

5 years ago
Created attachment 618524 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder v2
Attachment #605178 - Attachment is obsolete: true
Attachment #618524 - Flags: review?(roc)
Attachment #605178 - Flags: review?(roc)
(Assignee)

Comment 102

5 years ago
Created 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
Attachment #605179 - Attachment is obsolete: true
Attachment #618525 - Flags: review?(tnikkel)
Attachment #605179 - Flags: review?(tnikkel)
Attachment #605179 - Flags: review?(bas.schouten)
(Assignee)

Comment 103

5 years ago
Created attachment 618526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v2
Attachment #605261 - Attachment is obsolete: true
Attachment #618526 - Flags: review?(roc)
Attachment #605261 - Flags: review?(roc)
(Assignee)

Comment 104

5 years ago
Created attachment 618527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v2
Attachment #605262 - Attachment is obsolete: true
Attachment #618527 - Flags: review?(roc)
Attachment #605262 - Flags: review?(roc)
(Assignee)

Comment 105

5 years ago
Created attachment 618528 [details] [diff] [review]
Part 9c - Remove old invalidation code v2
Attachment #605264 - Attachment is obsolete: true
Attachment #618528 - Flags: review?(bzbarsky)
(Assignee)

Comment 106

5 years ago
Created attachment 618529 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model
Attachment #605265 - Attachment is obsolete: true
Attachment #618529 - Flags: review?(jwatt)
(Assignee)

Comment 107

5 years ago
Created attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2
Attachment #605266 - Attachment is obsolete: true
Attachment #618530 - Flags: review?(roc)
Attachment #605266 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #618530 - Flags: review?(tnikkel)
(Assignee)

Comment 108

5 years ago
Created attachment 618531 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v2
Attachment #605267 - Attachment is obsolete: true
Attachment #618531 - Flags: review?(roc)
Attachment #605267 - Flags: review?(roc)
(Assignee)

Comment 109

5 years ago
Created attachment 618534 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v2
Attachment #605268 - Attachment is obsolete: true
Attachment #618534 - Flags: review?(roc)
Attachment #605268 - Flags: review?(roc)
(Assignee)

Comment 110

5 years ago
Created attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

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)
(Assignee)

Comment 111

5 years ago
Created attachment 618537 [details] [diff] [review]
Part 11: Reimplement empty transactions
Attachment #618537 - Flags: review?(roc)
(Assignee)

Comment 112

5 years ago
Created attachment 618541 [details] [diff] [review]
Part 12: Remove unnecessary LayerManagerLayerBuilder indirection

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)
(Assignee)

Comment 113

5 years ago
Created attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

Reduce time spent in building display lists by only repainting widgets that have had changes made.
Attachment #618542 - Flags: review?(tnikkel)
(Assignee)

Comment 114

5 years ago
Created attachment 618543 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame.

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)
(Assignee)

Comment 115

5 years ago
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.
(Assignee)

Comment 118

5 years ago
(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.
(Assignee)

Comment 119

5 years ago
Created attachment 619830 [details] [diff] [review]
Part 15 - Add table invalidation test

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+

Updated

5 years ago
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)
Attachment #618523 - Flags: review?(roc) → review+
Attachment #618524 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 127

5 years ago
(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.
(Assignee)

Comment 129

5 years ago
(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.
(Assignee)

Comment 130

5 years ago
Created attachment 620926 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v3
Attachment #618526 - Attachment is obsolete: true
Attachment #620926 - Flags: review?(roc)
Attachment #618526 - Flags: review?(roc)
(Assignee)

Comment 131

5 years ago
Created attachment 620927 [details] [diff] [review]
Part 9b - Add new frame invalidation API v3
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.
(Assignee)

Comment 134

5 years ago
(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.
(Assignee)

Comment 135

5 years ago
(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.
(Assignee)

Comment 137

5 years ago
(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+
(Assignee)

Comment 139

5 years ago
(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.
(Assignee)

Comment 141

5 years ago
(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+
(Assignee)

Comment 148

5 years ago
Created attachment 623526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v4
Attachment #620926 - Attachment is obsolete: true
Attachment #623526 - Flags: review?(roc)
Attachment #620926 - Flags: review?(roc)
(Assignee)

Comment 149

5 years ago
Created attachment 623527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v4
Attachment #620927 - Attachment is obsolete: true
Attachment #623527 - Flags: review?(roc)
Attachment #620927 - Flags: review?(roc)
(Assignee)

Comment 150

5 years ago
(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?
(Assignee)

Comment 153

5 years ago
Created attachment 623545 [details] [diff] [review]
Part 9b - Add new frame invalidation API v5
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.
Alias: dlbi
Attachment #618541 - Flags: review?(roc) → review+
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
(Assignee)

Comment 159

5 years ago
Created attachment 623575 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v3
Attachment #618530 - Attachment is obsolete: true
Attachment #623575 - Flags: review?(roc)
Attachment #618530 - Flags: review?(tnikkel)
Attachment #618530 - Flags: review?(roc)
(Assignee)

Comment 160

5 years ago
Created attachment 623576 [details] [diff] [review]
Part 9b - Add new frame invalidation API v6
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)
(Assignee)

Comment 163

5 years ago
(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.
(Assignee)

Comment 164

5 years ago
Created attachment 624286 [details] [diff] [review]
Part 9b - Add new frame invalidation API v7
Attachment #623576 - Attachment is obsolete: true
Attachment #624286 - Flags: review?(roc)
Attachment #623576 - Flags: review?(roc)
(Assignee)

Comment 165

5 years ago
Created attachment 624287 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v4

Carrying forward r=roc
Attachment #623575 - Attachment is obsolete: true
Attachment #624287 - Flags: review+
(Assignee)

Comment 166

5 years ago
Created attachment 624288 [details] [diff] [review]
Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice
Attachment #624288 - Flags: review?(roc)
(Assignee)

Comment 167

5 years ago
Created attachment 624289 [details] [diff] [review]
Part 9f-2 - Compute the invalid area of the layer tree and pass this to the widget v3
Attachment #618531 - Attachment is obsolete: true
Attachment #624289 - Flags: review?(roc)
Attachment #618531 - Flags: review?(roc)
(Assignee)

Comment 168

5 years ago
Created attachment 624290 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v2
Attachment #624290 - 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.
(Assignee)

Comment 174

5 years ago
(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.
(Assignee)

Comment 175

5 years ago
(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.
(Assignee)

Comment 178

5 years ago
Created attachment 625817 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v4

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)
(Assignee)

Comment 180

5 years ago
(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.
(Assignee)

Comment 181

5 years ago
(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.
(Assignee)

Comment 183

5 years ago
Created attachment 625862 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v5
Attachment #625817 - Attachment is obsolete: true
Attachment #625817 - Flags: review?(roc)
Attachment #625862 - Flags: review?(roc)
(Assignee)

Comment 184

5 years ago
Created attachment 625866 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v3
Attachment #618521 - Attachment is obsolete: true
Attachment #625866 - Flags: review?(roc)
(Assignee)

Comment 185

5 years ago
Created attachment 625867 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v3
Attachment #618520 - Attachment is obsolete: true
Attachment #625867 - Flags: review?(roc)
(Assignee)

Comment 186

5 years ago
Created attachment 625868 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v3
Attachment #624290 - Attachment is obsolete: true
Attachment #625868 - 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 :-)
Attachment #625866 - Flags: review?(roc) → review+
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
Created attachment 625914 [details] [diff] [review]
Optimize invalidation regions (WR)

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...
(Assignee)

Comment 195

5 years ago
Created attachment 626253 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v6
Attachment #625862 - Attachment is obsolete: true
Attachment #625862 - Flags: review?(roc)
Attachment #626253 - Flags: review?(roc)
(Assignee)

Comment 196

5 years ago
Created attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3
Attachment #618534 - Attachment is obsolete: true
Attachment #626254 - Flags: review?(roc)
(Assignee)

Comment 197

5 years ago
Created attachment 626255 [details] [diff] [review]
Part 11: Reimplement empty transactions v2
Attachment #618537 - Attachment is obsolete: true
Attachment #618537 - Flags: review?(roc)
Attachment #626255 - Flags: review?(roc)
(Assignee)

Comment 198

5 years ago
Created attachment 626256 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v4
Attachment #625868 - Attachment is obsolete: true
Attachment #625868 - Flags: review?(roc)
Attachment #626256 - 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.
Attachment #626255 - Flags: review?(roc) → review+
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+
(Assignee)

Comment 202

5 years ago
(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.
Attachment #626254 - Flags: review?(roc) → review+
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?
(Assignee)

Comment 204

5 years ago
(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.
(Assignee)

Comment 206

5 years ago
Created attachment 626269 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v7
Attachment #626269 - Flags: review?(roc)
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+
Created attachment 626345 [details] [diff] [review]
A bit more smart region calculation
Attachment #625914 - Attachment is obsolete: true
Attachment #625914 - Flags: feedback?(matt.woodrow)
(Assignee)

Updated

5 years ago
Depends on: 758505
(Assignee)

Updated

5 years ago
Depends on: 761884
(Assignee)

Comment 209

5 years ago
Created attachment 630825 [details] [diff] [review]
Part 10: Test modifications required for DLBI v2
Attachment #618536 - Attachment is obsolete: true
Attachment #618536 - Flags: review?(roc)
Attachment #630825 - Flags: review?(roc)
(Assignee)

Comment 210

5 years ago
Created attachment 630826 [details] [diff] [review]
Part 16 - Revoke any pending ViewManager flushes when we do one (sometimes we get this called from Will Paint events)
Attachment #630826 - Flags: review?(roc)
(Assignee)

Comment 211

5 years ago
Created attachment 630827 [details] [diff] [review]
Part 17 - Don't paint widgets that an invisible or empty bounds
Attachment #630827 - Flags: review?(roc)
(Assignee)

Comment 212

5 years ago
Created attachment 630829 [details] [diff] [review]
Part 18 - Mark frames with only invalid children as an optimization to use when invalidating further frames
Attachment #630829 - Flags: review?(roc)
(Assignee)

Comment 213

5 years ago
Created attachment 630830 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window
Attachment #630830 - Flags: review?(roc)
(Assignee)

Comment 214

5 years ago
Created attachment 630831 [details] [diff] [review]
Part 20 - Simplify regions to avoid excessive region calculation

Patch originally by romaxa
Attachment #626345 - Attachment is obsolete: true
Attachment #630831 - Flags: review?(roc)
(Assignee)

Comment 215

5 years ago
Created attachment 630832 [details] [diff] [review]
Part 21 - BasicLayers should always retain content

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)
(Assignee)

Comment 216

5 years ago
Created attachment 630833 [details] [diff] [review]
Part 22 - Force a background color when running android reftests

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.
Attachment #630826 - Flags: review?(roc) → review+
Attachment #630827 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 219

5 years ago
(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?
Attachment #630829 - Flags: review?(roc) → review+
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
Attachment #630831 - Flags: review?(roc) → review+
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+
(Assignee)

Comment 226

5 years ago
(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.