Bug 57124 – When the scroller style is changed via delegate method, the page needs a full relayout and repaint

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

In the delegate method updateScrollerStyleForNewRecommendedScrollerStyle (which is is ScrollAnimatorMac), to avoid paint artifacts when the styles switch, we need a full relayout and repaint. Since this is an uncommon operation, I think the easiest way to do this with certainty is to just reload the page in this case. Patch forthcoming.

<rdar://problem/9059129>

[Beth Dakin]

Created attachment 86984 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 86984 [details]
Patch

I don't think a reload is right. Changing system preferences shouldn't reload all your pages - that's likely to change the content. Why can't you just schedule a full relayout and repaint? It seems like calling setNeedsLayout() on the FrameView should do the trick.

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 86984 [details])
> I don't think a reload is right. Changing system preferences shouldn't reload all your pages - that's likely to change the content. Why can't you just schedule a full relayout and repaint? It seems like calling setNeedsLayout() on the FrameView should do the trick.

A simple setNeedsLayout() does not seem to be sufficient actually, but I'm sure I can make something less intrusive work.

[Dave Hyatt]

I would think that

setNeedsRecalcStyleInAllFrames

would work.

[Beth Dakin]

Created attachment 87222 [details]
New patch

[Darin Adler]

Comment on attachment 87222 [details]
New patch

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

review-, but mainly because of the function name.

> Source/WebCore/page/Page.h:122
> +        void setNeedsRecalcStyleInAllFrames();

This is at the top of the Page class. Is it really the most important function in the entire class!? I suppose that theme() wasn’t either, but still, I suggest putting this function much further down, ideally grouped with other similar functions.

> Source/WebCore/platform/ScrollableArea.h:119
> +    virtual void setNeedsRecalcStyleInAllFrames() { }

This function should be named scrollerStyleChanged or something like that. It’s none of ScrollableArea’s business what exactly will be done, and “all frames” is certainly nothing the scrollable area should know about.

The implementation in FrameView will still call setNeedsRecalcStyle, of course.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:272
> +    _animator->scrollableArea()->setNeedsRecalcStyleInAllFrames();

How many times will this be called? Will we end up iterating over all the frames once for every scroller? That could add up and be too slow.

What if all the scrollers are in overflow areas? This code only works based on the FrameView, but really it should get to the page and do the work even if it’s not a frame.

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 87222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87222&action=review
> 
> review-, but mainly because of the function name.
> 
> > Source/WebCore/page/Page.h:122
> > +        void setNeedsRecalcStyleInAllFrames();
> 
> This is at the top of the Page class. Is it really the most important function in the entire class!? I suppose that theme() wasn’t either, but still, I suggest putting this function much further down, ideally grouped with other similar functions.
> 

I put it at the top mainly because it is very similar to the static function scheduleForcedStyleRecalcForAllPages(); which is listed at the very, very top of the page class…even before the constructor and destructor. And placing my new function near the top in the .h also means the implementations of these two similar functions are right next to each other in the .cpp. Perhaps the existing static function is only so high because it's static. But nevertheless, this new function is more similar to that static function than it is to any other function in the class.


> > Source/WebCore/platform/ScrollableArea.h:119
> > +    virtual void setNeedsRecalcStyleInAllFrames() { }
> 
> This function should be named scrollerStyleChanged or something like that. It’s none of ScrollableArea’s business what exactly will be done, and “all frames” is certainly nothing the scrollable area should know about.
> 
> The implementation in FrameView will still call setNeedsRecalcStyle, of course.

Okay.

> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:272
> > +    _animator->scrollableArea()->setNeedsRecalcStyleInAllFrames();
> 
> How many times will this be called? Will we end up iterating over all the frames once for every scroller? That could add up and be too slow.
> 
> What if all the scrollers are in overflow areas? This code only works based on the FrameView, but really it should get to the page and do the work even if it’s not a frame.

This might be a slow operation on some pages, but it is only called when a System Preference is changed. Therefore, it is a very uncommon operation. Also, it works for all ScrollableAreas because telling the Page that all frames need a style reclac means that all frames and everything insides those frames (including overflow areas) will be recalculated.

[Beth Dakin]

Created attachment 87228 [details]
Patch

[Beth Dakin]

Created attachment 87229 [details]
Patch that builds

[Beth Dakin]

Thanks! Fixed with r 82171.