Closed Bug 1454485 Opened 2 years ago Closed 2 years ago

Remove redundant storage of scrollbar target scroll id, ensure the right fields are propagated for scrollbar containers with WR

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

Follow-up from bug 1451469 comment 18, we should be able to turn ScrollThumbInfo::mTargetGuid into just a layers id and use the ViewId from the mThumbData.mTargetViewId.

But, this implies that the call at [1] is storing duplicate information into the HitTestingTreeNode itself, because the GetScrollbarTargetContainerId() call just reads the target scrollid from the layer's ScrollbarData field, which is the same thing passed as the third argument in [1]. i.e. we pass the whole ScrollbarData as well as the target scroll id, which is redundant. So that can be trimmed as well.

But then looking further, it looks like the WebRender codepath uses a different storage mechanism for scrollbar container layers and that's broken, because the ScrollbarData in WebRenderLayerScrollData is going to be empty (it's not updated at [2]). So that needs fixing too.

There might more things that need fixing, this bug is to audit all the code and make sure it's good.

[1] https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/gfx/layers/apz/src/APZCTreeManager.cpp#866-868
[2] https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/layout/painting/nsDisplayList.cpp#7058-7059
Assignee: nobody → bugmail
The WR side of things might be a regression from bug 1420512. Implementing the change described at https://bugzilla.mozilla.org/show_bug.cgi?id=1453469#c6 will make the differences between WR and non-WR more obvious, and will make the fixing easier as well since we'll just need to make sure we populate the same ScrollbarData fields in both cases. We can then drop some of the redundant fields from WebRenderLayerScrollData.
Comment on attachment 8968579 [details]
Bug 1454485 - Remove redundant fields from WebRenderLayerScrollData and ensure the ScrollbarData is always correctly populated instead.

https://reviewboard.mozilla.org/r/237274/#review243088

Thanks! (I thought of this as a further simplification while reviewing bug 1453469, but failed to realize that the intermediate state was not actually correct.)
Attachment #8968579 - Flags: review?(botond) → review+
Comment on attachment 8968580 [details]
Bug 1454485 - Stop passing around the scroll view and container direction since it's already in the scrollbar data.

https://reviewboard.mozilla.org/r/237276/#review243090

::: gfx/layers/LayerAttributes.h:284
(Diff revision 1)
> -
>    const ScrollbarData& GetScrollbarData() const {
>      return mScrollbarData;
>    }
>  
>    Maybe<ScrollDirection> GetScrollbarContainerDirection() const {

Consider inlining this into Layer::IsScrollbarContainer(), which is the only remaining call site.

::: gfx/layers/Layers.h
(Diff revision 1)
>    LayerPoint GetFixedPositionAnchor() { return mSimpleAttrs.GetFixedPositionAnchor(); }
>    int32_t GetFixedPositionSides() { return mSimpleAttrs.GetFixedPositionSides(); }
>    FrameMetrics::ViewID GetStickyScrollContainerId() { return mSimpleAttrs.GetStickyScrollContainerId(); }
>    const LayerRectAbsolute& GetStickyScrollRangeOuter() { return mSimpleAttrs.GetStickyScrollRangeOuter(); }
>    const LayerRectAbsolute& GetStickyScrollRangeInner() { return mSimpleAttrs.GetStickyScrollRangeInner(); }
> -  FrameMetrics::ViewID GetScrollbarTargetViewId() { return mSimpleAttrs.GetScrollbarTargetViewId(); }

As your try build points out, this has a call site in Android-only code in AsyncCompositionManager.

::: gfx/layers/Layers.cpp:1819
(Diff revision 1)
>      aStream << " [is3DContextLeaf]";
>    }
> -  if (GetScrollbarContainerDirection().isSome()) {
> +  if (IsScrollbarContainer()) {
>      aStream << " [scrollbar]";
>    }
>    if (Maybe<ScrollDirection> thumbDirection = GetScrollbarData().mDirection) {

There is actually another (minor) regression from bug 1420512 here, which we might as well fix while we're at it: we're only supposed to enter this branch and print [vscrollbar=]/[hscrollbar=] for thumb layers, but since bug 1420512 we now do for scrollbar container layers as well. The condition should be revised to check |GetScrollbarData().mScrollbarLayerType == ScrollbarLayerType::Thumb| as well. (Feel free to add a ScrollbarData::IsThumb() helper.)
Attachment #8968580 - Flags: review?(botond) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e2b83f17ef35a581e9da4a398a6ce52cd9819317 -d 8f31c3153ea0: rebasing 458895:e2b83f17ef35 "Bug 1454485 - Remove redundant fields from WebRenderLayerScrollData and ensure the ScrollbarData is always correctly populated instead. r=botond"
merging layout/painting/nsDisplayList.cpp
warning: conflicts while merging layout/painting/nsDisplayList.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Oh, I had written the patches on top of inbound which is where bug 1453469 landed. So they need to land on inbound, or wait until bug 1453469 gets merged over to autoland.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/945d0f8a4594
Remove redundant fields from WebRenderLayerScrollData and ensure the ScrollbarData is always correctly populated instead. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1e0a7e3e13
Stop passing around the scroll view and container direction since it's already in the scrollbar data. r=botond
https://hg.mozilla.org/mozilla-central/rev/945d0f8a4594
https://hg.mozilla.org/mozilla-central/rev/7d1e0a7e3e13
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1455182
You need to log in before you can comment on or make changes to this bug.