Closed Bug 1267470 Opened 8 years ago Closed 8 years ago

Move more fields from FrameMetrics to ScrollMetadata

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

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)
(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.
I'll review tomorrow, but I'm pretty sure mViewport can be removed entirely.
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.
(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).
(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.
I will fold the second patch into the first, I'm just posting it separately to be reviewed more easily.
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+
(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.
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
Attachment #8745133 - Attachment is obsolete: true
^ That's the combined patch.
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.

Attachment

General

Created:
Updated:
Size: