1896516 - Get rid of nsIScrollableFrame

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Moved from bug 1824877 comment 1: per https://phabricator.services.mozilla.com/D201893#6924920, we should consider removing nsIScrollableFrame since ScrollFrame is the only class implementing nsIScrollableFrame.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 6 - Rename IsScrollContainer() to IsScrollContainerOrSubclass(). r?#layout,dholbert,tnikkel

This is to avoid confusion with IsScrollContainerFrame() introduced in Part 3.

A note to performance concern:

We've used IsScrollContainerOrSubclass() in bug 1873414 to avoid expensive
nsIScrollableFrame* sf = do_QueryFrame(). Per [1], casting from nsIFrame* to
nsIScrollableFrame* has no fast path.

Ideally, ScrollContainerFrame* sf = do_QueryFrame() should be fast, and we can
use it to implement IsScrollContainerOrSubclass() like
IsImageFrameOrSubclass(). However, in this patch, I intend to keep the
performance metrics unchanged.

Remove GetAsScrollContainer() since it's only used in MOZ_ASSERT, and I'm
planning to remove nsIScrollableFrame in bug 1896516.

[1] https://searchfox.org/mozilla-central/rev/dd6e430c1bc2db90d9b3b1dd7e5215b4edc4d51a/layout/generic/nsQueryFrame.h#117-121

Comment 2

[Phabricator Automation]

Comment on attachment 9401802 [details]
Bug 1896516 Part 6 - Rename IsScrollContainer() to IsScrollContainerOrSubclass(). r?#layout,dholbert,tnikkel

Revision D210395 was moved to bug 1824877. Setting attachment 9401802 [details] to obsolete.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 1 - Rename PresShell::GetRootScrollFrame(), and make it return ScrollContainerFrame. r?#layout

In theory, changing the return type from nsIFrame* to ScrollContainerFrame*
exposes ScrollContainerFrame to the callers who might not needed, but almost
all of the callers in cpp files are already exposed to nsIScrollableFrame, as
demonstrated in this patch via replacing the #include from
"nsIScrollableFrame.h" to "ScrollContainerFrame.h", so this is OK.

Some callers can be simplified since we no longer need do_QueryFrame to
nsIScrollableFrame.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 2 - Sync the interface between nsIScrollableFrame and ScrollContainerFrame. r?#layout

We are going to switch callers from nsIScrollableFrame to
ScrollContainerFrame, so we need ScrollContainerFrame to have the same
interface as nsIScrollableFrame, including the same default values on params
and the same access privilege of public/protected/private APIs.

This patch is a preparation for later parts so that the code builds
successfully.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 3 - Change GetScrollTargetFrame() to return ScrollContainerFrame. r?#layout

Also, simplify some callers of GetScrollTargetFrame() to drop
nsIScrollableFrame* and unnecessary do_QueryFrame. We'll continue removing
more nsIScrollableFrame* in later parts.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 4 - Convert some nsIScrollableFrame usage in PresShell, nsLayoutUtils, and related helpers. r?#layout

This patch starts from renaming PresShell::GetScrollableFrameToScroll() to
PresShell::GetScrollContainerFrameToScroll(), making it return
ScrollContainerFrame*, and then fixing other helpers until everything
compiles. As before, we can remove some do_QueryFrame.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 5 - Fix build errors after including ScrollContainerFrame.h under dom/animation/. r?#layout

This is a preparation to remove #include "nsIScrollableFrame.h" for
ScrollTimeline.cpp and ViewTimeline.cpp.

Move the implementation of nsListControlFrame::GetMultiple() to cpp. Keeping
it inline requires adding #include "mozilla/dom/Element.h" in the header, and
I don't feel the method is hot enough to be inlined.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 6 - Remove nsIScrollableFrame usages in nsLayoutUtils. r?#layout

Convert nsIScrollableFrame to ScrollContainerFrame for all the APIs in
nsLayoutUtils, and then adapt other callers until everything compiles.

In nsLayoutUtils::CalculateBasicFrameMetrics()'s documentation,
s/ComputeFrameMetrics/ComputeScrollMetadata/ because the method was renamed in
https://hg.mozilla.org/mozilla-central/rev/cb2023f50288

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 7 - Remove PresShell::GetRootScrollFrameAsScrollable(). r?#layout

PresShell::GetRootScrollFrameAsScrollable() is equivalent to
PresShell::GetRootScrollContainerFrame().

In ScrollContainerFrame.h, DecideScrollableLayer() has two versions, one has
four parameters, and the other has five parameters with the fifth parameter
aDirtyRectHasBeenOverriden having a default value nullptr. When we switch
the caller from nsIScrollableFrame to ScrollContainerFrame, we need to
remove the default value for the fifth parameter to avoid ambiguity.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 8 - Remove nsIScrollableFrame usages under accessible/. r?#layout

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 9 - Remove nsIScrollableFrame usages under dom/. r?#layout

Also, in EventStateManager, convert ComputeScrollTarget() and
ComputeScrollTargetAndMayAdjustWheelEvent() to return ScrollContainerFrame*
so that we can drop more unnecessarily do_QueryFrame.

Due to removing nsIScrollableFrame forward declaration in DOM headers, we have
to add nsIScrollableFrame forward declaration temporarily in
nsMenuPopupFrame.h to make this patch compile, but we'll remove it once we
remove nsIScrollableFrame under layout/.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 10 - Remove nsIScrollableFrame usages under gfx/. r?#layout

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 11 - Remove nsIScrollableFrame usages under layout/, widget/, and toolkit/. r?#layout

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 12 - Tidy up ScrollContainerFrame's interface. r?#layout

The motivation of this patch is that we've now switched all the callers from
nsIScrollableFrame to ScrollContainerFrame, and it is better to audit the
access control of ScrollContainerFrame's interface, and I notice almost all
the member variables are public!

This patch shuffle the interface so that there are only one public, protected,
and private section, and all the member variables are moved to the private
section.

AutoMinimumScaleSizeChangeDetector and RemoveDisplayPortCallback need to
access member variables, so by declaring them under ScrollContainerFrame, they
have the privilege to do so.

Ideally, the caller only need to access what are already in
nsIScrollableFrame, so currently we might expose more methods than necessary.
We can audit that in a later part.

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 13 - Remove nsIScrollableFrame completely. r?#layout

This patch moves the documentation from nsIScrollableFrame.h to
ScrollContainerFrame.h. We keep methods in the original order as they were in
nsIScrollableFrame.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1896516 Part 14 - Minor adjustment to ScrollContainerFrame interfaces. r?#layout

Move nsIReflowCallback overridden methods to public section since
ScrollContainerFrame public inherits the interface.

Move reflow related helper methods to protected section.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea9ecb543e66
Part 1 - Rename PresShell::GetRootScrollFrame(), and make it return ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/a2d37b98273c
Part 2 - Sync the interface between nsIScrollableFrame and ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/b765169a7a8f
Part 3 - Change GetScrollTargetFrame() to return ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/2f0817331dbe
Part 4 - Convert some nsIScrollableFrame usage in PresShell, nsLayoutUtils, and related helpers. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/717dac08b607
Part 5 - Fix build errors after including ScrollContainerFrame.h under dom/animation/. r=layout-reviewers,boris
https://hg.mozilla.org/integration/autoland/rev/42e1bbe0ecb6
Part 6 - Remove nsIScrollableFrame usages in nsLayoutUtils. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/7b481b747b7a
Part 7 - Remove PresShell::GetRootScrollFrameAsScrollable(). r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/c964fccd5180
Part 8 - Remove nsIScrollableFrame usages under accessible/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/997c9249dbed
Part 9 - Remove nsIScrollableFrame usages under dom/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/6f7ab8adfa6e
Part 10 - Remove nsIScrollableFrame usages under gfx/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/f63b0c4335fe
Part 11 - Remove nsIScrollableFrame usages under layout/,  widget/, and toolkit/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/3c06f22da72b
Part 12 - Tidy up ScrollContainerFrame's interface. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/c8a6b0e526d6
Part 13 - Remove nsIScrollableFrame completely. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/2977ff81a23e
Part 14 - Minor adjustment to ScrollContainerFrame interfaces. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/fd6904338812
apply code formatting via Lando

Comment 18

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/ea9ecb543e66
https://hg.mozilla.org/mozilla-central/rev/a2d37b98273c
https://hg.mozilla.org/mozilla-central/rev/b765169a7a8f
https://hg.mozilla.org/mozilla-central/rev/2f0817331dbe
https://hg.mozilla.org/mozilla-central/rev/717dac08b607
https://hg.mozilla.org/mozilla-central/rev/42e1bbe0ecb6
https://hg.mozilla.org/mozilla-central/rev/7b481b747b7a
https://hg.mozilla.org/mozilla-central/rev/c964fccd5180
https://hg.mozilla.org/mozilla-central/rev/997c9249dbed
https://hg.mozilla.org/mozilla-central/rev/6f7ab8adfa6e
https://hg.mozilla.org/mozilla-central/rev/f63b0c4335fe
https://hg.mozilla.org/mozilla-central/rev/3c06f22da72b
https://hg.mozilla.org/mozilla-central/rev/c8a6b0e526d6
https://hg.mozilla.org/mozilla-central/rev/2977ff81a23e
https://hg.mozilla.org/mozilla-central/rev/fd6904338812

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing scrolling crashes on macOS:
https://hg.mozilla.org/mozilla-central/rev/e1da1893a0c56d2752be4e6eb1efa16c58723a55

There are crashes with [@ mozilla::ScrollContainerFrame::ScrollbarActivityStarted] e.g. bp-ada62b65-ccc2-40d7-94c5-e9a910240529

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

It is Part 9 - Remove nsIScrollableFrame usages under dom/ that causes the regression (bug 1899505). See https://phabricator.services.mozilla.com/D211496#7279371 for the root cause.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8abd8c6ab1b6
Part 1 - Rename PresShell::GetRootScrollFrame(), and make it return ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/852253304683
Part 2 - Sync the interface between nsIScrollableFrame and ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/09981021ee35
Part 3 - Change GetScrollTargetFrame() to return ScrollContainerFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/0ee9d3315ed6
Part 4 - Convert some nsIScrollableFrame usage in PresShell, nsLayoutUtils, and related helpers. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/dd540ee6e824
Part 5 - Fix build errors after including ScrollContainerFrame.h under dom/animation/. r=layout-reviewers,boris
https://hg.mozilla.org/integration/autoland/rev/1eac71cfc6a0
Part 6 - Remove nsIScrollableFrame usages in nsLayoutUtils. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/1ad8f3d267ae
Part 7 - Remove PresShell::GetRootScrollFrameAsScrollable(). r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/9a67eb7c4248
Part 8 - Remove nsIScrollableFrame usages under accessible/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/35b0ec410b2d
Part 9 - Remove nsIScrollableFrame usages under dom/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/fa4d373ad546
Part 10 - Remove nsIScrollableFrame usages under gfx/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/2eac8c029b4b
Part 11 - Remove nsIScrollableFrame usages under layout/,  widget/, and toolkit/. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/7e98e19cb506
Part 12 - Tidy up ScrollContainerFrame's interface. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/e6f0167db347
Part 13 - Remove nsIScrollableFrame completely. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/fd11ccb1e883
Part 14 - Minor adjustment to ScrollContainerFrame interfaces. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/5eab81fcdd7c
apply code formatting via Lando

Comment 22

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8abd8c6ab1b6
https://hg.mozilla.org/mozilla-central/rev/852253304683
https://hg.mozilla.org/mozilla-central/rev/09981021ee35
https://hg.mozilla.org/mozilla-central/rev/0ee9d3315ed6
https://hg.mozilla.org/mozilla-central/rev/dd540ee6e824
https://hg.mozilla.org/mozilla-central/rev/1eac71cfc6a0
https://hg.mozilla.org/mozilla-central/rev/1ad8f3d267ae
https://hg.mozilla.org/mozilla-central/rev/9a67eb7c4248
https://hg.mozilla.org/mozilla-central/rev/35b0ec410b2d
https://hg.mozilla.org/mozilla-central/rev/fa4d373ad546
https://hg.mozilla.org/mozilla-central/rev/2eac8c029b4b
https://hg.mozilla.org/mozilla-central/rev/7e98e19cb506
https://hg.mozilla.org/mozilla-central/rev/e6f0167db347
https://hg.mozilla.org/mozilla-central/rev/fd11ccb1e883
https://hg.mozilla.org/mozilla-central/rev/5eab81fcdd7c