Closed Bug 1420512 Opened 7 years ago Closed 7 years ago

Try unifying data structures for scrollbar container and scrollbar thumb info

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

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

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

bug_1420512_3.patch
(deleted), patch
botond
: review+
Details | Diff | Splinter Review 
Followup from https://bugzilla.mozilla.org/show_bug.cgi?id=1418387#c27, where Botond said:

====
::: gfx/layers/LayerAttributes.h:327
(Diff revision 3)
>    uint32_t mContentFlags;
>    float mOpacity;
>    bool mIsFixedPosition;
>    uint64_t mScrollbarTargetContainerId;
>    ScrollThumbData mThumbData;
> -  bool mIsScrollbarContainer;
> +  Maybe<ScrollDirection> mScrollbarContainerDirection;

It's a bit unfortunate to store the direction both here, and in mThumbData.

This could be cleaned up by generalizing ScrollThumbData to ScrollbarData, and adding an isScrollbarContainer flag to it.
====

Filing this bug to do this cleanup.
This would make a nice mentored bug.
Mentor: botond
Hi Botond,

I would like to get this one.

Please correct me if I'm wrong:
- struct 'ScrollThumbData' should be renamed to 'ScrollData' and all members of this struct also should be renamed, e.g. 'mThumbRatio'  to 'mRatio'
- 'Maybe<ScrollDirection> mScrollbarContainerDirection' member of 'SimpleLayerAttributes' should be removed and instead used 'mThumbData.mDirection'
- 'bool mIsScrollbarContainer' should be reverted
Flags: needinfo?(botond)
(In reply to szefoski22 from comment #2)
> I would like to get this one.

Thanks! I assigned the bug to you.

To provide a bit of background here:

  - A "layer" is an object in our rendering pipeline that
    represents a region of rendered data that can be
    moved around independently (without re-rendering)
    during composition.

  - SimpleLayerAttributes is a structure that aggregates
    attributes that are stored for all layers.

  - For the purposes of this bug, three kinds of layers
    are relevant:

      - Scrollbar thumb layers. These represent the
        scrollbar "thumb" that you can drag.

      - Scrollbar container layers. These represent an
        entire scrollbar, including the thumb, the
        track where the thumb moves, and any buttons
        at the ends.

      - All other kinds of layers.

  - There are currently three fields in SimpleLayerAttributes
    related to scrollbars:

      - mScrollbarTargetContainerId which is used for 
        both thumb and container layers

        (The "container" in this field's name doesn't
        refer to "scrollbar container", but something else
        which is largely historical. A better name today
        would be "mScrollbarTargetViewId".)

      - mThumbData which is only used for thumb layers

      - mScrollbarContainerDirection which is only used
        for scrollbar container layers

The duplication we're trying to fix here is that mThumbData also has an mDirection field; if we could use that, we wouldn't need mScrollbarContainerDirection.

To this end, I'm proposing to refactor ScrollThumbData so that it's used for both scrollbar thumb and scollbar container layers.

Now, on to the details:

> - struct 'ScrollThumbData' should be renamed to 'ScrollData' 

I would call it 'ScrollbarData', which is a bit more specific.

> and all members of this struct also should be renamed, e.g. 'mThumbRatio'  to 'mRatio'

Most of the members of the struct, except for 'mDirection', are still only used for thumb layers, so I would keep 'thumb' in the field names. If anything, I would rename 'mIsAsyncDraggable' to 'mThumbIsAsyncDraggable' for clarity.

> - 'Maybe<ScrollDirection> mScrollbarContainerDirection' member of
> 'SimpleLayerAttributes' should be removed and instead used
> 'mThumbData.mDirection'

Yep.

(The field 'mThumbData' should be renamed to 'mScrollbarData', to reflect its new type.)

> - 'bool mIsScrollbarContainer' should be reverted

You're right that we need a way to distinguish between thumb and container layers. (Previously, thumb layers would have an empty Maybe for mScrollbarContainerDirection, but they can't have that for mThumbData.mDirection).

Rather than reinstating the bool, I would suggest introducing an enum like this:

  enum class ScrollbarLayerType : uint8_t { None, Thumb, Container };

This way, we're very explicit about which kind of layer it is, instead of that information being implicit in the value of Maybe variables. The enum can be defined in LayersTypes.h.

As a final change, I would move 'mScrollbarTargetContainerId' into 'mScrollbarData', and rename it to just 'mTargetContainerId'. That way, all the scrollbar-related fields are stored in 'mScrollbarData'.
Assignee: nobody → szefoski22
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #3)
> As a final change, I would move 'mScrollbarTargetContainerId' into
> 'mScrollbarData', and rename it to just 'mTargetContainerId'.

In fact, as per:

>         (The "container" in this field's name doesn't
>         refer to "scrollbar container", but something else
>         which is largely historical. A better name today
>         would be "mScrollbarTargetViewId".)

might as well rename it to 'mTargetViewId'.
How is this coming along? Let me know if there's anything I can clarify / help with!
Hi Botond,

I had 2 weeks winter holidays so my interest was turned into different direction during this time, ski :)

But nevertheless...

I have dived into the source code. The most trivial part I have done what is rename struct 'ScrollThumbData' to 'ScrollData' what showed me many places where this structure is used. Probably I will have to change also instances/members name of this struct in many places, but this I will left for later.

Currently I'm checking how to use only one instance of Maybe<ScrollDirection> from 'mThumbData.mDirection' for 'SimpleLayerAttributes' when it acts as a thumb or as a container. And with that I have a lot of fun so far. At the beginning it looked trivial but when I have just simply redirected writing the 'ScrollDirection' value into 'mThumbData' member when 'SimpleLayerAttributes' acts as a container I have noticed some bad side effects. 
I have been looking for a root cause and I have found an unexpected for me 'shortcut' in 'nsDisplayList.cpp':

bool
nsDisplayOwnLayer::IsScrollThumbLayer() const
{
  return mThumbData.mDirection.isSome();
}


When I use the 'mDirection' from 'mThumbData' to store information for container type then 'nsDisplayOwnLayer::IsScrollThumbLayer()' tells to the world that this is thumb layer and bad things happens.

So I have checked in https://bugzilla.mozilla.org/show_bug.cgi?id=1418387 how this method looked before. But I assume we should not go back to the previous implementation of this method. Instead of that should be introduced and used mentioned 'enum class ScrollbarLayerType : uint8_t { None, Thumb, Container };'. You have mentioned about this:

> This way, we're very explicit about which kind of layer it is, instead of that information being implicit in the value of Maybe variables. The enum can be defined in LayersTypes.h.

But I didn't understand it before, I just had learn that by myself to understand it.

Currently I see this enum can not be a member of 'SimpleLayerAttributes' but should be member of 'ScrollData' (old 'ScrollThumbData') because only 'ScrollData' is used in many places to set meaning what kind of layer it is without overhead of 'SimpleLayerAttributes'.


I have to play with this task longer to learn more about dependencies there because it touches many sensitive functionality areas. So far it is very interesting journey!

I hope I have clarified my current intentions, if you have feeling that I'm going in a wrong way please let me know!
Flags: needinfo?(botond)
Thanks for the update!

(In reply to szefoski22 from comment #6)
> I have dived into the source code. The most trivial part I have done what is
> rename struct 'ScrollThumbData' to 'ScrollData' what showed me many places
> where this structure is used. 

Just a reminder, in comment 3 I requested that we name it 'ScrollbarData', to be more accurate.

> I have been looking for a root cause and I have found an unexpected for me
> 'shortcut' in 'nsDisplayList.cpp':
> 
> bool
> nsDisplayOwnLayer::IsScrollThumbLayer() const
> {
>   return mThumbData.mDirection.isSome();
> }
> 
> 
> When I use the 'mDirection' from 'mThumbData' to store information for
> container type then 'nsDisplayOwnLayer::IsScrollThumbLayer()' tells to the
> world that this is thumb layer and bad things happens.
> 
> So I have checked in https://bugzilla.mozilla.org/show_bug.cgi?id=1418387
> how this method looked before. But I assume we should not go back to the
> previous implementation of this method. Instead of that should be introduced
> and used mentioned 'enum class ScrollbarLayerType : uint8_t { None, Thumb,
> Container };'. 

Yeah, that's what I had in mind.

> Currently I see this enum can not be a member of 'SimpleLayerAttributes' but
> should be member of 'ScrollData' (old 'ScrollThumbData') because only
> 'ScrollData' is used in many places to set meaning what kind of layer it is
> without overhead of 'SimpleLayerAttributes'.

From the point of view of overhead (assuming you mean memory usage), it doesn't matter whether the enum is stored in ScrollbarData or SimpleLayerAttributes, since SimpleLayerAttributes stores an instance of ScrollbarData, so it will take up that space for all layers anyways.

> I have to play with this task longer to learn more about dependencies there
> because it touches many sensitive functionality areas. So far it is very
> interesting journey!

Glad to hear you're finding it interesting :)

> I hope I have clarified my current intentions, if you have feeling that I'm
> going in a wrong way please let me know!

It seems like you're on the right track! If you run into any issues, feel free to ask.
Flags: needinfo?(botond)
Hello,

I'm sorry for the confusion (again), it is 'ScrollbarData' not 'ScrollData' (I always had in maind 'ScrollbarData', never 'ScrollData').
Attached patch bug_1420512.patch (obsolete) (deleted) — — Splinter Review 
Please check this initial proposition.
Attachment #8958958 - Flags: review?(botond)
Thanks for the patch!

I'm at a conference this week and probably won't be able to look at this until I get back, but I will have a look as soon as I can (likely early next week).
Comment on attachment 8958958 [details] [diff] [review]
bug_1420512.patch

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

Thanks - this generally looks very good!

I have a few minor comments, mostly about changing relevant method names from ScrollThumbData -> ScrollbarData to go along with the change to the type name, and some suggestions for added simplification.

::: gfx/layers/LayerAttributes.h
@@ +40,5 @@
>      , mScrollTrackStart(aScrollTrackStart)
>      , mScrollTrackLength(aScrollTrackLength)
>    {}
>  
>    Maybe<ScrollDirection> mDirection;

It may be worth adding a comment stating the invariant that mDirection will contain a direction if mScrollbarLayerType is Thumb or Container, and will be empty otherwise.

@@ +122,5 @@
>      }
>      mIsFixedPosition = aFixedPosition;
>      return true;
>    }
> +  bool SetScrollThumbData(const ScrollbarData& aScrollbarData) {

Rename to SetScrollbarData()

@@ +247,5 @@
>    }
>    bool IsFixedPosition() const {
>      return mIsFixedPosition;
>    }
> +  FrameMetrics::ViewID TargetViewId() const {

As this is a method of SimpleLayerAttributes, not ScrollbarData, we should name it ScrollbarTargetViewId(), so it's clear that it pertains to the scrollbar.

@@ +253,2 @@
>    }
> +  const ScrollbarData& ThumbData() const {

Rename to ScrollbarData()

@@ +256,3 @@
>    }
>    Maybe<ScrollDirection> GetScrollbarContainerDirection() const {
> +    return mScrollbarData.mDirection;

In this function, we only want to return a direction for scrollbar container layers.

This implementation will also return a direction for thumb layers.

We need something like:

  return (mScrollbarData.mScrollbarLayerType == ScrollbarLayerType::Container)
      ? mScrollbarData.mDirection
      : Nothing();

::: gfx/layers/LayerMetricsWrapper.h
@@ +409,5 @@
>      }
>      return EventRegionsOverride::NoOverride;
>    }
>  
> +  const ScrollbarData& GetScrollThumbData() const

Rename to GetScrollbarData()

::: gfx/layers/Layers.h
@@ +1293,5 @@
>     * If a layer is a scroll thumb container layer, set the scroll identifier
>     * of the scroll frame scrolled by the thumb, and other data related to the
>     * thumb.
>     */
> +  void SetScrollThumbData(const ScrollbarData& aThumbData)

Rename to SetScrollbarData().

@@ +1369,5 @@
>    int32_t GetFixedPositionSides() { return mSimpleAttrs.FixedPositionSides(); }
>    FrameMetrics::ViewID GetStickyScrollContainerId() { return mSimpleAttrs.StickyScrollContainerId(); }
>    const LayerRect& GetStickyScrollRangeOuter() { return mSimpleAttrs.StickyScrollRangeOuter(); }
>    const LayerRect& GetStickyScrollRangeInner() { return mSimpleAttrs.StickyScrollRangeInner(); }
> +  FrameMetrics::ViewID GetTargetViewId() { return mSimpleAttrs.TargetViewId(); }

As with the SimpleLayerAttributes method, we should name this GetScrollbarTargetViewId().

@@ +1370,5 @@
>    FrameMetrics::ViewID GetStickyScrollContainerId() { return mSimpleAttrs.StickyScrollContainerId(); }
>    const LayerRect& GetStickyScrollRangeOuter() { return mSimpleAttrs.StickyScrollRangeOuter(); }
>    const LayerRect& GetStickyScrollRangeInner() { return mSimpleAttrs.StickyScrollRangeInner(); }
> +  FrameMetrics::ViewID GetTargetViewId() { return mSimpleAttrs.TargetViewId(); }
> +  const ScrollbarData& GetScrollThumbData() const { return mSimpleAttrs.ThumbData(); }

This method should be renamed to GetScrollbarData().

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +122,5 @@
>  
>  bool
>  HitTestingTreeNode::IsScrollbarNode() const
>  {
>    return mScrollbarContainerDirection.isSome() || IsScrollThumbNode();

With mScrollbarContainerDirection removed (as I suggest below), this can be written instead as:

  return mScrollbarData.mScrollbarLayerType != layers::ScrollbarLayerType::None;

@@ +129,5 @@
>  ScrollDirection
>  HitTestingTreeNode::GetScrollbarDirection() const
>  {
>    MOZ_ASSERT(IsScrollbarNode());
> +  if (mScrollThumbData.mScrollbarLayerType == layers::ScrollbarLayerType::Thumb) {

This can be simplified to:

  return *(mScrollbarData.mDirection);

For good measure, we can add 

  MOZ_ASSERT(mScrollbarData.mDirection.isSome());

as well.

::: gfx/layers/apz/src/HitTestingTreeNode.h
@@ +149,5 @@
>    // use to move the thumb node to reflect async scrolling.
>    uint64_t mScrollbarAnimationId;
>  
>    // This is set for scroll thumb Container layers only.
> +  ScrollbarData mScrollThumbData;

We can actually make a couple of simplifications to HitTestingTreeNode as part of this change:

  - Rename mScrollThumbData to mScrollbarData.
    Revise the comment to say it's set for both thumb and container layers.

  - Remove mScrollbarContainerDirection.
    We can just use mScrollbarData.mDirection instead.

    The last parameter of HitTestingTreeNode::SetScrollbarData()
    can be removed as well, since its only purpose is to set
    mScrollbarContainerDirection.

::: gfx/layers/wr/WebRenderScrollData.h
@@ +75,5 @@
>    void SetReferentId(uint64_t aReferentId) { mReferentId = Some(aReferentId); }
>    Maybe<uint64_t> GetReferentId() const { return mReferentId; }
>  
> +  void SetScrollThumbData(const ScrollbarData& aData) { mScrollThumbData = aData; }
> +  const ScrollbarData& GetScrollThumbData() const { return mScrollThumbData; }

Rename these to Get/SetScrollbarData() as well

@@ +112,5 @@
>    EventRegions mEventRegions;
>    LayerIntRegion mVisibleRegion;
>    Maybe<uint64_t> mReferentId;
>    EventRegionsOverride mEventRegionsOverride;
> +  ScrollbarData mScrollThumbData;

And this to mScrollbarData

::: gfx/layers/wr/WebRenderScrollDataWrapper.h
@@ +284,5 @@
>      MOZ_ASSERT(IsValid());
>      return mLayer->GetEventRegionsOverride();
>    }
>  
> +  const ScrollbarData& GetScrollThumbData() const

Rename to GetScrollbarData()
Attachment #8958958 - Flags: review?(botond) → feedback+
(In reply to Botond Ballo [:botond] from comment #11)
> @@ +253,2 @@
> >    }
> > +  const ScrollbarData& ThumbData() const {
> 
> Rename to ScrollbarData()

Actually, in this particular case that won't work because you can't have a method with the same name as a type.

Let's call it GetScrollbarData() instead.
Attached patch bug_1420512_2.patch (obsolete) (deleted) — — Splinter Review 
Hello Botond,

Thanks for the remarks, I have updated the patch so it now include mentioned improvements and I have done a little bit polishing in LayerAttributes.h to be more consistence there. Additionally to have the patch valid I have it to fit to the latest changes on the repository.
Attachment #8958958 - Attachment is obsolete: true
Attachment #8964705 - Flags: review?(botond)
Comment on attachment 8964705 [details] [diff] [review]
bug_1420512_2.patch

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

Thanks for the updated patch!

It wasn't necessary to rename all of the SimpleLayerAttributes getters to start with Get, although I suppose it doesn't hurt either, and it keeps things consistent, so we might as well keep that change.

::: gfx/layers/LayerAttributes.h
@@ +44,5 @@
>      , mScrollTrackLength(aScrollTrackLength)
>    {}
>  
> +  /**
> +   * The mDirection contains a direction data if mScrollbarLayerType is Thumb

"contains a direction data" -> "contains a direction"

@@ +51,4 @@
>    Maybe<ScrollDirection> mDirection;
> +
> +  /**
> +   * Indicate what kind of data it is. All possibilities are defined in

"waht kind of data it is" -> "what kind of layer this data is for"

@@ +307,4 @@
>      return mMixBlendMode;
>    }
> +
> +  bool IsForceIsolatedGroup() const {

The getter on Layer is called GetForceIsolatedGroup(), so let's use that name here too.

::: layout/xul/nsSliderFrame.cpp
@@ +460,5 @@
>        aLists.Content()->AppendToTop(
>          MakeDisplayItem<nsDisplayOwnLayer>(aBuilder, this, &masterList, ownLayerASR,
>                                             flags, scrollTargetId,
> +                                           ScrollbarData{scrollDirection,
> +                                           layers::ScrollbarLayerType::Thumb,

Keep the remaining ScrollbarData constructor arguments aligned with the first.
Attachment #8964705 - Flags: review?(botond) → review+
I made the minor changes suggested in comment 14 locally (so you don't have to upload a new patch) and pushed the resulting patch to the Try server to make sure it's building on all platforms and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41ac086f791485a75830b20af9e7317e015c7727
(In reply to Botond Ballo [:botond] from comment #15)
> I made the minor changes suggested in comment 14 locally (so you don't have
> to upload a new patch) and pushed the resulting patch to the Try server to
> make sure it's building on all platforms and passing tests:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=41ac086f791485a75830b20af9e7317e015c7727

Looks like we have some code in AsyncCompositionManager.cpp [1] that is inside an |#ifdef MOZ_WIDGET_ANDROID|, and therefore only compiled on Android, which needs to be updated.

[1] https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/gfx/layers/composite/AsyncCompositionManager.cpp#781
Comment on attachment 8964705 [details] [diff] [review]
bug_1420512_2.patch

Clearing review flag until the Android issue is addressed.
Attachment #8964705 - Flags: review+
Hello Botond,

Thank you for the remarks!

(In reply to Botond Ballo [:botond] from comment #14)
> It wasn't necessary to rename all of the SimpleLayerAttributes getters to
> start with Get, although I suppose it doesn't hurt either, and it keeps
> things consistent, so we might as well keep that change.

So the main motivation to do it is a naming collision. The 'SimpleLayerAttributes' class has a variable 'mScrollbarData' and according to original 'Getter' methods naming conventions in this class it should have a name 'ScrollbarData()' but there is a struct 'ScrollbarData' with the same name already and compiler doesn't like it.
So that is why this proposition of changing all 'Getters' in 'SimpleLayerAttributes' class to have 'Get' prefix is there, to be consistent.
Attached patch bug_1420512_3.patch (deleted) — — Splinter Review 
Remarks applied.
Attachment #8964705 - Attachment is obsolete: true
Attachment #8966036 - Flags: review?(botond)
Looks good, thanks!

Here's a new Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48c2778ec9f060e10e19ef1fe72965ae50fa0a2
Comment on attachment 8966036 [details] [diff] [review]
bug_1420512_3.patch

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

Try push is looking good!
Attachment #8966036 - Flags: review?(botond) → review+
Rebased locally. Here's one more Try push to be safe:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1e18a5382b1d8adf9951ff065c1801aed68b1f
(In reply to Botond Ballo [:botond] from comment #22)
> Rebased locally. Here's one more Try push to be safe:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5b1e18a5382b1d8adf9951ff065c1801aed68b1f

Looks good as well.

I will submit the patch once mozilla-inbound is open.
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
(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.
It was a long run :)

Botond, if you have any suggestion for a next improvement for me it would be great, otherwise I can find something on https://www.joshmatthews.net/bugsahoy/?cpp=1.
https://hg.mozilla.org/mozilla-central/rev/a2866a322bbb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1453469
(In reply to szefoski22 from comment #26)
> Botond, if you have any suggestion for a next improvement for me it would be great

There's a bit more cleanup we can do as a follow-up to this change, for which I filed bug 1453469.

I thought of it as I was reviewing the changes to nsDisplayList.cpp in this bug, but I didn't want to hold up this bug by expanding its scope more.

You're welcome to work on that if you'd like!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: