Closed
Bug 1454485
Opened 6 years ago
Closed 6 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)
Core
Panning and Zooming
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 | ||
Updated•6 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=486f70ea84cdd661cec8253c50f8dfb29769ed16 (the try push includes both patches even though only one is visible)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5be66f990aebaaa689cbd6cf09174ea87991917d has all the comments addressed and the android build fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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.
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/945d0f8a4594 https://hg.mozilla.org/mozilla-central/rev/7d1e0a7e3e13
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•