Closed Bug 1404181 Opened 7 years ago Closed 7 years ago

Land retained-dl into mozilla-central preffed off

Categories

(Core :: Web Painting, enhancement)

Product:
Core
Component:
Web Painting
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

Attachments

(28 files, 2 obsolete files)

Bug 1404181 - Part 1: Make AnimatedGeometryRoot/ActiveScrolledRoot retained and refcounted on the heap.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 3: Track the current set of alive display items on nsIFrame.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 6: Compute and store the frame that contributed the style data for backgrounds so that we can invalidate the display item when either frame changes.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 8: Add refcounting for clip chains.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 11: Factor out PresShell paint count code se we can still call it without needing to build the display items.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 12: Add pref for retained display lists (starting with it disabled).
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 13: Expose DisplayItemData better from FrameLayerBuilder so we can query the geometry (for sync decode invalidations).
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 14: Add a bunch of new helpers to display list builder and display item for retaining and reusing items.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 15: Add the notion of 'modified' frames, that need new display items built for them (and all their descendants).
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 16: Track window dragging area contributions per-frame so that can remove contributions from invalidated frames.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 17: Track will-change contributions per-frame so that can remove contributions from invalidated frames.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 18: Use nsPresArena for the display list builder arena since it supports recycling.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 19: Some items don't use style data from mFrame, so add overrides that let us check the right frame when determining if an item is invalid.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 20: Add code to override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 22: Make sure we mark frames as modified any time they change position or style data and make sure we don't accidentally mark the root as being modified when we don't need to.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 23: Only rebuild items within a displayport when the displayport changes, rather than rebuilding the whole document.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 24: Rebuild all display items when we encounter a blend mode, since we can't easily track changes to whether we need the blend container or not.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 26: Skip DLBI for reused items (since they must not be invalid).
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1404181 - Part 27: Add some retained-dl debugging tools.
59 bytes, text/x-review-board-request
mstange
: review+
Details
fix for part 2
1.17 KB, patch
Details | Diff | Splinter Review 
We should land the code to prevent bit rot (and make sure it's safe to ride the trains), and then work to enable it.
Comment on attachment 8913518 [details]
Bug 1404181 - Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists.

https://reviewboard.mozilla.org/r/184872/#review193602

::: layout/generic/nsIFrame.h:3837
(Diff revision 1)
>  
>    // Checks if we (or any of our descendents) have NS_FRAME_PAINTED_THEBES set, and
>    // clears this bit if so.
>    bool CheckAndClearPaintedState();
>  
> +  // Checks if we (or any of our descendents) have NS_FRAME_BUILT_DISPLAY_LIST set, and

You're not using a frame state bit any more.
Comment on attachment 8913510 [details]
Bug 1404181 - Part 1: Make AnimatedGeometryRoot/ActiveScrolledRoot retained and refcounted on the heap.

https://reviewboard.mozilla.org/r/184856/#review191188

::: layout/painting/nsDisplayList.h:205
(Diff revision 1)
> +    : mFrame(aFrame)
> +    , mParentAGR(aParent)
> +    , mIsAsync(aIsAsync)
> +    , mIsRetained(aIsRetained)
> +  {
> +    MOZ_ASSERT(mParentAGR || mIsAsync);

Maybe add an assertion message here, like "The root AGR should always be treated as an async AGR."

::: layout/painting/nsDisplayList.h:249
(Diff revision 1)
> +
> +    RefPtr<ActiveScrolledRoot> asr;
> +    if (aIsRetained) {
> +      asr = f->GetProperty(ActiveScrolledRootCache());
> +    }
> +    

end-of-line whitespace

::: layout/painting/nsDisplayList.h:287
(Diff revision 1)
>    static nsCString ToString(const mozilla::ActiveScrolledRoot* aActiveScrolledRoot);
>  
>    // Call this when inserting an ancestor.
>    void IncrementDepth() { mDepth++; }
>  
>    const ActiveScrolledRoot* mParent;

Should this be a RefPtr?
Attachment #8913510 - Flags: review?(mstange) → review+
Comment on attachment 8913511 [details]
Bug 1404181 - Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect.

https://reviewboard.mozilla.org/r/184858/#review193698

::: layout/generic/nsFrame.cpp:2712
(Diff revision 1)
>  
>      if (clipCapturedBy == ContainerItemType::eFilter) {
>        clipState.Restore();
>      }
>      // Revert to the post-filter dirty rect.
> -    aBuilder->SetDirtyRect(dirtyRectOutsideSVGEffects);
> +    aBuilder->SetVisibleRect(visibleRectOutsideSVGEffects);

Don't you need to restore both the visible and the dirty rect here?

::: layout/generic/nsFrame.cpp:2802
(Diff revision 1)
>        // Restore clip state now so nsDisplayTransform is clipped properly.
>        clipState.Restore();
>      }
>      // Revert to the dirtyrect coming in from the parent, without our transform
>      // taken into account.
> -    aBuilder->SetDirtyRect(dirtyRectOutsideTransform);
> +    aBuilder->SetVisibleRect(visibleRectOutsideTransform);

same here - I'd expect both the dirty and the visible rect to be set here.

::: layout/generic/nsFrame.cpp:2924
(Diff revision 1)
>  /**
>   * Check if a frame should be visited for building display list.
>   */
>  static bool
>  DescendIntoChild(nsDisplayListBuilder* aBuilder, nsIFrame *aChild,
> -                 const nsRect& aDirty)
> +                 const nsRect& aDirty, const nsRect& aVisible)

Could you make the order of the arguments here consistent with the AutoBuildingDisplayList constructor and with DecideScrollableLayer? First visible, then dirty.

::: layout/generic/nsIFrame.h:4237
(Diff revision 1)
> +  
> +  bool mForceDescendIntoIfVisible : 1;

End-of-line whitespace, and this needs a comment, and the comment below mIsPrimaryFrame about the size of the hole needs to be adjusted.

::: layout/generic/nsPageFrame.cpp:564
(Diff revision 1)
>      // these extra pages are pruned so that only display items for the
>      // page we currently care about (which we would have reached by
>      // following placeholders to their out-of-flows) end up on the list.
>      nsIFrame* page = child;
>      while ((page = GetNextPage(page)) != nullptr) {
>        nsRect childDirty = dirtyRect + child->GetOffsetTo(page);

Rename dirtyRect to visibleRect and childDirty to childVisible? I think that would be clearer.

::: layout/painting/nsDisplayList.cpp:1000
(Diff revision 1)
>  
>    nsCSSRendering::EndFrameTreesLocked();
>  }
>  
> -static void MarkFrameForDisplay(nsIFrame* aFrame, nsIFrame* aStopAtFrame) {
> +void
> +nsDisplayListBuilder::MarkFrameForDisplay(nsIFrame* aFrame, nsIFrame* aStopAtFrame) {

move brace into next line since you're touching this line anyway

::: layout/painting/nsDisplayList.cpp:1015
(Diff revision 1)
>      }
>    }
>  }
>  
> +void
> +nsDisplayListBuilder::MarkFrameForDisplayIfVisible(nsIFrame* aFrame, nsIFrame* aStopAtFrame) {

move brace

::: layout/painting/nsDisplayList.cpp:1128
(Diff revision 1)
> +    // TODO: We probably don't want visible and dirty to be the same here, figure
> +    // out what to do.
> +    visible = dirtyRectRelativeToDirtyFrame;

What's the problem here?

::: layout/xul/nsRootBoxFrame.cpp:179
(Diff revision 1)
> -    nsRect displayPortBase =
> +    nsRect displayPortBase = aBuilder->GetVisibleRect().Intersect(nsRect(nsPoint(0, 0), GetSize()));
> -      aBuilder->GetDirtyRect().Intersect(nsRect(nsPoint(0, 0), GetSize()));

Let's keep the line break :)

::: layout/xul/nsSliderFrame.cpp:425
(Diff revision 1)
>          refSize.height /= scale.height;
>        }
> -      nsRect dirty = aBuilder->GetDirtyRect().Intersect(thumbRect);
> -      dirty = nsLayoutUtils::ComputePartialPrerenderArea(aBuilder->GetDirtyRect(), overflow, refSize);
> +      nsRect dirty = aBuilder->GetVisibleRect().Intersect(thumbRect);
> +      dirty = nsLayoutUtils::ComputePartialPrerenderArea(aBuilder->GetVisibleRect(), overflow, refSize);
>  
> +      // TODO: Should we be setting visible and dirty to the same thing here?

Well, what is necessary for prerendering? I'd assume we just need to enlarge the visible rect, and take the dirty rect as is... unless the dirty rect has been affected by clipping from ancestor frames, in which case some of the dirtiness information may have been lost...
Attachment #8913511 - Flags: review?(mstange) → review+
Comment on attachment 8913512 [details]
Bug 1404181 - Part 3: Track the current set of alive display items on nsIFrame.

https://reviewboard.mozilla.org/r/184860/#review193704

::: layout/generic/nsFrame.cpp:919
(Diff revision 1)
> +
> +bool
> +nsIFrame::HasDisplayItems()
> +{
> +  DisplayItemArray* items = GetProperty(DisplayItems());
> +  return !!items;

return items != nullptr; is the more common way to do this, I think

::: layout/generic/nsIFrame.h:1138
(Diff revision 1)
>    nsPoint GetPositionIgnoringScrolling();
>  
>    typedef AutoTArray<nsIContent*, 2> ContentArray;
>    static void DestroyContentArray(ContentArray* aArray);
>  
> +  typedef AutoTArray<nsDisplayItem*, 4> DisplayItemArray;

Have you investigated whether using a SmallPointerArray for this one gives any benefits?
Attachment #8913512 - Flags: review?(mstange) → review+
Comment on attachment 8913513 [details]
Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again.

https://reviewboard.mozilla.org/r/184862/#review193718

::: layout/painting/nsDisplayList.cpp:2891
(Diff revision 1)
> +  SetClipChain(aBuilder->CreateClipChainIntersection(ancestorClip, mClipChain, aOther),
> +               true);

We have some callers of IntersectClip in ApplyOpacity methods. For those we should probably be passing aStore = false here. So I think IntersectClip needs to grow an aStore argument.
Attachment #8913513 - Flags: review?(mstange) → review+
Comment on attachment 8913514 [details]
Bug 1404181 - Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames.

https://reviewboard.mozilla.org/r/184864/#review193722

::: layout/painting/nsDisplayList.h:4139
(Diff revision 1)
> +    // TODO: It would be nice if we could ask the initial construction above
> +    // to include simplification.

Yeah, constructing a region from an array of lots of rectangles is probably going to be slower than what we had before, and this is something that impacts even builds with retained display lists off.

::: layout/painting/nsDisplayList.cpp:4709
(Diff revision 1)
> -      mDispatchToContentHitRegion.OrWith(CombinedTouchActionRegion());
> -      mDispatchToContentHitRegion.SimplifyOutward(8);
> +      // TODO: Is it possible that merging event region items
> +      // would change whether this needs to happen, and we can't
> +      // undo it? Should we defer this work until we need the result
> +      // instead?

I don't know, I haven't thought through the alreadyHadRegions case.
Attachment #8913514 - Flags: review?(mstange) → review+
Comment on attachment 8913515 [details]
Bug 1404181 - Part 6: Compute and store the frame that contributed the style data for backgrounds so that we can invalidate the display item when either frame changes.

https://reviewboard.mozilla.org/r/184866/#review194598
Attachment #8913515 - Flags: review?(mstange) → review+
Comment on attachment 8913511 [details]
Bug 1404181 - Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect.

https://reviewboard.mozilla.org/r/184858/#review193698

> Don't you need to restore both the visible and the dirty rect here?

I don't think I do, since we're past the point that the dirty rect is used to determine if we want to descend into frames. The remainder of the function just uses the visible rect to initialize mVisibleRect on the display items, and then we'll pop the AutoBuildingDisplayList off the stack and overwrite it.

> What's the problem here?

The existing code overrides the dirty/visible rect when we're building an async fixed position frame. 

We definitely don't want to make the dirty/visible the same (which is what it does now), since that results in us rebuilding all fixed position content.

I haven't yet understood the code well enough to figure out what transformation to apply to the dirty rect though. I'll do this as a follow-up to avoid breaking things.

> Well, what is necessary for prerendering? I'd assume we just need to enlarge the visible rect, and take the dirty rect as is... unless the dirty rect has been affected by clipping from ancestor frames, in which case some of the dirtiness information may have been lost...

Yeah indeed, the clipping makes this hard. I don't think it matters just building using the visible rect, so I'll just drop the comment.
Comment on attachment 8913512 [details]
Bug 1404181 - Part 3: Track the current set of alive display items on nsIFrame.

https://reviewboard.mozilla.org/r/184860/#review193704

> Have you investigated whether using a SmallPointerArray for this one gives any benefits?

SmallPointerArray is a space-optimized AutoTArray<T,2>. We frequently have more than 2 items (especially after partial building, but before merging when there can be two instances of a logical item), so I think the space savings would be cancelled out by more frequent allocations. I haven't actually measured this though.
Comment on attachment 8913516 [details]
Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode.

https://reviewboard.mozilla.org/r/184868/#review195034
Attachment #8913516 - Flags: review?(mstange) → review+
Comment on attachment 8913517 [details]
Bug 1404181 - Part 8: Add refcounting for clip chains.

https://reviewboard.mozilla.org/r/184870/#review195036

::: layout/painting/nsDisplayList.cpp:1351
(Diff revision 2)
> +  // Reverse iterate the clip chains, so that we destroy descendants
> +  // first which will drop the ref count on their ancestors.
> +  auto it = mClipChainsToDestroy.begin();

Please also say in the comment that the clip chains are stored in backwards order in the list, because we add the newest one at the front of the list. Otherwise it's a bit hard to understand how the code could be iterating over the list backwards.
Attachment #8913517 - Flags: review?(mstange) → review+
Comment on attachment 8913518 [details]
Bug 1404181 - Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists.

https://reviewboard.mozilla.org/r/184872/#review195042
Attachment #8913518 - Flags: review?(mstange) → review+
Comment on attachment 8913519 [details]
Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames.

https://reviewboard.mozilla.org/r/184874/#review195048

::: layout/painting/nsDisplayList.h:3694
(Diff revision 2)
>                              const nsRect& aBackgroundRect);
>    virtual ~nsDisplayThemedBackground();
>  
> +  void Destroy(nsDisplayListBuilder* aBuilder) override
> +  {
> +    aBuilder->UnregisterThemeGeometry(mFrame);

Maybe add a comment that it's fine to remove all theme geometries for the frame, because there will never be more than one nsDisplayThemedBackground per frame.

Actually, if that's the case, do we need to have arrays in the hashtable at all?
Attachment #8913519 - Flags: review?(mstange) → review+
Comment on attachment 8913520 [details]
Bug 1404181 - Part 11: Factor out PresShell paint count code se we can still call it without needing to build the display items.

https://reviewboard.mozilla.org/r/184876/#review195068
Attachment #8913520 - Flags: review?(mstange) → review+
Comment on attachment 8913522 [details]
Bug 1404181 - Part 12: Add pref for retained display lists (starting with it disabled).

https://reviewboard.mozilla.org/r/184880/#review195078
Attachment #8913522 - Flags: review?(mstange) → review+
Comment on attachment 8913523 [details]
Bug 1404181 - Part 13: Expose DisplayItemData better from FrameLayerBuilder so we can query the geometry (for sync decode invalidations).

https://reviewboard.mozilla.org/r/184882/#review195082
Attachment #8913523 - Flags: review?(mstange) → review+
Comment on attachment 8913524 [details]
Bug 1404181 - Part 15: Compute scrollbar thumb AGR state in advance, rather than setting it during building.

https://reviewboard.mozilla.org/r/184884/#review195086

::: layout/generic/nsGfxScrollFrame.cpp:5073
(Diff revision 2)
> +  nsIContent* content = mOuter->GetContent();
>    return mHasBeenScrolledRecently ||
>           IsAlwaysActive() ||
> -         mWillBuildScrollableLayer;
> +         nsLayoutUtils::HasDisplayPort(content) ||
> +         nsContentUtils::HasScrollgrab(content);
>  }
>  
>  bool
>  ScrollFrameHelper::IsScrollingActive(nsDisplayListBuilder* aBuilder) const
>  {
>    const nsStyleDisplay* disp = mOuter->StyleDisplay();
>    if (disp && (disp->mWillChangeBitField & NS_STYLE_WILL_CHANGE_SCROLL) &&
>      aBuilder->IsInWillChangeBudget(mOuter, GetScrollPositionClampingScrollPortSize())) {
>      return true;
>    }
>  
> +  nsIContent* content = mOuter->GetContent();
>    return mHasBeenScrolledRecently ||
>           IsAlwaysActive() ||
> -         mWillBuildScrollableLayer;
> +         nsLayoutUtils::HasDisplayPort(content) ||
> +         nsContentUtils::HasScrollgrab(content);

I think these changes are on mozilla-central now.
Attachment #8913524 - Flags: review?(mstange) → review+
Comment on attachment 8913525 [details]
Bug 1404181 - Part 14: Add a bunch of new helpers to display list builder and display item for retaining and reusing items.

https://reviewboard.mozilla.org/r/184886/#review195088

::: layout/painting/nsDisplayList.h:2513
(Diff revision 2)
> -    return Frame()->In3DContextAndBackfaceIsHidden();
> +    if (mBackfaceHidden) {
> +      return *mBackfaceHidden;
> +    }
> +
> +    bool backfaceHidden = Frame()->In3DContextAndBackfaceIsHidden();
> +    mBackfaceHidden.emplace(backfaceHidden);

Maybe add a comment somewhere that this is a cache that never needs to be invalidated because the display item will be recreated if the backface visibility changes.
Attachment #8913525 - Flags: review?(mstange) → review+
Comment on attachment 8913526 [details]
Bug 1404181 - Part 15: Add the notion of 'modified' frames, that need new display items built for them (and all their descendants).

https://reviewboard.mozilla.org/r/184888/#review195090

::: layout/painting/nsDisplayList.h:620
(Diff revision 2)
>  
>    bool IsBuilding() const { return mIsBuilding; }
>    void SetIsBuilding(bool aIsBuilding)
>    {
>      mIsBuilding = aIsBuilding;
> +    for (nsIFrame* f : mModifiedFramesDuringBuilding) {                                                                                                                        

Some serious end-of-line whitespace here
Attachment #8913526 - Flags: review?(mstange) → review+
Comment on attachment 8913527 [details]
Bug 1404181 - Part 16: Track window dragging area contributions per-frame so that can remove contributions from invalidated frames.

https://reviewboard.mozilla.org/r/184890/#review195096

::: layout/painting/nsDisplayList.cpp:1862
(Diff revision 2)
> +      mWindowDraggingFrames[i] = mWindowDraggingFrames[length - 1];
> +      mWindowDraggingRects[i] = mWindowDraggingRects[length - 1];
> +      length--;

This took me a second to understand, but it's not complicated enough to make me complain about it, so I think you can keep it.

::: layout/painting/nsDisplayList.cpp:1871
(Diff revision 2)
> +      i++;
> +    }
> +  }
> +  mWindowDraggingFrames.resize(length);
> +  mWindowDraggingRects.SetLength(length);
> +  

whitespace
Attachment #8913527 - Flags: review?(mstange) → review+
Comment on attachment 8913528 [details]
Bug 1404181 - Part 17: Track will-change contributions per-frame so that can remove contributions from invalidated frames.

https://reviewboard.mozilla.org/r/184892/#review195098

::: layout/generic/nsFrame.cpp:3112
(Diff revision 2)
>    }
>  
>    nsIFrame* child = aChild;
>    if (child->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE)
>      return;
> +  

whitespace

::: layout/generic/nsIFrame.h:4291
(Diff revision 2)
>     */
>    bool mBuiltDisplayList : 1;
>  
>    bool mFrameIsModified : 1;
>  
> +  bool mMayHaveWillChangeBudget : 1;

Another frame bit? :(

You need to update the comment about the size of the gap again.

::: layout/painting/nsDisplayList.cpp:1987
(Diff revision 2)
> +  
> +  DocumentWillChangeBudget& budget = 

end-of-line whitespace in both lines
Attachment #8913528 - Flags: review?(mstange) → review+
Comment on attachment 8913529 [details]
Bug 1404181 - Part 18: Use nsPresArena for the display list builder arena since it supports recycling.

https://reviewboard.mozilla.org/r/184894/#review195104

::: layout/painting/nsDisplayList.cpp:1449
(Diff revision 2)
>        }
>      }
>    }
>  }
>  
> +uint32_t gDisplayItemSizes[static_cast<uint32_t>(DisplayItemType::TYPE_MAX)] = { 0 };

Would be nice to initialize this statically, maybe with some nsDisplayItemTypesList.h #include magic and constexpr?
Attachment #8913529 - Flags: review?(mstange) → review+
Comment on attachment 8913530 [details]
Bug 1404181 - Part 19: Some items don't use style data from mFrame, so add overrides that let us check the right frame when determining if an item is invalid.

https://reviewboard.mozilla.org/r/184896/#review195526

::: layout/painting/nsDisplayList.h:1997
(Diff revision 2)
>      MOZ_ASSERT(mFrame, "Trying to use display item after deletion!");
>      return mFrame;
>    }
>    
> +  /**
> +   * @return the frame for invalidation.

Is this frame the only frame that causes this item to be invalidated? Or is it an *additional* frame that causes the item to be invalidated, next to item->Frame()? If it's the latter, I think ExtraFrameForInvalidation would be a better name.

I also think the comment could be a little more verbose.

::: layout/painting/nsDisplayList.cpp:2564
(Diff revision 2)
>  uint32_t nsDisplayList::Count() const {
> -  uint32_t count = 0;
> +  return mLength;
> -  for (nsDisplayItem* i = GetBottom(); i; i = i->GetAbove()) {
> -    ++count;
> -  }
> -  return count;
>  }
>  
>  nsDisplayItem* nsDisplayList::RemoveBottom() {
>    nsDisplayItem* item = mSentinel.mAbove;
>    if (!item)
>      return nullptr;
>    mSentinel.mAbove = item->mAbove;
>    if (item == mTop) {
>      // must have been the only item
>      mTop = &mSentinel;
>    }
>    item->mAbove = nullptr;
> +  mLength--;
>    return item;
>  }

Would be better to move these changes to part 16, where you're adding mLength.
Attachment #8913530 - Flags: review?(mstange) → review+
Comment on attachment 8913531 [details]
Bug 1404181 - Part 20: Add code to override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts.

https://reviewboard.mozilla.org/r/184898/#review195532

::: commit-message-b76b8:1
(Diff revision 2)
> +Bug 1404181 - Part 22: Add code for override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts. r?mstange

for -> to

::: layout/generic/nsFrame.cpp:2620
(Diff revision 2)
> +      dirtyRect = data->mDirtyRect;
> +      dirtyRect.IntersectRect(dirtyRect, visibleRect);

I suggest
dirtyRect = data->mDirtyRect.Intersect(visibleRect)

::: layout/generic/nsGfxScrollFrame.cpp:3748
(Diff revision 2)
> +      if (!aBuilder->IsPartialUpdate() ||
> +          aBuilder->InInvalidSubtree()) {

These could go in one line.

::: layout/generic/nsGfxScrollFrame.cpp:3748
(Diff revision 2)
> +      if (!aBuilder->IsPartialUpdate() ||
> +          aBuilder->InInvalidSubtree()) {
> -      *aDirtyRect = displayPort;
> +        *aDirtyRect = displayPort;
> +        if (aUsingDisplayPortInvalidRect) {
> +          *aUsingDisplayPortInvalidRect = true;
> +        }
> +      } else if (mOuter->HasOverrideDirtyRegion()) {
> +        nsRect* rect =
> +          mOuter->GetProperty(nsDisplayListBuilder::DisplayListBuildingDisplayPortRect());
> +        if (rect) {
> +          *aDirtyRect = *rect;
> +          if (aUsingDisplayPortInvalidRect) {
> +            *aUsingDisplayPortInvalidRect = true;

I don't really understand the commonality between the two cases where you set aUsingDisplayPortInvalidRect to true. In the first case, you use the full display port, and in the second case, you use the override rectangle. I guess in the "*aUsingDisplayPortInvalidRect == false" case, the dirty rect is unchanged? Maybe the argument should be renamed to "dirtyRectHasBeenOverridden" or "dirtyRectHasPotentiallyBeenEnlarged"?
Attachment #8913531 - Flags: review?(mstange) → review+
Comment on attachment 8913513 [details]
Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again.

https://reviewboard.mozilla.org/r/184862/#review195722


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsTextFrame.cpp:4950
(Diff revision 3)
>    virtual ~nsDisplayText() {
>      MOZ_COUNT_DTOR(nsDisplayText);
>    }
>  #endif
>  
> +  virtual void RestoreState() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8913516 [details]
Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode.

https://reviewboard.mozilla.org/r/184868/#review195726


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsBulletFrame.cpp:191
(Diff revision 3)
>    {
>      nsBulletFrame* f = static_cast<nsBulletFrame*>(aItem->Frame());
>      mOrdinal = f->GetOrdinal();
>    }
>  
> +  virtual bool InvalidateForSyncDecodeImages() const override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: layout/generic/nsPluginFrame.cpp:919
(Diff revision 3)
>                                     const ContainerLayerParameters& aParameters) override
>    {
>      return LAYER_ACTIVE;
>    }
> +
> +  virtual nsDisplayItemGeometry* AllocateGeometry(nsDisplayListBuilder* aBuilder) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
(In reply to Markus Stange [:mstange] from comment #72)
> Comment on attachment 8913519 [details]
> Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that
> can remove contributions from invalidated frames.
> 
> https://reviewboard.mozilla.org/r/184874/#review195048
> 
> ::: layout/painting/nsDisplayList.h:3694
> (Diff revision 2)
> >                              const nsRect& aBackgroundRect);
> >    virtual ~nsDisplayThemedBackground();
> >  
> > +  void Destroy(nsDisplayListBuilder* aBuilder) override
> > +  {
> > +    aBuilder->UnregisterThemeGeometry(mFrame);
> 
> Maybe add a comment that it's fine to remove all theme geometries for the
> frame, because there will never be more than one nsDisplayThemedBackground
> per frame.
> 
> Actually, if that's the case, do we need to have arrays in the hashtable at
> all?

The reason for using the array is because nsTreeBodyFrame might register more than one ThemeGeometry, one for each selected row.
http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame.cpp#2881

Currently though, I am not sure if any UI elements[1] using this code actually allow multiple selection.

[1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path=
Comment on attachment 8913532 [details]
Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list.

https://reviewboard.mozilla.org/r/184900/#review196506

::: layout/base/nsLayoutUtils.cpp:3722
(Diff revision 2)
>    builder.ClearHaveScrollableDisplayPort();
>    if (builder.IsPaintingToWindow()) {
>      MaybeCreateDisplayPortInFirstScrollFrameEncountered(aFrame, builder);
>    }
>  
>    nsRect dirtyRect = visibleRegion.GetBounds();

Should this variable be renamed to visibleRect maybe?

::: layout/base/nsLayoutUtils.cpp:3777
(Diff revision 2)
> +        builder.EnterPresShell(aFrame);
> +        builder.SetDirtyRect(dirtyRect);
> +        builder.ClearWindowDraggingRegion();
> +        aFrame->BuildDisplayListForStackingContext(&builder, &list);
> -    AddExtraBackgroundItems(builder, list, aFrame, canvasArea, visibleRegion, aBackstop);
> +        AddExtraBackgroundItems(builder, list, aFrame, canvasArea, visibleRegion, aBackstop);
> -
> +  

end-of-line whitespace

::: layout/base/nsLayoutUtils.cpp:3987
(Diff revision 2)
>    }
>  
> -  builder.EndFrame();
> +  {
> +    AutoProfilerTracing tracing("Paint", "DisplayListResources");
>  
> +    

end-of-line whitespace

::: layout/base/nsLayoutUtils.cpp:3992
(Diff revision 2)
> +      delete listPtr;
> +      delete builderPtr;

This seems brittle to me. Could you maybe have local variables like this:
Maybe<nsDisplayListBuilder> nonRetainedBuilder;
Maybe<nsDisplayList> nonRetainedList;

which you make listPtr and builderPtr point to in the non-retained case? Then you wouldn't have to free these objects manually.

::: layout/painting/RetainedDisplayListBuilder.cpp:752
(Diff revision 2)
> +    //printf_stderr("Painting --- Merged list:\n");
> +    //nsFrame::PrintDisplayList(&builder, list);
> +
> +    merged = true;
> +  }
> +      

end-of-line whitespace
Comment on attachment 8913532 [details]
Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list.

https://reviewboard.mozilla.org/r/184900/#review196514

::: layout/base/nsLayoutUtils.cpp:3767
(Diff revision 3)
> +      const bool paintedPreviously =
> +        aFrame->HasProperty(nsIFrame::ModifiedFrameList());
> +
> +      bool merged = false;
> +      if (retainedBuilder && paintedPreviously) {
> +        merged = retainedBuilder->AttemptPartialUpdate(aBackstop);

So this does mBuilder.RootReferenceFrame()->BuildDisplayListForStackingContext(...) internally. Maybe it would be better to pass aFrame in here? When I first read this, it wasn't obvious to me just by looking at the caller side that AttemptPartialUpdate is going to actually do the display list building + merging, as opposed to just doing some preparatory work.
Or maybe just a comment would help.

::: layout/base/nsLayoutUtils.cpp:3784
(Diff revision 3)
> +    // if (XRE_IsContentProcess()) {
> +    //   printf_stderr("Painting --- Full list:\n");
> +    //   nsFrame::PrintDisplayList(&builder, list);
> +    // }

May want to remove this before landing, or maybe turn it into an #if 0. (Or add a pref, or an #ifdef with a name, or whatever you prefer.)

::: layout/painting/RetainedDisplayListBuilder.h:22
(Diff revision 3)
> +  nsDisplayListBuilder* Builder() { return &mBuilder; }
> +
> +  nsDisplayList* List() { return &mList; }
> +
> +  bool AttemptPartialUpdate(nscolor aBackstop);

This is a very small public API. Good!

::: layout/painting/RetainedDisplayListBuilder.cpp:55
(Diff revision 3)
> +      } else if (!(i->GetFlags() & TYPE_RENDERS_NO_IMAGES)) {
> +        invalidate = true;
> +      }
> +
> +      if (invalidate) {
> +        i->Frame()->MarkNeedsDisplayItemRebuild();

Maybe also do this for i->FrameForInvalidation()? Or is the assumption that items with images always have Frame() == FrameForInvalidation()? If that's the case, please add a comment.

::: layout/painting/RetainedDisplayListBuilder.cpp:95
(Diff revision 3)
> +      continue;
> +    }
> +
> +    if (i->GetChildren()) {
> +      AnimatedGeometryRoot *childAGR = aAGR;
> +      if (i->Frame()->IsStackingContext()) {

You use i->Frame() quite a bit in this function. May want to store it in a variable.

::: layout/painting/RetainedDisplayListBuilder.cpp:133
(Diff revision 3)
> +{
> +  return aFirst->Frame() == aSecond->Frame() &&
> +         aFirst->GetPerFrameKey() == aSecond->GetPerFrameKey();
> +}
> +
> +struct DisplayItemKey {

I think the style guide wants a line break before the opening brace.

::: layout/painting/RetainedDisplayListBuilder.cpp:141
(Diff revision 3)
> +    return mFrame == aOther.mFrame &&
> +           mKey == aOther.mKey;
> +  }
> +
> +  nsIFrame* mFrame;
> +  uint32_t mKey;

Maybe call this mPerFrameKey? The different meanings of the word "key" in the nsDisplayItemHashEntry implementations were a bit confusing to me.

::: layout/painting/RetainedDisplayListBuilder.cpp:187
(Diff revision 3)
> +  }
> +
> +  aArray.RemoveElementAt(aArray.Length() - 1);
> +}
> +
> +void MergeFrameRects(nsDisplayLayerEventRegions* aOldItem,

static void

::: layout/painting/RetainedDisplayListBuilder.cpp:249
(Diff revision 3)
> +  }
> +}
> +
> +void MergeLayerEventRegions(nsDisplayItem* aOldItem,
> +                            nsDisplayItem* aNewItem,
> +                            bool aUpdateOld)

I'd like to buy an UPDATE_OLD_ITEM | UPDATE_NEW_ITEM enum to replace the bool with.

::: layout/painting/RetainedDisplayListBuilder.cpp:298
(Diff revision 3)
> + *
> + * The basic algorithm is:
> + *
> + * For-each item in the new list:
> + *     If the item has a matching item in the old list:
> + *         Pop items off the old list until we reach the matching item:

Not sure what "Pop items off" means; remove them from the start? Do the removed items get destroyed?

::: layout/painting/RetainedDisplayListBuilder.cpp:301
(Diff revision 3)
> + * For-each item in the new list:
> + *     If the item has a matching item in the old list:
> + *         Pop items off the old list until we reach the matching item:
> + *             Add valid items to the merged list, destroy invalid items.
> + *         Destroy the matching item from the old list.
> + *     Add the item from the new lis into the merged list.

lis -> list

::: layout/painting/RetainedDisplayListBuilder.cpp:344
(Diff revision 3)
> +RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
> +                                              nsDisplayList* aOldList,
> +                                              nsDisplayList* aOutList)
> +{
> +  nsDisplayList merged(&mBuilder);
> +  nsDisplayItem* old;

The declaration of this variable is so far away from its uses. I'd prefer to have two different variables for this in the two places where its used, just before the respective loop.

::: layout/painting/RetainedDisplayListBuilder.cpp:381
(Diff revision 3)
> +    }
> +    newListLookup.Put({ i->Frame(), i->GetPerFrameKey() }, i);
> +  }
> +#endif
> +
> +  while (nsDisplayItem* i = aNewList->RemoveBottom()) {

Maybe call i newItem instead

::: layout/painting/RetainedDisplayListBuilder.cpp:398
(Diff revision 3)
> +        // If the new item has a matching counterpart in the old list, copy all valid
> +        // items from the old list into the merged list until we get to the repeat.

We already know that there is a matching counterpart in the old list, because we've null-checked oldItem. Maybe rephrase the comment?

::: layout/painting/RetainedDisplayListBuilder.cpp:408
(Diff revision 3)
> +          } else {
> +            oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
> +            old->Destroy(&mBuilder);
> +          }
> +        }
> +        MOZ_ASSERT(old && IsSameItem(i, old));

Would MOZ_ASSERT(old == oldItem); be a valid assertion here?

::: layout/painting/RetainedDisplayListBuilder.cpp:467
(Diff revision 3)
> +
> +  aOutList->AppendToTop(&merged);
> +}
> +
> +static void
> +AddModifiedFramesFromRootFrame(std::vector<WeakFrame>& aFrames,

How about TakeAndAddModifiedFramesFromRootFrame? Would make it clearer that the array in the property will be cleared.

::: layout/painting/RetainedDisplayListBuilder.cpp:476
(Diff revision 3)
> +    for (WeakFrame& frame : *frames) {
> +      aFrames.push_back(Move(frame));
> +    }
> +
> +    frames->clear();

How about:

aFrames.swap(*frames);
frames->clear();

instead of the loop

::: layout/painting/RetainedDisplayListBuilder.cpp:522
(Diff revision 3)
> +
> +  return modifiedFrames;
> +}
> +
> +// ComputeRebuildRegion  debugging
> +// #define CRR_DEBUG 0

If it's commented out anyway, might as well #define it to 1 :)

::: layout/painting/RetainedDisplayListBuilder.cpp:562
(Diff revision 3)
> +                                                 nsRect* aOutDirty,
> +                                                 AnimatedGeometryRoot** aOutModifiedAGR,
> +                                                 nsTArray<nsIFrame*>* aOutFramesWithProps)
> +{
> +  CRR_LOG("Computing rebuild regions for %d frames:\n", aModifiedFrames.size());
> +  for (nsIFrame* f : aModifiedFrames) {

... neat (how you're getting the nsIFrame* out of the nsWeakFrame here)

::: layout/painting/RetainedDisplayListBuilder.cpp:626
(Diff revision 3)
> +          // Don't contribute to the root dirty area at all.
> +          overflow.SetEmpty();

This is cool! No more slow display list building because of an offscreen throbber animation.

::: layout/painting/RetainedDisplayListBuilder.cpp:637
(Diff revision 3)
> +      if (currentFrame->IsStackingContext()) {
> +        CRR_LOG("Frame belongs to stacking context frame %p\n", currentFrame);
> +        // If we found an intermediate stacking context with an existing display item
> +        // then we can store the dirty rect there and stop.
> +        if (currentFrame != mBuilder.RootReferenceFrame() &&
> +            currentFrame->HasDisplayItems()) {

I forget: Does this also return true if any descendant frames have display items?

It looks like we're not rebuilding any part of the stacking context if the stacking context does not have display items. Why is that ok? Couldn't there be a case where the stacking context had no display items before, and then something inside the stacking context changed so that the stacking context needs display items now?

::: layout/painting/RetainedDisplayListBuilder.cpp:652
(Diff revision 3)
> +            currentFrame->SetProperty(nsDisplayListBuilder::DisplayListBuildingRect(), data);
> +            currentFrame->SetHasOverrideDirtyRegion(true);
> +            aOutFramesWithProps->AppendElement(currentFrame);
> +          }
> +          data->mDirtyRect.UnionRect(data->mDirtyRect, overflow);
> +          CRR_LOG("Adding area to stacking context draw area: %d %d %d %d\n", overflow.x, overflow.y, overflow.width, overflow.height);

Should probably break this line somewhere.

::: layout/painting/RetainedDisplayListBuilder.cpp:766
(Diff revision 3)
> +    f->DeleteProperty(nsDisplayListBuilder::DisplayListBuildingRect());
> +    f->DeleteProperty(nsDisplayListBuilder::DisplayListBuildingDisplayPortRect());

Can you add a comment that explains the life cycle of these properties?
Attachment #8913532 - Flags: review?(mstange) → review+
Comment on attachment 8913533 [details]
Bug 1404181 - Part 22: Make sure we mark frames as modified any time they change position or style data and make sure we don't accidentally mark the root as being modified when we don't need to.

https://reviewboard.mozilla.org/r/184902/#review196548

::: dom/animation/KeyframeEffectReadOnly.cpp:977
(Diff revision 3)
>        EffectSet::GetOrCreateEffectSet(mTarget->mElement, mTarget->mPseudoType);
>      effectSet->AddEffect(*this);
>      mInEffectSet = true;
>      UpdateEffectSet(effectSet);
> +    if (mTarget->mElement->GetPrimaryFrame()) {
> +      mTarget->mElement->GetPrimaryFrame()->MarkNeedsDisplayItemRebuild();

What about continuation frames?
Attachment #8913533 - Flags: review?(mstange) → review+
Comment on attachment 8913534 [details]
Bug 1404181 - Part 23: Only rebuild items within a displayport when the displayport changes, rather than rebuilding the whole document.

https://reviewboard.mozilla.org/r/184904/#review196550

::: dom/base/nsDOMWindowUtils.cpp:477
(Diff revision 3)
> +  nsLayoutUtils::InvalidateForDisplayPortChange(content, !!currentData,
> +    currentData->mRect, displayport);

Either !!currentData is always true or currentData->mRect can crash.
Attachment #8913534 - Flags: review?(mstange) → review+
Comment on attachment 8913535 [details]
Bug 1404181 - Part 24: Rebuild all display items when we encounter a blend mode, since we can't easily track changes to whether we need the blend container or not.

https://reviewboard.mozilla.org/r/184906/#review196552

::: layout/generic/nsFrame.cpp:2780
(Diff revision 3)
> +    // to the stacking context frame, or the blend mode frame (e.g. by moving
> +    // an intermediate frame).
> +    // When we gain/remove a blend container item, we need to mark this frame
> +    // as invalid and have the full display list for merging to track
> +    // the change correctly.
> +    // It seems really hard to track this in advanced, as the bookkeeping

advanced -> advance

::: layout/generic/nsIFrame.h:4317
(Diff revision 3)
>     * True if frame has will-change, and currently has display
>     * items consuming some of the will-change budget.
>     */
>    bool mMayHaveWillChangeBudget : 1;
>  
> +  bool mBuiltBlendContainer : 1;

need to adjust the comment about the size of the hole again
Attachment #8913535 - Flags: review?(mstange) → review+
Comment on attachment 8913536 [details]
Bug 1404181 - Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result.

https://reviewboard.mozilla.org/r/184908/#review196554

::: layout/generic/nsSubDocumentFrame.cpp:456
(Diff revision 3)
> +    // The value of needsOwnLayer can change between builds without
> +    // an invalidation recorded for this frame. If this happens,

Can you add an example of when this might happen?

::: layout/generic/nsSubDocumentFrame.cpp:468
(Diff revision 3)
> +    // We could just invalidate the old and new frame
> +    // areas and save some work here.

Isn't that what RetainedDisplayListBuilder does for the root document? Could it descend into subdocuments and do it for them as well?

::: layout/painting/nsDisplayList.cpp:1265
(Diff revision 3)
> +    // We are forcing a rebuild of nsDisplayCanvasBackgroundColor to make sure
> +    // that the canvas background color will be set correctly, and that only one
> +    // unscrollable item will be created.

It feels like we could be doing better here. But I suppose we can fix it up later.
Attachment #8913536 - Flags: review?(mstange) → review+
Comment on attachment 8913537 [details]
Bug 1404181 - Part 26: Skip DLBI for reused items (since they must not be invalid).

https://reviewboard.mozilla.org/r/184910/#review196556
Attachment #8913537 - Flags: review?(mstange) → review+
Comment on attachment 8913538 [details]
Bug 1404181 - Part 27: Add some retained-dl debugging tools.

https://reviewboard.mozilla.org/r/184912/#review196558
Attachment #8913538 - Flags: review?(mstange) → review+
Comment on attachment 8913532 [details]
Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list.

https://reviewboard.mozilla.org/r/184900/#review196514

> How about:
> 
> aFrames.swap(*frames);
> frames->clear();
> 
> instead of the loop

Going to leave this here, since Miko has a follow-up where we does the WeakFrame -> nsIFrame conversion at this point, and we need the loop.

> I forget: Does this also return true if any descendant frames have display items?
> 
> It looks like we're not rebuilding any part of the stacking context if the stacking context does not have display items. Why is that ok? Couldn't there be a case where the stacking context had no display items before, and then something inside the stacking context changed so that the stacking context needs display items now?

Not quite, it's that we only stop converting the dirty rect into ancestor coordinate space at this stacking context if we had an existing display item for that stacking context.

The idea is that we can do MarkFrameForDisplayIfVisible to rebuild the stacking context display item itself (which will merge into the old list without needing infront/behind items, since it'll just match to the same spot), and then we can use the override dirty area to build what actually changed within the stacking context.

If we don't have any existing display item for the stacking context, then we won't actually be recursing into the merge algorithm with the matched item, so we can't do this optimization. In this case we continue the outer loop, and keep bubbling up to the next stacking context.
Comment on attachment 8913536 [details]
Bug 1404181 - Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result.

https://reviewboard.mozilla.org/r/184908/#review196554

> Isn't that what RetainedDisplayListBuilder does for the root document? Could it descend into subdocuments and do it for them as well?

It could, but finding the set of to-be-painted subdocs can be a pain, and we have to do the equivalent of EnterPresShell to check if the caret frame changed.

I'll leave this for now, but add a comment explaining that we could do it there if it ever shows up as mattering.
Comment on attachment 8913524 [details]
Bug 1404181 - Part 15: Compute scrollbar thumb AGR state in advance, rather than setting it during building.

https://reviewboard.mozilla.org/r/184884/#review195086

> I think these changes are on mozilla-central now.

Indeed, dropped.
(In reply to Miko Mynttinen [:miko] from comment #115)
> (In reply to Markus Stange [:mstange] from comment #72)
> > Comment on attachment 8913519 [details]
> > Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that
> > can remove contributions from invalidated frames.
> > 
> > https://reviewboard.mozilla.org/r/184874/#review195048
> > 
> > ::: layout/painting/nsDisplayList.h:3694
> > (Diff revision 2)
> > >                              const nsRect& aBackgroundRect);
> > >    virtual ~nsDisplayThemedBackground();
> > >  
> > > +  void Destroy(nsDisplayListBuilder* aBuilder) override
> > > +  {
> > > +    aBuilder->UnregisterThemeGeometry(mFrame);
> > 
> > Maybe add a comment that it's fine to remove all theme geometries for the
> > frame, because there will never be more than one nsDisplayThemedBackground
> > per frame.
> > 
> > Actually, if that's the case, do we need to have arrays in the hashtable at
> > all?
> 
> The reason for using the array is because nsTreeBodyFrame might register
> more than one ThemeGeometry, one for each selected row.
> http://searchfox.org/mozilla-central/rev/
> d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame.
> cpp#2881
> 
> Currently though, I am not sure if any UI elements[1] using this code
> actually allow multiple selection.
> 
> [1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path=

It looks like when Destroy() is called, we delete all entries for that frame.

What happens when we build a second item (during a partial build), so that we have two alive, then run merging and call Destroy on one of them? Do we end up losing both copies?

Might need to tag entries with the display item that added them, and make sure we clean up only what's our.
Flags: needinfo?(mikokm)
Comment on attachment 8913529 [details]
Bug 1404181 - Part 18: Use nsPresArena for the display list builder arena since it supports recycling.

https://reviewboard.mozilla.org/r/184894/#review195104

> Would be nice to initialize this statically, maybe with some nsDisplayItemTypesList.h #include magic and constexpr?

That would be cool! I had a look, nsDisplayItemTypesList.h doesn't encode the actual class name, only the identifier, so there's no easy way to get sizeof() without rewriting that.
The power of 2 functions we use aren't constepxr either, and the implementation is pretty complex and platform dependent.
Comment on attachment 8913514 [details]
Bug 1404181 - Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames.

https://reviewboard.mozilla.org/r/184864/#review193722

> I don't know, I haven't thought through the alreadyHadRegions case.

I had more of a think about this. I think that if we previously had two touch-actions (and both were in the dispatch to content region), and then we had a partial update that removed one of them, we wouldn't remove the other.

That's probably not a big deal really, but it's easy to fix. I moved the combining into the final computation of DispatchToContentHitRegion() that adds the touch regions if there are multiple.
Attachment #8913521 - Attachment is obsolete: true
Attachment #8913524 - Attachment is obsolete: true
(In reply to Matt Woodrow (:mattwoodrow) from comment #127)
> (In reply to Miko Mynttinen [:miko] from comment #115)
> > (In reply to Markus Stange [:mstange] from comment #72)
> > > Comment on attachment 8913519 [details]
> > > Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that
> > > can remove contributions from invalidated frames.
> > > 
> > > https://reviewboard.mozilla.org/r/184874/#review195048
> > > 
> > > ::: layout/painting/nsDisplayList.h:3694
> > > (Diff revision 2)
> > > >                              const nsRect& aBackgroundRect);
> > > >    virtual ~nsDisplayThemedBackground();
> > > >  
> > > > +  void Destroy(nsDisplayListBuilder* aBuilder) override
> > > > +  {
> > > > +    aBuilder->UnregisterThemeGeometry(mFrame);
> > > 
> > > Maybe add a comment that it's fine to remove all theme geometries for the
> > > frame, because there will never be more than one nsDisplayThemedBackground
> > > per frame.
> > > 
> > > Actually, if that's the case, do we need to have arrays in the hashtable at
> > > all?
> > 
> > The reason for using the array is because nsTreeBodyFrame might register
> > more than one ThemeGeometry, one for each selected row.
> > http://searchfox.org/mozilla-central/rev/
> > d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame.
> > cpp#2881
> > 
> > Currently though, I am not sure if any UI elements[1] using this code
> > actually allow multiple selection.
> > 
> > [1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path=
> 
> It looks like when Destroy() is called, we delete all entries for that frame.
> 
> What happens when we build a second item (during a partial build), so that
> we have two alive, then run merging and call Destroy on one of them? Do we
> end up losing both copies?
> 
> Might need to tag entries with the display item that added them, and make
> sure we clean up only what's our.

I think you are right, good catch. Fixing this is probably just the matter of replacing nsIFrame key with nsDisplayItem key.
Flags: needinfo?(mikokm)
Comment on attachment 8913519 [details]
Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames.

https://reviewboard.mozilla.org/r/184874/#review195048

> Maybe add a comment that it's fine to remove all theme geometries for the frame, because there will never be more than one nsDisplayThemedBackground per frame.
> 
> Actually, if that's the case, do we need to have arrays in the hashtable at all?

As discussed in the bug, we do need multiple, and there's an issue with clearing them all at once.

We're going to fix this as a follow-up (since the issues don't affect us when preffed off), so that we can land before things bitrot.
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5584bf011ce4
Part 1: Make AnimatedGeometryRoot/ActiveScrolledRoot retained and refcounted on the heap. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ce1bfa06c117
Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a8d68ec7119c
Part 3: Track the current set of alive display items on nsIFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/defa442c4d89
Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again. r=mstange
https://hg.mozilla.org/integration/autoland/rev/bbca6b8fbbd7
Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames. r=mstange
https://hg.mozilla.org/integration/autoland/rev/16fd7be0e580
Part 6: Compute and store the frame that contributed the style data for backgrounds so that we can invalidate the display item when either frame changes. r=mstange
https://hg.mozilla.org/integration/autoland/rev/edb7dfb73a6c
Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7461fa8a43fa
Part 8: Add refcounting for clip chains. r=mstange
https://hg.mozilla.org/integration/autoland/rev/751f80bf76ab
Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists. r=mstange
https://hg.mozilla.org/integration/autoland/rev/87d9ba7f8891
Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames. r=mstange
https://hg.mozilla.org/integration/autoland/rev/952c5ec068bd
Part 11: Factor out PresShell paint count code se we can still call it without needing to build the display items. r=mstange
https://hg.mozilla.org/integration/autoland/rev/5a143355d1c8
Part 12: Add pref for retained display lists (starting with it disabled). r=mstange
https://hg.mozilla.org/integration/autoland/rev/5ae28672a397
Part 13: Expose DisplayItemData better from FrameLayerBuilder so we can query the geometry (for sync decode invalidations). r=mstange
https://hg.mozilla.org/integration/autoland/rev/6bd65bee0bf2
Part 14: Add a bunch of new helpers to display list builder and display item for retaining and reusing items. r=mstange
https://hg.mozilla.org/integration/autoland/rev/57299dc2cce3
Part 15: Add the notion of 'modified' frames, that need new display items built for them (and all their descendants). r=mstange
https://hg.mozilla.org/integration/autoland/rev/1dbd1a2eaba4
Part 16: Track window dragging area contributions per-frame so that can remove contributions from invalidated frames. r=mstange
https://hg.mozilla.org/integration/autoland/rev/2fb8ca01eaaf
Part 17: Track will-change contributions per-frame so that can remove contributions from invalidated frames. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6dcdda0d7977
Part 18: Use nsPresArena for the display list builder arena since it supports recycling. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4d21123bf063
Part 19: Some items don't use style data from mFrame, so add overrides that let us check the right frame when determining if an item is invalid. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7188aa2eacc9
Part 20: Add code to override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts. r=mstange
https://hg.mozilla.org/integration/autoland/rev/fce6bb594ee9
Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list. r=mstange
https://hg.mozilla.org/integration/autoland/rev/981efe443788
Part 22: Make sure we mark frames as modified any time they change position or style data and make sure we don't accidentally mark the root as being modified when we don't need to. r=mstange
https://hg.mozilla.org/integration/autoland/rev/484e07a716f1
Part 23: Only rebuild items within a displayport when the displayport changes, rather than rebuilding the whole document. r=mstange
https://hg.mozilla.org/integration/autoland/rev/42edf5487253
Part 24: Rebuild all display items when we encounter a blend mode, since we can't easily track changes to whether we need the blend container or not. r=mstange
https://hg.mozilla.org/integration/autoland/rev/90d2b2c704d5
Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result. r=mstange
https://hg.mozilla.org/integration/autoland/rev/0f8db6837b65
Part 26: Skip DLBI for reused items (since they must not be invalid). r=mstange
https://hg.mozilla.org/integration/autoland/rev/63c91c79579b
Part 27: Add some retained-dl debugging tools. r=mstange
Comment on attachment 8913513 [details]
Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again.

https://reviewboard.mozilla.org/r/184862/#review197026


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsTextFrame.cpp:4950
(Diff revision 5)
>    virtual ~nsDisplayText() {
>      MOZ_COUNT_DTOR(nsDisplayText);
>    }
>  #endif
>  
> +  virtual void RestoreState() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8913516 [details]
Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode.

https://reviewboard.mozilla.org/r/184868/#review197028


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsBulletFrame.cpp:191
(Diff revision 5)
>    {
>      nsBulletFrame* f = static_cast<nsBulletFrame*>(aItem->Frame());
>      mOrdinal = f->GetOrdinal();
>    }
>  
> +  virtual bool InvalidateForSyncDecodeImages() const override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: layout/generic/nsPluginFrame.cpp:919
(Diff revision 5)
>                                     const ContainerLayerParameters& aParameters) override
>    {
>      return LAYER_ACTIVE;
>    }
> +
> +  virtual nsDisplayItemGeometry* AllocateGeometry(nsDisplayListBuilder* aBuilder) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
https://hg.mozilla.org/mozilla-central/rev/5584bf011ce4
https://hg.mozilla.org/mozilla-central/rev/ce1bfa06c117
https://hg.mozilla.org/mozilla-central/rev/a8d68ec7119c
https://hg.mozilla.org/mozilla-central/rev/defa442c4d89
https://hg.mozilla.org/mozilla-central/rev/bbca6b8fbbd7
https://hg.mozilla.org/mozilla-central/rev/16fd7be0e580
https://hg.mozilla.org/mozilla-central/rev/edb7dfb73a6c
https://hg.mozilla.org/mozilla-central/rev/7461fa8a43fa
https://hg.mozilla.org/mozilla-central/rev/751f80bf76ab
https://hg.mozilla.org/mozilla-central/rev/87d9ba7f8891
https://hg.mozilla.org/mozilla-central/rev/952c5ec068bd
https://hg.mozilla.org/mozilla-central/rev/5a143355d1c8
https://hg.mozilla.org/mozilla-central/rev/5ae28672a397
https://hg.mozilla.org/mozilla-central/rev/6bd65bee0bf2
https://hg.mozilla.org/mozilla-central/rev/57299dc2cce3
https://hg.mozilla.org/mozilla-central/rev/1dbd1a2eaba4
https://hg.mozilla.org/mozilla-central/rev/2fb8ca01eaaf
https://hg.mozilla.org/mozilla-central/rev/6dcdda0d7977
https://hg.mozilla.org/mozilla-central/rev/4d21123bf063
https://hg.mozilla.org/mozilla-central/rev/7188aa2eacc9
https://hg.mozilla.org/mozilla-central/rev/fce6bb594ee9
https://hg.mozilla.org/mozilla-central/rev/981efe443788
https://hg.mozilla.org/mozilla-central/rev/484e07a716f1
https://hg.mozilla.org/mozilla-central/rev/42edf5487253
https://hg.mozilla.org/mozilla-central/rev/90d2b2c704d5
https://hg.mozilla.org/mozilla-central/rev/0f8db6837b65
https://hg.mozilla.org/mozilla-central/rev/63c91c79579b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1410876
Depends on: 1410904
Depends on: 1410906
Depends on: 1410908
Depends on: 1411248
Bugs with layout.display-list.retain true:
Black flickering screen instead of embedded video: https://vk.com/video-3156562_456239978
Black flickering screen instead of full screen video: https://www.youtube.com/watch?v=CM40b14i41M
Depends on: 1411411
(In reply to ajfhajf from comment #190)
> Bugs with layout.display-list.retain true:
> Black flickering screen instead of embedded video:
> https://vk.com/video-3156562_456239978

This seems to work fine in the graphics branch builds, so it should be fixed in central within the next few days.

> Black flickering screen instead of full screen video:
> https://www.youtube.com/watch?v=CM40b14i41M

Filed bug 1411411 for this issue, should be fixed soon (and just got fixed on graphics).
Depends on: 1411421
Depends on: 1411426
(In reply to Matt Woodrow (:mattwoodrow) from comment #191)
> This seems to work fine in the graphics branch builds, so it should be fixed
> in central within the next few days.
Thank you very much! There is one more bug: white flashes when watching video in fullscreen. STR:
Open this attachment in new tab, enable Full Screen
https://bug1265527.bmoattachments.org/attachment.cgi?id=8742491
Depends on: 1418790
Depends on: 1418797
Depends on: 1418807
Depends on: 1424348
Depends on: 1429314
Depends on: 1431147
Depends on: 1436572
Depends on: 1438165
Depends on: 1448818
Depends on: 1451477
Blocks: 1467514
Depends on: 1514291
No longer depends on: 1514291
Depends on: 1535862
Regressions: 1548397
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: