In bug 1420512, we generalized the ScrollThumbData struct to ScrollbarData, so that it can represent information about both scrollbar thumb layers and scrollbar container layers. As a follow-up simplification, we can get rid of the field nsDisplayOwnLayer::mScrollTarget, and use the mTargetViewId field of nsDisplayOwnLayer::mThumbData (which we'd want to rename to mScrollbarData or similar) instead.

(As part of this change, we'll need to make sure nsDisplayOwnLayer::mScrollbarData is actually populated for nsDisplayOwnLayer items representing scrollbar container layers.)

Hello, I would like to work on it.

Great, I assigned the bug to you! Here is the declaration of nsDisplayOwnLayer::mScrollTarget [1], and right below it is the mThumbData field that we'll want to rename to mScrollbarData. References to mScrollTarget can be replaced with references to mScrollbarData.mTargetViewId. Code that sets mScrollTarget needs be modified to set mScrollbarData instead, with the scroll target now going into mScrollbarData.mTargetViewId. I believe the only place where we set mScrollTarget is in the nsDisplayOwnLayer constructors; we can propagate the change through them by removing the constructor parameter for the scroll target, and update the places where the constructor is called. The places where the nsDisplayOwnLayer constructor is called with a scroll target are a bit hard to find, because the construction is through a helper function template called MakeDisplayItem, so I'll list them here: - AppendToTop() in nsGfxScrollFrame.cpp [2] This is the place where we construct nsDisplayOwnLayer objects that represent scrollbar container layers. - nsSliderFrame::BuildDisplayListForChildren() [3] This is the place where we construct nsDisplayOwnlayer objects that represent scrollbar thumb layers. Hopefully that's enough to get you started. If you have questions, feel free to ask! [1] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/painting/nsDisplayList.h#5549 [2] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/generic/nsGfxScrollFrame.cpp#3059 [3] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/xul/nsSliderFrame.cpp#461

Thanks for the detailed explanation, a patch is ready.

Comment on attachment 8967932 [details] [diff] [review] bug_1453469.patch Review of attachment 8967932 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks great! I have one suggestion for a small further improvement. ::: layout/painting/nsDisplayList.cpp @@ +6991,5 @@ > if (mFlags & nsDisplayOwnLayerFlags::eScrollbarContainer) { > ScrollDirection dir = (mFlags & nsDisplayOwnLayerFlags::eVerticalScrollbar) > ? ScrollDirection::eVertical > : ScrollDirection::eHorizontal; > + layer->SetScrollbarContainer(mScrollbarData.mTargetViewId, dir); There is a bit of further cleanup we could do here: we can populate mScrollbarData with the fields that SetScrollbarContainer() would set [1], and call SetScrollbarData() instead of SetScrollbarContainer() here. We can then remove Layer::SetScrollbarContainer() and LayerAttributes::SetScrollbarContainer(), since no one else calls them. Finally, to reflect the fact that Layer::SetScrollbarData() is now used not only for thumbs, we can revise this comment [2] like so: /** * CONSTRUCTION PHASE ONLY * If a layer is a scroll thumb container layer or scrollbar container * layer, set the scroll identifier of the scroll frame scrolled by * the scrolbar, and other data related to the scrollbar. */ [1] https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/gfx/layers/LayerAttributes.h#169 [2] https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/gfx/layers/Layers.h#1261 ::: layout/painting/nsDisplayList.h @@ +5654,1 @@ > // If this nsDisplayOwnLayer represents a scroll thumb layer, mThumbData Please revise this comment to reflect that we now use mScrollbarData for scrollbar containers layers too: // If this nsDisplayOwnLayer represents a scroll thumb layer or a // scrollbar container layer, mScrollbarData stores information // about the scrollbar. Otherwise, mScrollarData will be // default-constructed (in particular with mDirection == Nothing()) // and can be ignored.

Small improvements are added. Please review them.

Comment on attachment 8968325 [details] [diff] [review] bug_1453469_2.patch Review of attachment 8968325 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f66a5f9beb6b71708bcdcddda1e3a1d424f86b

(In reply to Botond Ballo [:botond] from comment #9) > Try push: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=73f66a5f9beb6b71708bcdcddda1e3a1d424f86b Looking good!

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6047ab9ac46a Remove nsDisplayOwnLayer::mScrollTarget. r=botond

Great, it was fast :) If you have something more on the table please just tell.

Thanks for your work here, and in bug 14205152! (In reply to szefoski22 from comment #12) > If you have something more on the table please just tell. If you're not tired of making cleanups related to scrollbar data, there is actually a bit more we can do :) We should wait for bug 1454485 to land first, as it's making some changes to the same code. Once that lands I'll file a new bug for what I have in mind, and cc you on it.

(In reply to Botond Ballo [:botond] from comment #13) > Thanks for your work here, and in bug 14205152! > > (In reply to szefoski22 from comment #12) > > If you have something more on the table please just tell. > > If you're not tired of making cleanups related to scrollbar data, there is > actually a bit more we can do :) > > We should wait for bug 1454485 to land first, as it's making some changes to > the same code. Once that lands I'll file a new bug for what I have in mind, > and cc you on it. Not tired at all! (In reply to Botond Ballo [:botond] from comment #25) > (In reply to Pulsebot from comment #24) > > Pushed by bballo@mozilla.com: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/a2866a322bbb > > Try unifying data structures for scrollbar container and scrollbar thumb > > info. r=botond > > I meant to edit that commit message to say: > > "Unify data structures for scrollbar container and scrollbar thumb info" > > since we're not just trying, we're actually doing it :) > > Oh well, too late now. Sorry for the regression, but at least "Try" as a part of the commit message seems to be correct now :D

(In reply to szefoski22 from comment #14) > (In reply to Botond Ballo [:botond] from comment #13) > > If you're not tired of making cleanups related to scrollbar data, there is > > actually a bit more we can do :) > > > > We should wait for bug 1454485 to land first, as it's making some changes to > > the same code. Once that lands I'll file a new bug for what I have in mind, > > and cc you on it. > > Not tired at all! Great :) I filed bug 1455182. > (In reply to Botond Ballo [:botond] from comment #25) > Sorry for the regression, No worries, I should have caught it during review of bug 1420512. > but at least "Try" as a part of the commit message > seems to be correct now :D Indeed :)