Move more fields from FrameMetrics to ScrollMetadata

RESOLVED FIXED in Firefox 49

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

({arch})

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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

2 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

2 years ago
Created attachment 8745133 [details]
MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats

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

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8e5625ec856
I'll review tomorrow, but I'm pretty sure mViewport can be removed entirely.
(Assignee)

Comment 5

2 years ago
There's a gtest failure I'll need to investigate.
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+
(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

2 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

2 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

2 years ago
Created attachment 8746175 [details]
MozReview Request: Bug 1267470 - Move more fields from FrameMetrics to ScrollMetadata. r=kats

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

2 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

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76aeaaf515e8
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

2 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.)
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

2 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

2 years ago
Attachment #8745133 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
^ That's the combined patch.

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d13359a3cc

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69d13359a3cc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.