Bug 78862 – Register scrolling layers with ScrollingCoordinator

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sami Kyostila]

Touch overflow scrolling elements (-webkit-overflow-scrolling: touch) should be split into layers and registered with the ScrollCoordinator so they can be scrolled efficiently without repainting the contents.

[Antonio Gomes]

Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch?

[Antonio Gomes]

(In reply to comment #1)
> Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch?

I am asking because I have done that for the BlackBerry port myself, but it was not a clean backport...

[Sami Kyostila]

It's really more of a reimplementation than a straight port. I agree that the iOS5 branch isn't really usable as such.

[Sami Kyostila]

Created attachment 146645 [details]
Patch

First version with the main pieces of functionality in place. Does not calculate per-layer non-fast scrollable regions, but that could be left for a follow-up patch.

[Antonio Gomes]

Comment on attachment 146645 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Touch overflow scrolling elements should be split into layers
> +        https://bugs.webkit.org/show_bug.cgi?id=78862
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        When a regular overflow element is scrolled, we repaint the entire area
> +        it occupies in the view from scratch. Since this painting is done in the
> +        main thread, it can be blocked by, e.g., JavaScript execution, further
> +        limiting performance.
> +

I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point..

> Source/WebCore/rendering/RenderLayer.cpp:4862
> +    if (hasOverflow && isVisibleToHitTest
> +#if ENABLE(OVERFLOW_SCROLLING)
> +        && !hasAcceleratedScrolling()
> +#endif
> +        )

the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not.

Could you explain why this change specifically?

[Sami Kyostila]

Comment on attachment 146645 [details]
Patch

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

Something I failed to mention earlier is that I'm still working on layout tests for these changes.

>> Source/WebCore/ChangeLog:12
>> +
> 
> I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point..

You are absolutely right. I did not mean to pass this off as original work.

>> Source/WebCore/rendering/RenderLayer.cpp:4862
>> +        )
> 
> the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not.
> 
> Could you explain why this change specifically?

My motivation was to avoid the scrollable area from becoming part of the non-fast scrollable region (see ScrollingCoordinator.cpp) which would prevent it from being independently scrolled by the threaded compositor.

Completely removing the scrollable layer from the scrollable set is a bit drastic, so perhaps it would be better to move this logic to ScrollingCoordinator instead.

[Sami Kyostila]

Created attachment 147387 [details]
Patch

Updated to match new interface from bug 73350. Did not fix FrameView scrollable are registry yet.

[Sami Kyostila]

Created attachment 147879 [details]
Patch

- Keep all ScrollableAreas -- fast scrollable or not -- in the FrameView's scrollable area set.
- Add layout tests to verify graphics layer configuration and stacking context behavior with overflow:hidden.

[Sami Kyostila]

The test coverage for the prepainting part of this patch isn't as good as I'd like yet. I'm thinking of somehow capturing the GraphicsLayer invalidations from DRT.

[Sami Kyostila]

Created attachment 148135 [details]
Patch

- Added new test to verify scrolling tree using the framework from bug 89216.

[Adam Barth]

Who should review this patch?  @jamesr?

[Sami Kyöstilä]

Created attachment 148562 [details]
Patch

- Cleaned up most of the #ifdef spaghetti.
- Added test to verify scroll content layer alignment with borders.
- Added test to check outline fallback.

[Sami Kyöstilä]

Created attachment 148604 [details]
Patch

- Added overflow clipping test. Does not cover all the cases yet but it's a start.
- Removed scrollingTreeAsText() test for now.

[Simon Fraser (smfr)]

Comment on attachment 148604 [details]
Patch

I have some review feedback, but need a few days to look at the patch. Please wait for my feedback.

[Simon Fraser (smfr)]

Sami: please cc: me on any future bugs that involve changes to RenderLayerCompositor/RenderLayerBacking/GraphicsLayer. Thanks.

[Simon Fraser (smfr)]

Comment on attachment 148604 [details]
Patch

I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.

[Sami Kyöstilä]

(In reply to comment #16)
> I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.

Okay, thanks Simon!

[Adam Barth]

> I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.

@smfr: Did this end up happening? This patch accounts for a moderate amount of the remaining chromium-android fork and I'd like to make sure we don't stall out here.  Thanks!

[Simon Fraser (smfr)]

Sorry I haven't had time yet.

[Adam Barth]

> Sorry I haven't had time yet.

Would it be appropriate to proceed with this patch and then plan to merge with your work when you have time to contribute it?  I don't fully understand the relation between the patches, so it's hard for me to know what the best course of action is here.

[Simon Fraser (smfr)]

Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week.

[Sami Kyöstilä]

(In reply to comment #21)
> Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week.

Thanks for the update. Is there another bug open for your patch, or were you going to land it against this one?

[Simon Fraser (smfr)]

I'll do it via bug 91117.

[Adam Barth]

Thanks Simon.

[Adam Barth]

> I'll try to get to it early next week.

Simon, I'm sorry to keep nagging you about this issue, but you've been blocking this patch for upwards of a month now.  This bug is blocking us from upstreaming the chromium-android port and is one of the larger diffs left in WebCore, as described in <http://lists.webkit.org/pipermail/webkit-dev/2012-July/021516.html>.

How do we make progress here?

[Simon Fraser (smfr)]

This is top of my list to do after some internal stuff.

[Adam Barth]

> This is top of my list to do after some internal stuff.

Thanks Simon.

[Antonio Gomes]

(In reply to comment #27)
> > This is top of my list to do after some internal stuff.
> 
> Thanks Simon.

Just to make it clearer to me, once bug 91117 is done, there should still work to be done here?

[Simon Fraser (smfr)]

ENABLE(OVERFLOW_SCROLLING) is a really bad name for this flag. overflows already scroll. It should have been ENABLE(COMPOSITED_OVERFLOW_SCROLLING) or something.

[Simon Fraser (smfr)]

Also, I think it's fine to only protected the CSS parser code with #if ENABLE(OVERFLOW_SCROLLING). That just needs to hide the new CSS property. The rest of the code doesn't need #ifdefs.

[Antonio Gomes]

(In reply to comment #28)
> (In reply to comment #27)
> > > This is top of my list to do after some internal stuff.
> > 
> > Thanks Simon.
> 
> Just to make it clearer to me, once bug 91117 is done, there should still work to be done here?

Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...

[Antonio Gomes]

The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here:

        void setAlwaysUseAcceleratedOverflowScroll(bool flag)
        bool alwaysUseAcceleratedOverflowScroll() const

[Sami Kyöstilä]

(In reply to comment #31)
> Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...

Right, as far as I can tell the following things are still needed on top of bug 91117:

- Letting ScrollingCoordinator know about scrollable layers.
- Avoiding repainting layer contents after scrolling.
- Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).

[Simon Fraser (smfr)]

(In reply to comment #32)
> The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here:
> 
>         void setAlwaysUseAcceleratedOverflowScroll(bool flag)
>         bool alwaysUseAcceleratedOverflowScroll() const

I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages.

[Simon Fraser (smfr)]

(In reply to comment #33)
> (In reply to comment #31)
> > Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...
> 
> Right, as far as I can tell the following things are still needed on top of bug 91117:
> 
> - Letting ScrollingCoordinator know about scrollable layers.
> - Avoiding repainting layer contents after scrolling.

Yes on these two.

> - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).

I don't think you need any changes here; the repaintContainner logic should take case of this.

[Antonio Gomes]

(In reply to comment #34)
> (In reply to comment #32)
> > The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here:
> > 
> >         void setAlwaysUseAcceleratedOverflowScroll(bool flag)
> >         bool alwaysUseAcceleratedOverflowScroll() const
> 
> I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages.

would you have example of broken web pages with that overly enabled?

[Sami Kyöstilä]

Created attachment 154694 [details]
Patch

Work-in-progress patch against 91117
    - Register scrolling layers with ScrollingCoordinator.
    - Implement bounds origin in Chromium compositor.
    - Keep scrolling contents layer under overflow controls.
    - Don't reparent overflow controls inside scrolling content.
    - Promote composited scrolling layers to composited layers.
    - Include composited scrolling layers in memory usage estimate.
    - Avoid repaints when scrolling (not working yet).

[Adrienne Walker]

Comment on attachment 154694 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:120
> +    const FloatPoint& boundsOrigin() const { return m_boundsOrigin; }
> +    void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; }

What does bounds origin mean? 

Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?

[Sami Kyöstilä]

(In reply to comment #38)
> What does bounds origin mean? 
> 
> Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?

As far as I can tell it could be baked into sublayerTransform, so I don't think the compositor actually needs to care about it. I'm with you on not adding any more degrees of freedom to the layer transforms.

[Sami Kyöstilä]

(In reply to comment #35)
> > - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).
> 
> I don't think you need any changes here; the repaintContainner logic should take case of this.

I tried to make this work but looks like the repaints are still getting clipped to the visible part of the overflow contents. For example:

1. Make an overflow div with lots of text.
2. Select a part of the text at the beginning and scroll to the end.
3. Deselect and scroll back to the beginning.

=> The text is still selected, because the deselect repaint got clipped.

I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something?

[Adrienne Walker]

(In reply to comment #40)

> I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something?

For what it's worth, Chromium explicitly doesn't clip repaint rects for frame scrolling so that we can know which parts of the layer are invalidated even if they're not visible and don't have to invalidate on scroll.  See FrameView::clipsRepaints().

[Simon Fraser (smfr)]

(In reply to comment #38)
> (From update of attachment 154694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154694&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120
> > +    const FloatPoint& boundsOrigin() const { return m_boundsOrigin; }
> > +    void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; }
> 
> What does bounds origin mean? 
> 
> Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?

We used boundsOrigin because it plays nicely with UIScrollViews on iOS.

[James Robinson]

(In reply to comment #42)
> (In reply to comment #38)
> > (From update of attachment 154694 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154694&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120
> > > +    const FloatPoint& boundsOrigin() const { return m_boundsOrigin; }
> > > +    void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; }
> > 
> > What does bounds origin mean? 
> > 
> > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?
> 
> We used boundsOrigin because it plays nicely with UIScrollViews on iOS.

I don't understand what UIScrollViews has to do with adding a new property to LayerChromium.

Created attachment 157739 [details]
Diff with 91117

This patch set removes boundsOrigin from LayerChromium and below and instead
bakes the value into the sublayerTransform.

Created attachment 157740 [details]
Patch including 91117

This attachment also contains code from 91117 (so that it can apply for the ews
bots).

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 157740 [details]
Patch including 91117

Attachment 157740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13479010

[Simon Fraser (smfr)]

(In reply to comment #44)
> Created an attachment (id=157739) [details]
> Diff with 91117
> 
> This patch set removes boundsOrigin from LayerChromium and below and instead
> bakes the value into the sublayerTransform.

How does that work if the overflow element also has perspective style?

Created attachment 157805 [details]
Patch

Adding missing file: WebScrollDelegate.h.

[WebKit Review Bot]

Comment on attachment 157805 [details]
Patch

Attachment 157805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13464933

New failing tests:
fast/css/resize-corner-tracking-transformed.html
compositing/geometry/clip.html
compositing/overflow/overflow-scroll.html

[WebKit Review Bot]

Created attachment 157848 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

The layout test failures appear to be due to 91117, so this should still be safe to review.

[Sami Kyöstilä]

Comment on attachment 157805 [details]
Patch

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

> Source/WebKit/chromium/features.gypi:182
> +          'ENABLE_OVERFLOW_SCROLLING=1',

I don't think we want to enable this unconditionally just yet -- see, e.g., http://code.google.com/p/chromium/issues/detail?id=128325 for the kinds of problems it can cause.

[Antonio Gomes]

Comment on attachment 157805 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:673
> +        // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer.
> +        m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);

why the fixme instead of using it right way?

[Sami Kyöstilä]

(In reply to comment #54)
> > +        // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer.
> > +        m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);
> 
> why the fixme instead of using it right way?

The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get bug 91117 landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work.

[James Robinson]

(In reply to comment #55)
> (In reply to comment #54)
> > > +        // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer.
> > > +        m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);
> > 
> > why the fixme instead of using it right way?
> 
> The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get bug 91117 landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work.

91117 has landed now - what's next for this patch?

[James Robinson]

Comment on attachment 157805 [details]
Patch

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

This needs a rebase now that 91117 has landed

>> Source/WebCore/rendering/RenderLayerBacking.cpp:673
>> +        m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);
> 
> why the fixme instead of using it right way?

Now that bug 91117 has landed I'd like to see an attempt at this.

[Sami Kyöstilä]

(In reply to comment #57)
> > why the fixme instead of using it right way?
> 
> Now that bug 91117 has landed I'd like to see an attempt at this.

Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll.

I've updated the title to better match the scope -- please let me know if want this bug split further.

[James Robinson]

(In reply to comment #58)
> (In reply to comment #57)
> > > why the fixme instead of using it right way?
> > 
> > Now that bug 91117 has landed I'd like to see an attempt at this.
> 
> Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll.
> 
> I've updated the title to better match the scope -- please let me know if want this bug split further.

It's hard to say when to split.  I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy.

[Sami Kyöstilä]

(In reply to comment #59)
> It's hard to say when to split.  I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy.

Ok. I'll use this bug for the registration part since it's (mostly) Chromium and will open another bug for prepainting.

[Sami Kyöstilä]

Created attachment 161523 [details]
Patch

[James Robinson]

Comment on attachment 161523 [details]
Patch

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

> Source/Platform/ChangeLog:3
> +        Register scrolling layers with ScrollingCoordinator

this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc.  Please split the chromium parts out into a different patch so it can be reviewed separately.

> Source/Platform/chromium/public/WebLayer.h:177
> +    virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0;

document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?)

> Source/Platform/chromium/public/WebLayerScrollDelegate.h:32
> +class WebLayerScrollDelegate {

s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API

> Source/Platform/chromium/public/WebLayerScrollDelegate.h:35
> +};

declared d'tor please (protected most likely since you shouldn't call delete on a client ptr)

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33
> +#include "LayerChromium.h"

this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation

> Source/WebCore/platform/ScrollableArea.h:173
> +    virtual bool usesCompositedScrolling() const { return false; }

why is this outside of USE(ACCELERATED_COMPOSITING) ?

[Sami Kyöstilä]

Comment on attachment 161523 [details]
Patch

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

>> Source/Platform/ChangeLog:3
>> +        Register scrolling layers with ScrollingCoordinator
> 
> this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc.  Please split the chromium parts out into a different patch so it can be reviewed separately.

Sure, here's the Chromium part: https://bugs.webkit.org/show_bug.cgi?id=95679

>> Source/Platform/chromium/public/WebLayer.h:177
>> +    virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0;
> 
> document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?)

Good idea. I've added a comment block.

>> Source/Platform/chromium/public/WebLayerScrollDelegate.h:32
>> +class WebLayerScrollDelegate {
> 
> s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API

Done. FWIW, this was modeled after WebAnimationDelegate.

>> Source/Platform/chromium/public/WebLayerScrollDelegate.h:35
>> +};
> 
> declared d'tor please (protected most likely since you shouldn't call delete on a client ptr)

Done.

>> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33
>> +#include "LayerChromium.h"
> 
> this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation

Sorry, I added this include by mistake; there isn't an actual dependency here.

>> Source/WebCore/platform/ScrollableArea.h:173
>> +    virtual bool usesCompositedScrolling() const { return false; }
> 
> why is this outside of USE(ACCELERATED_COMPOSITING) ?

This used to be so because RenderLayer exposes usesCompositedScrolling() both with and without USE(ACCELERATED_COMPOSITING). I've now rearranged things so that RenderLayer uses OVERRIDE for usesCompositedScrolling() based on the build configuration.

[Sami Kyöstilä]

Created attachment 161924 [details]
Patch

Cross-platform patch.

[James Robinson]

Comment on attachment 161924 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 161924 [details]
Patch

Clearing flags on attachment: 161924

Committed r127480: <http://trac.webkit.org/changeset/127480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(In reply to comment #57)
> (From update of attachment 157805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157805&action=review
> 
> This needs a rebase now that 91117 has landed
> 
> >> Source/WebCore/rendering/RenderLayerBacking.cpp:673
> >> +        m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay);
> > 
> > why the fixme instead of using it right way?
> 
> Now that bug 91117 has landed I'd like to see an attempt at this.

If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik.

[Sami Kyöstilä]

(In reply to comment #68)
> If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik.

That's right, I've opened https://bugs.webkit.org/show_bug.cgi?id=96087 to track that.