1094484 - Make LayerState decision of display items at DisplayListBuilder

Status: NEW

(Core :: Web Painting, defect)

Description

[Thinker Li [:sinker]]

For now, we make the decision of if an item would create an active or inactive layer while building layers.  And, for some items, GetLayerState() would travel descendant items.

This bug is for the work of refactoring code to make decisions at DisplayListBuilder, and avoid recursively checking LayerState of descendant items.

Comment 1

[Thinker Li [:sinker]]

Attachment: wip.diff

It is first workable patch.  It bases on #290f799b4f58 of m-c.  I am trying to do more tests.

Comment 2

[Thinker Li [:sinker]]

Attachment: wip-v2

Fix random crash and clip problem.

Comment 3

[Thinker Li [:sinker]]

Attachment: Make LayerState decisions at DisplayListBuilder

Comment 4

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #3)
> Created attachment 8520954 [details] [diff] [review]
> Make LayerState decisions at DisplayListBuilder

I will file a follow-up bug to remove duplications of code.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Thinker Li [:sinker] from comment #4)
> (In reply to Thinker Li [:sinker] from comment #3)
> > Created attachment 8520954 [details] [diff] [review]
> > Make LayerState decisions at DisplayListBuilder
> 
> I will file a follow-up bug to remove duplications of code.

I think the duplication removal should be an additional patch in this bug.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8520954 [details] [diff] [review]
Make LayerState decisions at DisplayListBuilder

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

I think we need to move/remove code in the same patches as the moved/new code, otherwise it's not clear how much duplication there's going to be.

We can break the patch up a bit; some ideas below.

::: layout/base/nsDisplayList.cpp
@@ +568,5 @@
>    nsCSSRendering::BeginFrameTreesLocked();
>    PR_STATIC_ASSERT(nsDisplayItem::TYPE_MAX < (1 << nsDisplayItem::TYPE_BITS));
> +
> +  {
> +    // See nsDisplayList::PaintForFrame().

Move the code from PaintForFrame to here. This can go in a separate patch.

@@ +708,5 @@
> +                           const Matrix4x4* aTransform,
> +                           const ContainerLayerParameters& aIncomingScale,
> +                           LayerState aState,
> +                           ContainerLayerParameters& aOutgoingScale,
> +                           Matrix4x4& aOutBaseTransform)

Move this code from FrameLayerBuilder to nsDisplayListBuilder first, in a separate patch.

@@ +866,5 @@
> + * \see nsDisplayWrapList::mLayerStateChildren.
> + */
> +static LayerState
> +PredicateLayerState(nsDisplayListBuilder *aBuilder,
> +                    nsDisplayItem *aItem) {

Predict

@@ +939,5 @@
> +  }
> +
> +  setter->PrepareContainerParams(aBuilder,
> +                                 *aBuilder->CurrentContainerLayerParameters(),
> +                                 *aNewParameters);

PrepareContainerParams should be a virtual method on nsDisplayItem itself. then we don't need this helper function or the separate base class.

::: layout/base/nsDisplayList.h
@@ +547,5 @@
> +        mPrevLayerStateForChildren(aBuilder->mLayerStateChildren),
> +        mPrevTrackLayerState(aBuilder->mTrackingLayerState),
> +        mStopped(false) {
> +      if (IsNotRendering()) {
> +        return;

Seems to me we don't need to check IsNotRendering. It should be safe to just set and restore mLayerStateChildren unconditionally.

@@ +806,5 @@
> +   * item.
> +   */
> +  class AutoContainer;
> +  friend class AutoContainer;
> +  class AutoContainer {

AutoContainerParams

@@ +838,5 @@
> +   * nsDisplayListBuilder before building display items.
> +   */
> +  class AutoContainerParams;
> +  friend class AutoContainerParams;
> +  class AutoContainerParams {

AutoRootContainerParams

@@ +846,5 @@
> +      : mSavedParams(aBuilder->CurrentContainerLayerParameters())
> +      , mBuilder(aBuilder)
> +      , mInit(true)
> +    {
> +      aBuilder->SetContainerParameters(aParams);

Seems like users of this class can just call aBuilder->SetContainerParameters directly instead of using this class ... since we don't need to restore state.

@@ +857,5 @@
> +      Restore();
> +    }
> +
> +    void Restore() {
> +      mBuilder->SetContainerParameters(mSavedParams);

We don't actually need to restore here.

@@ +961,5 @@
> +  LayerState                     mLayerStateChildren;
> +  nsRefPtr<LayerManager>         mLayerManager;
> +  nsRefPtr<LayerManager>         mInactiveContainerLayerManager;
> +  const ContainerLayerParameters* mContainerParameters;
> +  Matrix4x4                      mContainerBaseTransform;

Better document all these

@@ +984,5 @@
>    // True when the first async-scrollable scroll frame for which we build a
>    // display list has a display port. An async-scrollable scroll frame is one
>    // which WantsAsyncScroll().
>    bool                           mHaveScrollableDisplayPort;
> +  bool                           mTrackingLayerState;

Do we really need this? I hope we don't!

@@ +985,5 @@
>    // display list has a display port. An async-scrollable scroll frame is one
>    // which WantsAsyncScroll().
>    bool                           mHaveScrollableDisplayPort;
> +  bool                           mTrackingLayerState;
> +  bool                           mRootLayerManagerRetaining;

Call this IsBuildingRetainedLayers to match FrameLayerBuilder

::: layout/base/nsLayoutUtils.cpp
@@ +2990,5 @@
>                                                         frameDirty, &list);
>      }
>    }
>  
> +  // Remmove ContainerLayerParameters of the frame of root container

"Remove"

::: layout/generic/nsFrame.cpp
@@ +2002,5 @@
> +  nsDisplayTransform *transformItem = nullptr;
> +  if (isTransformed && !Preserves3DChildren()) {
> +    transformItem =
> +      new (aBuilder) nsDisplayTransform(aBuilder, this, dirtyRect);
> +  }

We shouldn't need to build the transform display item here.

::: xpcom/base/nsCondVar.h
@@ +15,5 @@
> + * the heap even for locally use.  With this template, an instance on
> + * the frame of caller function could be constructed conditional.
> + */
> +template<class T>
> +class nsCondVar {

Isn't this just mozilla::Maybe?

Comment 7

[Markus Stange [:mstange]]

(In reply to Thinker Li [:sinker] from comment #0)
> For now, we make the decision of if an item would create an active or
> inactive layer while building layers.  And, for some items, GetLayerState()
> would travel descendant items.
> 
> This bug is for the work of refactoring code to make decisions at
> DisplayListBuilder, and avoid recursively checking LayerState of descendant
> items.

What are the advantages of doing it this way?

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.