Bug 60305 - Separate scrolling code out of RenderLayer
Summary: Separate scrolling code out of RenderLayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
: 215443 (view as bug list)
Depends on: 220705 220708 220715 220729 220851
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-05 15:12 PDT by Simon Fraser (smfr)
Modified: 2021-01-25 14:39 PST (History)
42 users (show)

See Also:


Attachments
Patch (24.58 KB, patch)
2011-05-05 17:27 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
WIP patch 1: rebaselined, cleaned-up, still some rough edges butcomments welcome. (31.53 KB, patch)
2011-12-15 09:34 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed refactoring 1: Introduce RenderLayerScrollableArea. (32.75 KB, patch)
2011-12-18 14:24 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed refactoring 2: Updated on ToT, tweaked the logic a bit. (34.79 KB, patch)
2012-04-09 15:19 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch (319.34 KB, patch)
2020-08-13 16:27 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (319.52 KB, patch)
2020-08-13 16:49 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (325.11 KB, patch)
2020-08-14 00:59 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (325.29 KB, patch)
2020-08-14 03:18 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (329.48 KB, patch)
2020-08-14 12:23 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (331.55 KB, patch)
2020-08-14 15:03 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (337.40 KB, patch)
2020-08-14 16:54 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (336.93 KB, patch)
2020-08-15 15:14 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (348.97 KB, patch)
2020-08-16 15:45 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (358.44 KB, patch)
2020-08-17 06:00 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v3 (358.43 KB, patch)
2020-08-17 06:45 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v4 (358.45 KB, patch)
2020-08-17 10:07 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (squashed for testing) (303.66 KB, patch)
2020-11-22 17:05 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (squashed for testing) (304.83 KB, patch)
2020-11-22 17:43 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v6 (squashed for testing) (303.83 KB, patch)
2020-11-26 01:55 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v8 (squashed for testing) (304.78 KB, patch)
2020-11-26 03:48 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v9 (squashed for testing) (313.46 KB, patch)
2020-11-26 03:59 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v10 (squashed for testing) (316.83 KB, patch)
2020-11-26 04:55 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v11 (squashed for testing) (319.26 KB, patch)
2020-11-26 06:27 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v12 (squashed for testing) (320.35 KB, patch)
2020-11-26 06:47 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v13 (squashed for testing) (320.37 KB, patch)
2020-11-26 09:16 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v14 (squashed for testing) (300.37 KB, patch)
2020-11-27 00:12 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v15 (squashed for testing) (300.60 KB, patch)
2020-11-27 01:06 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v16 (squashed for testing) (299.03 KB, patch)
2020-11-28 00:28 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v17 (squashed for testing) (293.52 KB, patch)
2020-12-11 15:32 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v18 (squashed for testing) (293.52 KB, patch)
2020-12-11 15:44 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v19 (288.83 KB, patch)
2021-01-05 02:02 PST, Nikolas Zimmermann
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch, v20 (290.90 KB, patch)
2021-01-05 02:34 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v21 (239.07 KB, patch)
2021-01-13 15:25 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v22 (239.09 KB, patch)
2021-01-13 15:44 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v23 (242.93 KB, patch)
2021-01-14 16:12 PST, Nikolas Zimmermann
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-05-05 15:12:12 PDT
Most RenderLayers are not created for scrolling reasons. RenderLayer should not inherit from ScrollableArea; rather, it should own one when necessary. This would significantly reduce RenderLayer bloat.
Comment 1 Simon Fraser (smfr) 2011-05-05 17:27:04 PDT
Created attachment 92512 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-05-05 17:27:41 PDT
Comment on attachment 92512 [details]
Patch

I haven't done extensive testing of this patch. Feel free to take it and do so.
Comment 3 Simon Fraser (smfr) 2011-05-05 17:53:05 PDT
Taking a safer approach via 60327 for now, but we still think it's a good direction to separate scrolling code out of RenderLayer.
Comment 4 Adam Barth 2011-05-06 01:17:46 PDT
Comment on attachment 92512 [details]
Patch

This patch causes many test failures:


Regressions: Unexpected text diff mismatch : (20)
  fast/block/float/marquee-shrink-to-avoid-floats.html = TEXT
  fast/css/MarqueeLayoutTest.html = TEXT
  fast/css/text-overflow-ellipsis-bidi.html = TEXT
  fast/css/text-overflow-ellipsis-strict.html = TEXT
  fast/css/text-overflow-ellipsis.html = TEXT
  fast/events/autoscroll-in-textfield.html = TEXT
  fast/forms/date-input-visible-strings.html = TEXT
  fast/forms/input-readonly-autoscroll.html = TEXT
  fast/forms/search-rtl.html = TEXT
  fast/html/marquee-scroll.html = TEXT
  fast/html/marquee-scrollamount.html = TEXT
  fast/inline-block/003.html = TEXT
  fast/repaint/intermediate-layout-position-clip.html = TEXT
  fast/repaint/line-in-scrolled-clipped-block.html = TEXT
  fast/repaint/text-selection-rect-in-overflow-2.html = TEXT
  fast/repaint/text-selection-rect-in-overflow.html = TEXT
  fast/table/colspanMinWidth-vertical.html = TEXT
  fast/transforms/scrollIntoView-transformed.html = TEXT
  inspector/console/console-command-clear.html = TEXT
  svg/custom/scroll-hit-test.xhtml = TEXT
Comment 5 Simon Fraser (smfr) 2011-05-06 08:25:41 PDT
Yeah, it wasn't meant for review.
Comment 6 Simon Fraser (smfr) 2011-05-20 12:35:51 PDT
Not an issue after fixing 42550.
Comment 7 Julien Chaffraix 2011-12-15 09:34:26 PST
Created attachment 119439 [details]
WIP patch 1: rebaselined, cleaned-up, still some rough edges butcomments welcome.
Comment 8 Julien Chaffraix 2011-12-18 14:24:54 PST
Created attachment 119780 [details]
Proposed refactoring 1: Introduce RenderLayerScrollableArea.
Comment 9 Julien Chaffraix 2012-04-09 15:19:45 PDT
Created attachment 136315 [details]
Proposed refactoring 2: Updated on ToT, tweaked the logic a bit.
Comment 10 Julien Chaffraix 2012-04-25 11:13:02 PDT
For the record, this was discussed briefly as part of the scrolling session at this year's contributors meeting (http://trac.webkit.org/wiki/Scrolling%20Session%20Meeting%202012).

Sam / Anders, I hope the talk answered your concerns over this patch and you can find some time to look at it. The idea is to move everything scrolling and resizing related to the new RenderLayerScrollableArea (maybe we can come up with a better naming for that though like ScrollingHandler)
Comment 11 Julien Chaffraix 2012-10-15 13:59:29 PDT
Comment on attachment 136315 [details]
Proposed refactoring 2: Updated on ToT, tweaked the logic a bit.

The patch won't apply anymore and I have switched to other tasks so clearing the review flag.
Comment 12 Antonio Gomes 2013-08-30 11:07:12 PDT
(In reply to comment #3)
> Taking a safer approach via 60327 for now, but we still think it's a good direction to separate scrolling code out of RenderLayer.

Simon/Julien. I would to take on this patch. Should it be fine?
Comment 13 Nikolas Zimmermann 2020-08-13 16:05:09 PDT
*** Bug 215443 has been marked as a duplicate of this bug. ***
Comment 14 Nikolas Zimmermann 2020-08-13 16:27:32 PDT
I'll upload newer revisions from the patch from bug 215443 here, per request on Simon. Good idea to re-use this much older ticket for this :-)

First need to get build running for all platforms, please bear with me.
Comment 15 Nikolas Zimmermann 2020-08-13 16:27:54 PDT
Created attachment 406553 [details]
Patch
Comment 16 Nikolas Zimmermann 2020-08-13 16:49:06 PDT
Created attachment 406558 [details]
Patch
Comment 17 Nikolas Zimmermann 2020-08-14 00:59:48 PDT
Created attachment 406578 [details]
Patch
Comment 18 Nikolas Zimmermann 2020-08-14 03:18:11 PDT
Created attachment 406581 [details]
Patch
Comment 19 Darin Adler 2020-08-14 10:53:22 PDT
Comment on attachment 406581 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406581&action=review

Seems like probably a great idea as long as it doesn’t add too much virtual function overhead. Love to hear what Simon thinks.

Is there some way to make this change in smaller steps, for example adding a scrollableLayer function that’s a synonym for layer first and deploying it while they still both return the same thing, so it’s easier to see the final step that splits the classes is correct? Even adding a scrollableLayer function to RenderLayer that returns "this" as a similar staging step. Could even add a RenderLayerScrollable.h header that just includes RenderLayer.h as a similar staging step.

> Source/WebCore/page/ios/FrameIOS.mm:824
> -    RenderLayer& layer = *downcast<RenderBoxModelObject>(*renderer).layer();
> +    auto* layer = downcast<RenderBoxModelObject>(*renderer).scrollableLayer();
> +    if (!layer)
> +        return;

I noticed that in some other cases we were able to assert rather than checking null. Not sure why here we had to check for null.

Could we add a hasScrollableLayer function to the renderer rather than adding this null check?
Comment 20 Nikolas Zimmermann 2020-08-14 12:23:44 PDT
Created attachment 406614 [details]
Patch
Comment 21 Nikolas Zimmermann 2020-08-14 15:03:10 PDT
Created attachment 406623 [details]
Patch
Comment 22 Simon Fraser (smfr) 2020-08-14 15:51:48 PDT
Comment on attachment 406623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406623&action=review

> Source/WebCore/editing/FrameSelection.cpp:2415
> +                auto* scrollableLayer = layer->scrollableLayer();

I think having scrollableLayer() on RenderLayer is confusing. I'd prefer to see this written as:

if (layer->isScrollable())
  auto& scrollableLayer = downcast<RenderLayerScrollable>(&layer).
Comment 23 Nikolas Zimmermann 2020-08-14 16:54:21 PDT
Created attachment 406636 [details]
Patch
Comment 24 Nikolas Zimmermann 2020-08-14 16:56:25 PDT
(In reply to Simon Fraser (smfr) from comment #22)
> 
> I think having scrollableLayer() on RenderLayer is confusing. I'd prefer to
> see this written as:
> 
> if (layer->isScrollable())
>   auto& scrollableLayer = downcast<RenderLayerScrollable>(&layer).

Hehe, I had that in my last variant :-) Will first try to get this to build everywhere, only iOS missing. Then will work on improving the readability - I agree, it really reads odd 'layer.scrollableLayer()'.
Comment 25 Nikolas Zimmermann 2020-08-15 15:14:19 PDT
Created attachment 406678 [details]
Patch
Comment 26 Nikolas Zimmermann 2020-08-16 15:45:24 PDT
Created attachment 406688 [details]
Patch
Comment 27 Nikolas Zimmermann 2020-08-17 06:00:01 PDT
Created attachment 406712 [details]
Patch, v2

Remove scrollableLayer() accessors, use downcast directly instead, as suggested by Simon
Comment 28 Nikolas Zimmermann 2020-08-17 06:45:10 PDT
Created attachment 406713 [details]
Patch, v3

Fix typo in DOMHTML.mm - should fix iOS/tv/watch builds again
Comment 29 Nikolas Zimmermann 2020-08-17 10:07:44 PDT
Created attachment 406724 [details]
Patch, v4

Fix build in WebFrame.mm - hopefully the last iOS/tv/watch specific issue
Comment 30 Said Abou-Hallawa 2020-08-17 10:11:06 PDT
Comment on attachment 406724 [details]
Patch, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=406724&action=review

After looking at some of the changes in this patch, I felt annoyed by seeing the same pattern:

Original:

    layer->doSomething();

New:

    if (is<RenderLayerScrollable>(layer)) {
        auto* scrollableLayer = downcast<RenderLayerScrollable>(layer);
        scrollableLayer->doSomething();
    }

RenderLayerScrollable is a superclass of RenderLayer, so why does not RenderLayer define doSomething() a virtual method and RenderLayerScrollable override it? I think this would leave most of the code unchanged. I understand the virtual method adds indirection calling overhead, but are not the if-statement and the casting add similar or even more expensive overhead as well?

class RenderLayer {
public:
    virtual void doSomething() { };
};

class ScrollArea {
public:
    void doSomething() { ... }
};

class RenderLayerSrollable : public RenderLayer, public ScrollArea {
public:
    void doSomething() override { ScrollArea::doSomething(); }
};

Maybe another approach is to make RenderLayer contain a pointer to ScrollableArea or a superclass of it, which Simon did in his original patch. In this case, no virtual table will be needed. But some of the calls to RenderLayer will be directed to ScrollableArea.

class ScrollArea {
public:
    void doSomething() { ... }
};

class RenderLayer {
public:
    void doSomething()
    {
        if (m_scrollableArea)
            m_scrollArea->doSomething();
    };
    
private:
    std::unique_ptr<RenderLayerScrollableArea> m_scrollableArea;
};

> Source/WebCore/rendering/RenderLayerScrollable.h:60
> +    friend class RenderLayer;

Why RenderLayer is the base of this class and a friend at the same time?
Comment 31 Nikolas Zimmermann 2020-08-17 14:31:04 PDT
(In reply to Said Abou-Hallawa from comment #30)

Thanks Said for the comments.

> Original:
> 
>     layer->doSomething();
> 
> New:
> 
>     if (is<RenderLayerScrollable>(layer)) {
>         auto* scrollableLayer = downcast<RenderLayerScrollable>(layer);
>         scrollableLayer->doSomething();
>     }
> 
> RenderLayerScrollable is a superclass of RenderLayer, so why does not
> RenderLayer define doSomething() a virtual method and RenderLayerScrollable
> override it? I think this would leave most of the code unchanged. I
> understand the virtual method adds indirection calling overhead, but are not
> the if-statement and the casting add similar or even more expensive overhead
> as well?

When I first split RenderLayer in two parts (RenderLayerHTML/SVG in the SVG PoC branch), I did just that: used virtual methods, avoiding all if(is<>) / downcast constructs.

While profiling the SVG PoC branch I found that by removing all virtual calls and employing the LayerType concept I could gain a speed-up - I can look up the profiles, I think it was a ~ 7% progression on MotionMark SVG suites compared to the SVG PoC branch before with virtual function calls.

The is<RenderLayerScrollable>() boils down to one inlined function call to access RenderLayer::layerType() and one comparison (== LayerType::Scrollable).
The downcast<> boils down to a static_cast, which is cheap (zero for single-inheritance relations, less higher for N base classes).

Anyhow, I did not perform the performance analysis on this branch so cannot proof that this is faster - I'd need to repeat the exercise, but I doubt it will be faster :-)

> 
> Maybe another approach is to make RenderLayer contain a pointer to
> ScrollableArea or a superclass of it, which Simon did in his original patch.
> In this case, no virtual table will be needed.
> But some of the calls to RenderLayer will be directed to ScrollableArea.

I wasn't aware of his patch when I started to work on this.
It boils down to: do we want a is-a or has-a relationship? Currently we assume that the RenderLayer is-a scrollable area, and I've adapted that.

I haven't explored has-a yet, maybe I'm overlooking other benefits.
 
> > Source/WebCore/rendering/RenderLayerScrollable.h:60
> > +    friend class RenderLayer;
> 
> Why RenderLayer is the base of this class and a friend at the same time?
Probably a leftover, will check.
Comment 32 Nikolas Zimmermann 2020-11-22 17:05:42 PST
Created attachment 414792 [details]
Patch, v5 (squashed for testing)

NOTE: Set r? to trigger EWS - don't know if that's necessary, not intended for a real review yet.

"Patch, v5" actually consists of four independent patches, that can be reviewed/landed separated.
I squashed them together to obtain EWS results first for the final patch, since I still expect regressions on Mac/Win etc. that need to be checked first. Once EWS shows no regressions, I'll start uploading the individual patch chunks for review. Feel free to check the patch nevertheless. It is based on the previous iterations + discussions after the meeting with Simon, Said, etc.

The four independent patches are:
1) Introduce RenderLayerScrollableArea stub (build system changes, etc.)
2) Move overflow/scrolling functionality from RenderLayer -> RenderLayerScrollableArea.
   Introduce temporary glue code: void RenderLayer::foo() { if (m_scrollableArea) m_scrollableArea->foo(); }
   to minimize the changes to WebCore in general and ease review.
3) Remove glue code, make WebCore aware of RenderLayerScrollableArea
4) Enable dynamic creation of RenderLayerScrollableArea, only when it is needed!
   This is the tricky part that kept me busy for a long while.

If a scrollable area is not needed for a RenderLayer, the RenderLayer will now consume less memory than before.
I measured this to be performance and memory-neutral on two benchmarks: MotionMark, Speedometer.

While the size reduction of RenderLayer is significant, it is negligible as soon as the layer needs a backing store...
Thus I expect this to have little impact on memory consumption in "real-life" use-cases. Nevertheless it simplifies the code a lot, and opens possibilities for further refactoring & cleanup that are the main motivation for this work: e.g. introduce RenderLayerHTML, further cleaning up RenderLayer.

The final goal is to make RenderLayer less expensive to use, to allow to be reused for SVG.
I have a PoC branch where SVG rendering/hit-testing/etc. goes through RenderLayer, which opens up nice possibilites, such as 3D transformations, GPU composition etc. However on static rendering it currently regresses performance, due to the RenderLayer cost.

Disclaimer: I just finished rebasing this on ToT -- it was ~ 2 months behind. Only ran a limited set of layout tests, as I want to get this patch out quickly to get some early results before EWS is shut-down due to the maintenance tasks this week.
.
Comment 33 Nikolas Zimmermann 2020-11-22 17:43:15 PST
Created attachment 414793 [details]
Patch, v5 (squashed for testing)
Comment 34 Nikolas Zimmermann 2020-11-26 01:55:51 PST
Created attachment 414887 [details]
Patch, v6 (squashed for testing)
Comment 35 Nikolas Zimmermann 2020-11-26 03:48:17 PST
Created attachment 414891 [details]
Patch, v8 (squashed for testing)
Comment 36 Nikolas Zimmermann 2020-11-26 03:59:44 PST
Created attachment 414893 [details]
Patch, v9 (squashed for testing)
Comment 37 Nikolas Zimmermann 2020-11-26 04:55:37 PST
Created attachment 414894 [details]
Patch, v10 (squashed for testing)
Comment 38 Nikolas Zimmermann 2020-11-26 06:27:20 PST
Created attachment 414900 [details]
Patch, v11 (squashed for testing)
Comment 39 Nikolas Zimmermann 2020-11-26 06:47:20 PST
Created attachment 414901 [details]
Patch, v12 (squashed for testing)
Comment 40 Nikolas Zimmermann 2020-11-26 09:16:39 PST
Created attachment 414907 [details]
Patch, v13 (squashed for testing)

Hopefully "Patch, v13" compiles everywhere. Willn now aggregate and fix remaining layout test regressions/crashes on all platforms. Stay tuned.
Comment 41 Nikolas Zimmermann 2020-11-27 00:12:46 PST
Created attachment 414917 [details]
Patch, v14 (squashed for testing)
Comment 42 Nikolas Zimmermann 2020-11-27 01:06:17 PST
Created attachment 414922 [details]
Patch, v15 (squashed for testing)

"Patch, v15" should compile everywhere and show no crashes. Three layout test regressions are expected to remain on all platforms, that are tackled once the crashes are gone.
Comment 43 Nikolas Zimmermann 2020-11-28 00:28:43 PST
Created attachment 414974 [details]
Patch, v16 (squashed for testing)
Comment 44 Nikolas Zimmermann 2020-12-11 02:07:23 PST
It took a while to setup a local macOS/iOS debugging environment, and debug the compositing test failures -- I now have a clear understanding what went wrong and only one remaining regressions to tackle -- a final patch can be expected today or tomorrow.
Comment 45 Nikolas Zimmermann 2020-12-11 15:32:53 PST
Created attachment 416053 [details]
Patch, v17 (squashed for testing)
Comment 46 Nikolas Zimmermann 2020-12-11 15:44:53 PST
Created attachment 416058 [details]
Patch, v18 (squashed for testing)
Comment 47 Nikolas Zimmermann 2020-12-11 16:30:45 PST
I've fixes the remaining layout tests failures on macOS/iOS using a catalina system. Assuming patch v18 passes EWS everywhere, I consider the initial work done.

As the patch is really large, I propose to split up the patch into 4 atomic chunks:

1) Add RenderLayerScrollableArea stub + build system changes (already filed https://bugs.webkit.org/show_bug.cgi?id=219808 for this)

2) Move overflow/scroll/marquee/... code from RenderLayer to RenderLayerScrollableArea in such a way that the impact outside of RenderLayer is minimal (to allow for a smaller patch, at the cost of a few proxy methods in RenderLayer that forward to RenderLayerScrollableArea)

3) Remove the proxy methods from RenderLayer, and make a few call-sites (EventHandler, ...) aware of it

4) Only create RenderLayerScrollableArea on demand.

The patches for 1) + 2) + 3) shall have no observable difference.
Only patch 4) actually enables the benefit of the improved design.

See RenderBox::requiresLayerWithScrollableArea() for the small list of conditions: RenderView + document element renderer receive a RenderLayerScrollableArea (RLSA). Boxes with horizontal/vertical overflow present, CSS 'resize' property set, or <marquee> elements also get a RLSA -- all other cases save memory.

Let me know what you think of this.
Comment 48 Nikolas Zimmermann 2021-01-05 02:02:19 PST
Created attachment 416978 [details]
Patch, v19
Comment 49 Nikolas Zimmermann 2021-01-05 02:06:11 PST
I've uploaded the full patch for review - unfortunately the part that moves methods from RenderLayer -> RenderLayerScrollableArea is ~ 130k alone + ChangeLog.

Before taking additional time to split up things, I'd like to hear from the reviewers first what they propose -- keep this large patch, or add temporary glue methods to minimize the changes outside of RenderLayer (but this glue code would be removed again in follow-up patches...)
Comment 50 Nikolas Zimmermann 2021-01-05 02:34:34 PST
Created attachment 416980 [details]
Patch, v20
Comment 51 Darin Adler 2021-01-05 09:38:08 PST
Comment on attachment 416980 [details]
Patch, v20

I like the "temporary glue" idea if it can effectively keep the large code movement separate from the other changes. Great for bisecting later.
Comment 52 Nikolas Zimmermann 2021-01-12 00:11:01 PST
(In reply to Darin Adler from comment #51)
> Comment on attachment 416980 [details]
> Patch
> 
> I like the "temporary glue" idea if it can effectively keep the large code
> movement separate from the other changes. Great for bisecting later.

Good, I will adapt the patch!
Thanks Darin, and sorry for the late reply - I missed the bugzilla mail.
Comment 53 Nikolas Zimmermann 2021-01-13 15:25:13 PST
Created attachment 417571 [details]
Patch, v21
Comment 54 Nikolas Zimmermann 2021-01-13 15:44:28 PST
Created attachment 417573 [details]
Patch, v22
Comment 55 Nikolas Zimmermann 2021-01-13 15:45:31 PST
"Patch, v22" aims to provide the same functionality than the previous patches, but the code changes are more isolated in RenderLayer/RenderLayerScrollableArea.

Important public RenderLayerScrollableArea methods are proxied through RenderLayer (temporary glue code) to minimize code changes outside of RenderLayer. The only code changes that have to be made is in places that assume that a RenderLayer* can be casted to a ScrollableArea*, which is no longer the case.

This is the smallest version I could produce, obviously still a large patch...

Just finished rebasing this on WebKit ToT, and haven't performed a full layout test run yet after rebasing. Will get some sleep, but didn't want to leave without giving EWS a chance to work overnight :-)
Comment 56 Nikolas Zimmermann 2021-01-14 16:12:31 PST
Created attachment 417666 [details]
Patch, v23
Comment 57 Simon Fraser (smfr) 2021-01-16 10:45:54 PST
Comment on attachment 417666 [details]
Patch, v23

View in context: https://bugs.webkit.org/attachment.cgi?id=417666&action=review

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:90
> +    auto& renderer = m_layer.renderer();
> +    ASSERT(m_registeredScrollableArea == renderer.view().frameView().containsScrollableArea(this));
> +
> +    if (m_registeredScrollableArea)
> +        renderer.view().frameView().removeScrollableArea(this);
> +
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    unregisterAsTouchEventListenerForScrolling();
> +#endif
> +    if (Element* element = renderer.element())
> +        element->setSavedLayerScrollPosition(m_scrollPosition);
> +
> +    destroyScrollbar(HorizontalScrollbar);
> +    destroyScrollbar(VerticalScrollbar);
> +
> +    if (auto* scrollingCoordinator = renderer.page().scrollingCoordinator())
> +        scrollingCoordinator->willDestroyScrollableArea(*this);
> +
> +    clearScrollCorner();
> +    clearResizer();

Doing all this in the destructor makes me a bit nervous. Can we do it in a "clear" function that's called from RenderLayer?

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:106
> +void RenderLayerScrollableArea::storeScrollPosition()
> +{
> +    auto* element = m_layer.renderer().element();
> +    if (!element)
> +        return;
> +
> +    if (m_layer.renderBox()) {
> +        // We save and restore only the scrollOffset as the other scroll values are recalculated.
> +        m_scrollPosition = element->savedLayerScrollPosition();
> +        if (!m_scrollPosition.isZero())
> +            scrollAnimator().setCurrentPosition(m_scrollPosition);
> +    }
> +
> +    element->setSavedLayerScrollPosition(IntPoint());

"Store" is ambiguous here. It's more like "restore".

> Source/WebCore/rendering/RenderLayerScrollableArea.h:277
> +    // Minimum padding on x86_64 (4 bytes), after m_scrollHeight)
> +    bool m_scrollDimensionsDirty : 1;
> +    bool m_inOverflowRelayout : 1;
> +    bool m_registeredScrollableArea : 1;
> +    bool m_hasCompositedScrollableOverflow : 1;
> +
> +#if PLATFORM(IOS_FAMILY)
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    bool m_registeredAsTouchEventListenerForScrolling : 1;
> +#endif
> +    bool m_adjustForIOSCaretWhenScrolling : 1;
> +#endif
> +    bool m_requiresScrollPositionReconciliation : 1;
> +    bool m_containsDirtyOverlayScrollbars : 1;
> +    bool m_updatingMarqueePosition : 1;

It's probably not worth using a bitset here for memory saving any more; if these are plain bools you can use initializers.
Comment 58 Nikolas Zimmermann 2021-01-16 16:27:30 PST
(In reply to Simon Fraser (smfr) from comment #57)
> Comment on attachment 417666 [details]
> Patch, v23
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417666&action=review
> 
> > Source/WebCore/rendering/RenderLayerScrollableArea.cpp:90
> > +    auto& renderer = m_layer.renderer();
> > +    ASSERT(m_registeredScrollableArea == renderer.view().frameView().containsScrollableArea(this));
> > +
> > +    if (m_registeredScrollableArea)
> > +        renderer.view().frameView().removeScrollableArea(this);
> > +
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> > +    unregisterAsTouchEventListenerForScrolling();
> > +#endif
> > +    if (Element* element = renderer.element())
> > +        element->setSavedLayerScrollPosition(m_scrollPosition);
> > +
> > +    destroyScrollbar(HorizontalScrollbar);
> > +    destroyScrollbar(VerticalScrollbar);
> > +
> > +    if (auto* scrollingCoordinator = renderer.page().scrollingCoordinator())
> > +        scrollingCoordinator->willDestroyScrollableArea(*this);
> > +
> > +    clearScrollCorner();
> > +    clearResizer();
> 
> Doing all this in the destructor makes me a bit nervous. Can we do it in a
> "clear" function that's called from RenderLayer?
Good idea. RenderLayer::clearLayerScrollableArea() already calls clearScrollCorner / clearResizer on RenderLayerScrollableArea. I'll change that to a single clear() call that encapsulates what the destructor is currenly doing.

> > Source/WebCore/rendering/RenderLayerScrollableArea.cpp:106
> > +void RenderLayerScrollableArea::storeScrollPosition()
> > +{
> > +    auto* element = m_layer.renderer().element();
> > +    if (!element)
> > +        return;
> > +
> > +    if (m_layer.renderBox()) {
> > +        // We save and restore only the scrollOffset as the other scroll values are recalculated.
> > +        m_scrollPosition = element->savedLayerScrollPosition();
> > +        if (!m_scrollPosition.isZero())
> > +            scrollAnimator().setCurrentPosition(m_scrollPosition);
> > +    }
> > +
> > +    element->setSavedLayerScrollPosition(IntPoint());
> 
> "Store" is ambiguous here. It's more like "restore".
Indeed, bad naming, fixed.
> 
> > Source/WebCore/rendering/RenderLayerScrollableArea.h:277
> > +    // Minimum padding on x86_64 (4 bytes), after m_scrollHeight)
> > +    bool m_scrollDimensionsDirty : 1;
> > +    bool m_inOverflowRelayout : 1;
> > +    bool m_registeredScrollableArea : 1;
> > +    bool m_hasCompositedScrollableOverflow : 1;
> > +
> > +#if PLATFORM(IOS_FAMILY)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> > +    bool m_registeredAsTouchEventListenerForScrolling : 1;
> > +#endif
> > +    bool m_adjustForIOSCaretWhenScrolling : 1;
> > +#endif
> > +    bool m_requiresScrollPositionReconciliation : 1;
> > +    bool m_containsDirtyOverlayScrollbars : 1;
> > +    bool m_updatingMarqueePosition : 1;
> 
> It's probably not worth using a bitset here for memory saving any more; if
> these are plain bools you can use initializers.
Good point - by this I spotted that m_scrollWidth / m_scrollHeight were not initialized in the constructor -- moved everything to in-place initialization to avoid that.
Comment 59 Nikolas Zimmermann 2021-01-16 16:28:29 PST
Comment on attachment 417666 [details]
Patch, v23

Landed in r271559 (https://trac.webkit.org/changeset/271559/webkit).
Leaving the bug open, as the glue code removal is now pending.
Comment 60 Truitt Savell 2021-01-19 13:19:21 PST
The changes in https://trac.webkit.org/changeset/271559/webkit

broke fast/forms/password-scrolled-after-caps-lock-toggled.html on Big Sur

History;
https://results.webkit.org/?suite=layout-tests&test=fast%2Fforms%2Fpassword-scrolled-after-caps-lock-toggled.html

Diff:
--- /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/fast/forms/password-scrolled-after-caps-lock-toggled-expected.txt
+++ /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/fast/forms/password-scrolled-after-caps-lock-toggled-actual.txt
@@ -25,7 +25,7 @@
 PASS document.getElementById('input').scrollLeft is 0
 
 After caps lock enabled:
-PASS document.getElementById('input').scrollLeft is 0
+FAIL document.getElementById('input').scrollLeft should be 0. Was 5.
 
 After caps lock disabled:
 PASS document.getElementById('input').scrollLeft is 0
@@ -39,6 +39,7 @@
 After caps lock disabled:
 PASS document.getElementById('input').scrollLeft is 0
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
 

I can reproduce this with command:
run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f fast/forms/password-scrolled-after-caps-lock-toggled.html
Comment 61 Nikolas Zimmermann 2021-01-25 14:34:32 PST
(In reply to Truitt Savell from comment #60)
> The changes in https://trac.webkit.org/changeset/271559/webkit
> 
> broke fast/forms/password-scrolled-after-caps-lock-toggled.html on Big Sur
> 
> History;
> https://results.webkit.org/?suite=layout-tests&test=fast%2Fforms%2Fpassword-
> scrolled-after-caps-lock-toggled.html
> 
Oh I missed this comment, sorry. webkit.org/b/220743 was opened to track the issue. The regression was fixed in r271732.

Please let me know if you see any other issues/flakiness since this landed.
Comment 62 Nikolas Zimmermann 2021-01-25 14:38:25 PST
Since this change was rather intrusive and large, I'd leave a comment here explaining the dependencies of the whole patchset, if we ever need to bisect this :-)

The chronological order of the RenderLayerScrollableArea introduction:

#1) webkit.org/b/219808 - Introduce RenderLayerScrollableArea
Landed: 30.12.2020
Committed r271111: <https://trac.webkit.org/changeset/271111>

#2) webkit.org/b/60305 - Separate scrolling code out of RenderLayer
Landed: 16.01.2021
Committed r271559: <https://trac.webkit.org/changeset/271559>

#3) webkit.org/b/220705 - Remove recently added glue code: RenderLayer::(scrollToOffset / scrollToXOffset / scrollToYOffset)
Landed: 18.01.2021
Committed r271577: <https://trac.webkit.org/changeset/271577>

#4) webkit.org/b/220708 - Remove recently added glue code: RenderLayer::(setAdjustForIOSCaretWhenScrolling / setScrollShouldClearLatchedState / setConstrainsScrollingToContentEdge)
Landed: 18.01.2021
Committed r271585: <https://trac.webkit.org/changeset/271585>

#5) webkit.org/b/220715 - Continue removing glue code from RenderLayer that was recently added in r271559
Landed: 19.01.2021
Committed r271598: <https://trac.webkit.org/changeset/r271598>

#6) webkit.org/b/220743 - REGRESSION (r271559): [BigSur] fast/forms/password-scrolled-after-caps-lock-toggled.html is consistently failing
Landed: 22.01.2021
Committed r271732: <https://trac.webkit.org/changeset/271732>

#7) webkit.org/b/220729 - Continue removing glue code from RenderLayer that was recently added in r271559
Landed: 22.01.2021
Committed r271740: <https://trac.webkit.org/changeset/r271740>

#8) webkit.org/b/220851 - Finish introduction of RenderLayerScrollableArea: remove remaining glue code from RenderLayer
Landed: 25.01.2021
Committed r271814: <https://trac.webkit.org/changeset/r271814>

Today the final patch landed - bug 220851 - removing the last bits of the temporary glue code and choosing a sane, consistent variable naming scheme for the scrollableArea() / ensureLayerScrollableArea() results (prefer 'scrollableArea' over 'scrollableLayer').

Therefore this bug can be closed now, finally. Now moving on to SVG + RenderLayer again.
Comment 63 Radar WebKit Bug Importer 2021-01-25 14:39:17 PST
<rdar://problem/73589122>