Closed Bug 1453469 Opened 2 years ago Closed 2 years ago

Remove nsDisplayOwnLayer::mScrollTarget

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: botond, Assigned: zielas.daniel, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(botond)
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
Assignee: nobody → szefoski22
Flags: needinfo?(botond)
Attached patch bug_1453469.patch (obsolete) — Splinter Review
Attachment #8967932 - Flags: review?(botond)
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.
Attachment #8967932 - Flags: review?(botond) → feedback+
Small improvements are added. Please review them.
Attachment #8967932 - Attachment is obsolete: true
Attachment #8968325 - Flags: review?(botond)
Comment on attachment 8968325 [details] [diff] [review]
bug_1453469_2.patch

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

Looks good to me, thanks!
Attachment #8968325 - Flags: review?(botond) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/6047ab9ac46a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1455182
(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 :)
You need to log in before you can comment on or make changes to this bug.