| Summary: | REGRESSION (r260571): Scrolling on weather.com in Safari causes the gradient background to flicker (fixed backgrounds) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
| Component: | Scrolling | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | cmarcelo, darin, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, thorton, tonikitoo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Simon Fraser (smfr)
2020-09-04 14:36:52 PDT
Created attachment 408025 [details]
Patch
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. (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. |