REGRESSION (r260571): Scrolling on weather.com in Safari causes the gradient background to flicker (fixed backgrounds)

[Simon Fraser (smfr)]

REGRESSION (r260571): Scrolling on weather.com in Safari causes the gradient background to flicker (fixed backgrounds)

[Simon Fraser (smfr)]

Created attachment 408025 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/68192010>

[Darin Adler]

Comment on attachment 408025 [details]
Patch

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

> Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:124
> +    if (auto* rootNode = this->rootNode())
> +        return !rootNode->hasSynchronousScrollingReasons();
> +
> +    return true;

Here’s how I would write this:

    auto rootNode = this->rootNode();
    return !rootNode || !rootNode->hasSynchronousScrollingReasons();

I actually find this easier to reason about than the return-based one. Not sure others agree. Another option would be:

    auto rootNode = this->rootNode();
    return rootNode && rootNode->hasSynchronousScrollingReasons();

A plus for using auto rather than auto* is that we can switch to returning RefPtr from rootNode if we want to for object lifetime safety without modifying this call site.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #3)
> Comment on attachment 408025 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408025&action=review
> 
> > Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:124
> > +    if (auto* rootNode = this->rootNode())
> > +        return !rootNode->hasSynchronousScrollingReasons();
> > +
> > +    return true;
> 
> Here’s how I would write this:
> 
>     auto rootNode = this->rootNode();
>     return !rootNode || !rootNode->hasSynchronousScrollingReasons();
> 
> I actually find this easier to reason about than the return-based one. Not
> sure others agree. Another option would be:
> 
>     auto rootNode = this->rootNode();
>     return rootNode && rootNode->hasSynchronousScrollingReasons();

I think you mean !(rootNode && rootNode->hasSynchronousScrollingReasons());

I tried both previous versions as I was writing the patch.
 
> A plus for using auto rather than auto* is that we can switch to returning
> RefPtr from rootNode if we want to for object lifetime safety without
> modifying this call site.

I'm not a fan of 'auto' hiding the fact that it's a pointer.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/266664/webkit