Closed
Bug 1267470
Opened 8 years ago
Closed 8 years ago
Move more fields from FrameMetrics to ScrollMetadata
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
Details
(Keywords: arch, Whiteboard: [gfx-noted])
Attachments
(1 file, 1 obsolete file)
Bug 1219296 split a few fields of FrameMetrics into a ScrollMetadata class, which contains the FrameMetrics and the fields that were split out. The motivation is to avoid passing around all the fields everywhere. The layer tree (on both the client and compositor sides) contains the full ScrollMetadata, and APZ has a full copy of the ScrollMetadata as well. That leaves three places that only have access to FrameMetrics, which I'd like to keep that way: - Things called by the implementations of GeckoContentController:: RequestContentRepaint. - nsLayoutUtils::CalculateBasicFrameMetrics(). - The FrameMetrics copy that's shared via IPC between the compositor and content threads. With this in mind, there are several additional fields that can be moved to ScrollMetadata, and which I'd like to propose moving: mScrollParentId mBackgroundColor mContentDescription mLineScrollAmount mPageScrollAmount mHasScrollgrab mAllowVerticalScrollWithWheel mIsLayersIdRoot mUsesContainerScrolling mForceDisableApz There are also some fields which we _could_ move to ScrollMetadata, but which I'd like to propose keeping in FrameMetrics, because they "belong together" with other FrameMetrics fields that cannot be moved to ScrollMetadata: mCriticalDisplayPort (belongs together with mDisplayPort) mSmoothScrollOffset (belongs together with mScrollGeneration) mViewport (just feels like a "metrics" thing) mScrollUpdateType (belongs together with mScrollGeneration) mUseDisplayPortMargins (belongs together with mDisplayPortMargins)
Assignee | ||
Comment 1•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #0) > mContentDescription As a bonus, if we move this to ScrollMetadata we get to remove the MakePODObject() hack.
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48869/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48869/
Attachment #8745133 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8e5625ec856
Comment 4•8 years ago
|
||
I'll review tomorrow, but I'm pretty sure mViewport can be removed entirely.
Assignee | ||
Comment 5•8 years ago
|
||
There's a gtest failure I'll need to investigate.
Comment 6•8 years ago
|
||
Comment on attachment 8745133 [details] MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats https://reviewboard.mozilla.org/r/48869/#review45853 LGTM ::: gfx/layers/FrameMetrics.h:692 (Diff revision 1) > + , mLineScrollAmount(0, 0) > + , mPageScrollAmount(0, 0) These should get default-initialized to 0,0, so you should be able to skip this
Attachment #8745133 -
Flags: review?(bugmail.mozilla) → review+
Comment 7•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > (Diff revision 1) > > + , mLineScrollAmount(0, 0) > > + , mPageScrollAmount(0, 0) > > These should get default-initialized to 0,0, so you should be able to skip > this Whoops, ignore this comment. I realized later that it's good to leave it in.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > I'll review tomorrow, but I'm pretty sure mViewport can be removed entirely. It looks like mViewport is still used in NotifyLayersUpdated() to ensure that we change zoom correctly after a meta-viewport change (bug 973619).
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #5) > There's a gtest failure I'll need to investigate. This test failure caught a legitimate bug in the patch! The bug is that we short-circuit NotifyLayersUpdated() if the incoming metrics is equal to mLastContentPaintMetrics. However, some of the fields that are being moved to ScrollMetadata need to be included in this comparison, so we should be comparing ScrollMetadatas instead.
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49283/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49283/
Attachment #8746175 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 11•8 years ago
|
||
I will fold the second patch into the first, I'm just posting it separately to be reviewed more easily.
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76aeaaf515e8
Comment 13•8 years ago
|
||
Comment on attachment 8746175 [details] MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats https://reviewboard.mozilla.org/r/49283/#review46141 Cool, glad to see the tests are paying off :)
Attachment #8746175 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #8) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > > I'll review tomorrow, but I'm pretty sure mViewport can be removed entirely. > > It looks like mViewport is still used in NotifyLayersUpdated() to ensure > that we change zoom correctly after a meta-viewport change (bug 973619). Though, to be fair, that predates ZoomConstraintsClient, so maybe it's not necessary any more? (To be honest, I've mostly paged the ZoomConstraintsClient stuff out of my head, so I'm not sure off the top of my head.)
Comment 15•8 years ago
|
||
I've mostly paged it out as well. We can take a look at removing that next time we touch that code. For now what you have is still a good improvement.
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8746175 [details] MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49283/diff/1-2/
Attachment #8746175 -
Attachment description: MozReview Request: Bug 1267470 - Fix short-circuiting in NotifyLayersUpdated(). r=kats → MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats
Assignee | ||
Updated•8 years ago
|
Attachment #8745133 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
^ That's the combined patch.
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69d13359a3cc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•