1455182 - Additional cleanup to nsDisplayOwnLayer and ScrollbarData

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P3)

Description

[Botond Ballo [:botond]]

We've done some cleanups to the data structures used to represent scrollbars in the display list and layer / WebRender transactions in bug 1420512, bug 1453469, and bug 1454485, but there is more we could do.

Here are some suggestions:

  - For nsDisplayOwnLayer objects representing scrollbar
    container layers, we populate mScrollbarData's
    mScrollbarLayerType and mDirection fields in two
    different places ([1], [2]). We should just do it
    once, in the place where the nsDisplayOwnLayer
    is constructed [3].

  - After the above change is made, the only place
    where nsDisplayOwnLayerFlags::eScrollbarContainer
    is checked will be that one place where the
    nsDisplayOwnLayer for scrollbar containers is
    constructed, which is just below the only place
    where it's set [4]. That means this flag is no
    longer useful, we can just remove it.

  - Also, the only place where nsDisplayOwnLayerFlags::
    eVerticalScrollbar and eHorizontalScrollbar [5] are 
    used is in nsDisplayListBuilder::mCurrentScrollbarFlags [6].
    Moreover, mCurrentScrollbarFlags _only_ stores these
    flags (or the zero value, eNone), not the other flags
    in nsDisplayOwnLayerFlags.

    That means we could replace mCurrentScrollbarFlags
    with a Maybe<ScrollDirection>, and remove the
    eVerticalScrollbar and eHorizontalScrollbar flags from
    nsDisplayOwnLayerFlags as well.

    This will involve changes to the getter function [7] 
    and to AutoCurrentScrollbarInfoSetter [8], and the 
    places where they are used [9] [10].

  - Finally, since ScrollbarData is used to represent both
    thumb layers and scrollbar container layers, and some
    of the fields are only used for thumb layers, it might
    be nice to make this ScrollbarData constructor [11] 
    private, and instead write two static functions,
    ScrollbarData::ForThumb() and 
    ScrollbarData::ForScrollbarContainer(), which take
    only the necessary fields for each type, and call the
    private constructor.

    (The necessary fields for thumbs are all of them, and
    for scrollbar containers it's just the scrollbar layer type,
    direction, and view id. Note that the ForThumb() and
    ForScrollbarContainer() functions don't need to take the
    scrollbar layer type as an argument, since they know
    to pass ScrollbarLayerType::Thumb and ScrollbarContainer, 
    respectively.)

    We can then call ForThumb() in the place where we create
    the ScrollbarData for thumbs (recall, that's here [12]), 
    and ForScrollbarContainer() in the place where we create
    the ScrollbarData for scrollbar containers [3].

I know these descriptions are a bit more high-level than in previous bugs, so let me know if you'd like me to elaborate on any of them.

Also, feel free to make these changes one at a time, in separate patches, if that makes it easier. (All in one patch is also fine, it's up to you.)

[1] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.cpp#6992
[2] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.cpp#7053
[3] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/generic/nsGfxScrollFrame.cpp#3059
[4] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/generic/nsGfxScrollFrame.cpp#3056
[5] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.h#5578
[6] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.h#1979
[7] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.h#620
[8] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/painting/nsDisplayList.h#1354
[9] https://searchfox.org/mozilla-central/search?q=symbol:_ZNK20nsDisplayListBuilder24GetCurrentScrollbarFlagsEv&redirect=false
[10] https://searchfox.org/mozilla-central/search?q=symbol:_ZN20nsDisplayListBuilder30AutoCurrentScrollbarInfoSetterC1EPS_m22nsDisplayOwnLayerFlagsb&redirect=false
[11] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/gfx/layers/LayerAttributes.h#29
[12] https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/layout/xul/nsSliderFrame.cpp#463
[13]

Comment 1

[Botond Ballo [:botond]]

Note: the changes here depend on the changes in bug 1454485 which landed earlier today, so be sure to pull the latest mozilla-central to pick up those changes.

Comment 2

[zielas.daniel]

Hello,

I would like to work on it.

Comment 3

[Botond Ballo [:botond]]

Great :)

Let me know if you have any questions!

Comment 4

[Botond Ballo [:botond]]

Hi Daniel, just wanted to check in and see how things are going. I know this bug is a bit more involved than the previous ones and I didn't go into as much detail as before, so please let me know if there is something I can clarify or provide additional details about!

Comment 5

[zielas.daniel]

Hello Botond,

Explanation is good, everything is clear. I just was on a business trip.
In next couple of minutes the patch should land :)

Comment 6

[zielas.daniel]

Attachment: bug_1455182.patch

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8975364 [details] [diff] [review]
bug_1455182.patch

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

Thanks, this generally looks great! Just a couple of minor nits:

::: gfx/layers/LayerAttributes.h
@@ +28,1 @@
>    ScrollbarData(ScrollDirection aDirection,

As this constructor is only called for thumbs, we might as well omit the |aScrollbarLayerType| argument, and just do:

  mScrollbarLayerType(ScrollbarLayerType::Thumb)

in the constructor initializer list.

For greater clarity, we can add a comment that says this constructor is for thumbs.

@@ +44,5 @@
>      , mScrollTrackLength(aScrollTrackLength)
>      , mTargetViewId(aTargetViewId)
>    {}
>  
> +  ScrollbarData(Maybe<ScrollDirection> aDirection,

Likewise for this constructor which is used for scrollbar containers only.

::: layout/painting/nsDisplayList.cpp
@@ +7007,5 @@
>      BuildContainerLayerFor(aBuilder, aManager, mFrame, this, &mList,
>                             aContainerParameters, nullptr,
>                             FrameLayerBuilder::CONTAINER_ALLOW_PULL_BACKGROUND_COLOR);
>  
> +  if (IsScrollThumbLayer() || IsScrollbarContainer()) {

This is a nice simplification, and likewise in UpdateScrollData() below :)

::: layout/xul/nsSliderFrame.cpp
@@ +380,4 @@
>  
>      if (thumbGetsLayer) {
> +      const Maybe<ScrollDirection> scrollDirection = aBuilder->GetCurrentScrollbarDirection();
> +      MOZ_ASSERT(scrollDirection.isSome() && *scrollDirection != eEither);

We don't have an eEither value. Consequently, this line does not compile in a debug build.

We can just remove the second conjunct, checking for scrollDirection.isSome() is sufficient.

Comment 8

[zielas.daniel]

Attachment: bug_1455182_2.patch

Hello Botond,

Thanks for the feedback. A new patch is available.

Comment 9

[Botond Ballo [:botond]]

Looks good, thanks!

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

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8976440 [details] [diff] [review]
bug_1455182_2.patch

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

::: layout/generic/nsFrame.cpp
@@ +11369,5 @@
>          result |= CompositorHitTestInfo::eDispatchToContent;
>        }
>      }
> +
> +    MOZ_ASSERT(*scrollDirection == ScrollDirection::eHorizontal);

I'm getting an assertion failure on this line on startup (in a debug build).

Looking at it again, the assertion shouldn't be here, as we haven't checked for eVertical yet. It should remain in the 'else' branch which this patch removes.

Comment 11

[Botond Ballo [:botond]]

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

The Try push is also showing some static analysis failures:

/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/LayerAttributes.h:54:40: error: Type 'Maybe<mozilla::layers::ScrollDirection>' must not be used as parameter
  ScrollbarData(Maybe<ScrollDirection> aDirection,
                                       ^
/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/LayerAttributes.h:54:40: note: Please consider passing a const reference instead

/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/LayerAttributes.h:83:75: error: Type 'Maybe<mozilla::layers::ScrollDirection>' must not be used as parameter
  static ScrollbarData CreateForScrollbarContainer(Maybe<ScrollDirection> aDirection,
                                                                          ^
/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/LayerAttributes.h:83:75: note: Please consider passing a const reference instead

Looks like the static analysis wants us to pass Maybe objects by const reference rather than by value.

Comment 12

[zielas.daniel]

Hello,

Thanks for the remarks!

(In reply to Botond Ballo [:botond] from comment #10)
> Comment on attachment 8976440 [details] [diff] [review]
> bug_1455182_2.patch
> 
> Review of attachment 8976440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsFrame.cpp
> @@ +11369,5 @@
> >          result |= CompositorHitTestInfo::eDispatchToContent;
> >        }
> >      }
> > +
> > +    MOZ_ASSERT(*scrollDirection == ScrollDirection::eHorizontal);
> 
> I'm getting an assertion failure on this line on startup (in a debug build).
> 
> Looking at it again, the assertion shouldn't be here, as we haven't checked
> for eVertical yet. It should remain in the 'else' branch which this patch
> removes.

When I was on a way to revert the assert I have realized the original intention to it (together with the 'if' statement) was to be sure that other flags than directions are not set. So when we have migrated to the 'ScrollDirection' enum class we don't have to check it anymore because only the directions can be now represented by the 'scrollDirection' variable. What do you think about it?


(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Botond Ballo [:botond] from comment #9)
> > Try push:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=8a3ccf6065b6d85d67e4efa6664fc777575a52a3
> 
> The Try push is also showing some static analysis failures:
> 
> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/
> LayerAttributes.h:54:40: error: Type
> 'Maybe<mozilla::layers::ScrollDirection>' must not be used as parameter
>   ScrollbarData(Maybe<ScrollDirection> aDirection,
>                                        ^
> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/
> LayerAttributes.h:54:40: note: Please consider passing a const reference
> instead
> 
> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/
> LayerAttributes.h:83:75: error: Type
> 'Maybe<mozilla::layers::ScrollDirection>' must not be used as parameter
>   static ScrollbarData CreateForScrollbarContainer(Maybe<ScrollDirection>
> aDirection,
>                                                                           ^
> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/
> LayerAttributes.h:83:75: note: Please consider passing a const reference
> instead
> 
> Looks like the static analysis wants us to pass Maybe objects by const
> reference rather than by value.

Done.

Comment 13

[zielas.daniel]

Attachment: bug_1455182_3.patch

Comment 14

[Botond Ballo [:botond]]

(In reply to szefoski22 from comment #12)
> > ::: layout/generic/nsFrame.cpp
> > @@ +11369,5 @@
> > >          result |= CompositorHitTestInfo::eDispatchToContent;
> > >        }
> > >      }
> > > +
> > > +    MOZ_ASSERT(*scrollDirection == ScrollDirection::eHorizontal);
> > 
> > I'm getting an assertion failure on this line on startup (in a debug build).
> > 
> > Looking at it again, the assertion shouldn't be here, as we haven't checked
> > for eVertical yet. It should remain in the 'else' branch which this patch
> > removes.
> 
> When I was on a way to revert the assert I have realized the original
> intention to it (together with the 'if' statement) was to be sure that other
> flags than directions are not set. So when we have migrated to the
> 'ScrollDirection' enum class we don't have to check it anymore because only
> the directions can be now represented by the 'scrollDirection' variable.
> What do you think about it?

Agreed, there is no need for an assertion here any more.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8979392 [details] [diff] [review]
bug_1455182_3.patch

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

Looks good, thanks!

::: layout/generic/nsFrame.cpp
@@ +11300,5 @@
>      // Viewport frames are never event targets, other frames, like canvas frames,
>      // are the event targets for any regions viewport frames may cover.
>      return result;
>    }
> +  const uint8_t pointerEvents = StyleUserInterface()->GetEffectivePointerEvents(this);

In the future, please avoid making changes to unrelated lines. (It makes life more difficult for people using "hg annotate" / "git blame" to discover the reason a line of code was last changed.)

(There is no need to revert this change now, this is just a note for the future.)

Comment 16

[Botond Ballo [:botond]]

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

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> New Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fa47f553b87aef3e6315a44c458c05851a4646d1

Unfortunately, there are other places where the patch is passing Maybe<ScrollDirection> by value:

nsDisplayList.h:1367:57: error: Type 'nsDisplayListBuilder::MaybeScrollDirection' (aka 'Maybe<mozilla::layers::ScrollDirection>') must not be used as parameter

Comment 18

[Botond Ballo [:botond]]

By the way, Daniel, as you've now contributed a number of patches, you're welcome to apply for Level 1 commit access if you'd like (see [1] for details). That would allow you to push patches to Try yourself.

I'm happy to vouch for you if you decide to apply!

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 19

[zielas.daniel]

Attachment: bug_1455182_4.patch

Comment 20

[zielas.daniel]

(In reply to Botond Ballo [:botond] from comment #18)
> By the way, Daniel, as you've now contributed a number of patches, you're
> welcome to apply for Level 1 commit access if you'd like (see [1] for
> details). That would allow you to push patches to Try yourself.
> 
> I'm happy to vouch for you if you decide to apply!
> 
> [1] https://www.mozilla.org/en-US/about/governance/policies/commit/

Hello Botond,

It sounds great, for sure I would like to apply. I will study the "Commit Access Policy" right after my weekend trip.

Comment 21

[Botond Ballo [:botond]]

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

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8980493 [details] [diff] [review]
bug_1455182_4.patch

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

All right, passing Try now :)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764056f83105
Additional cleanup to nsDisplayOwnLayer and ScrollbarData. r=botond

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/764056f83105

Comment 25

[Botond Ballo [:botond]]

Thanks for your work here, Daniel! This, together with bug 1420512 and bug 1453469, was really useful cleanup to the code for representing scrollbars in the rendering pipeline.

I don't have any others bugs in this particular area to work on, but if you're interested in working on something else, let me know and I'll try to suggest something!

Comment 26

[zielas.daniel]

Thanks Botond,

Would be great to grab something! It is very enjoyable.