Open
Bug 1094484
Opened 10 years ago
Updated 2 years ago
Make LayerState decision of display items at DisplayListBuilder
Categories
(Core :: Web Painting, defect)
Core
Web Painting
Tracking
()
NEW
People
(Reporter: sinker, Unassigned)
References
Details
Attachments
(2 files, 1 obsolete file)
98.22 KB,
patch
|
Details | Diff | Splinter Review | |
86.77 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
It is first workable patch. It bases on #290f799b4f58 of m-c. I am trying to do more tests.
Reporter | ||
Comment 2•10 years ago
|
||
Fix random crash and clip problem.
Reporter | ||
Comment 3•10 years ago
|
||
Attachment #8520954 -
Flags: review?(roc)
Reporter | ||
Comment 4•10 years ago
|
||
(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.
(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 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?
Depends on: 1097464
Comment 7•9 years ago
|
||
(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?
Attachment #8520954 -
Flags: review?(roc)
Comment 8•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: thinker.li → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Component: Layout → Web Painting
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•