WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
57124
When the scroller style is changed via delegate method, the page needs a full relayout and repaint
https://bugs.webkit.org/show_bug.cgi?id=57124
Summary
When the scroller style is changed via delegate method, the page needs a full...
Beth Dakin
Reported
2011-03-25 14:43:57 PDT
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
>
Attachments
Patch
(3.35 KB, patch)
2011-03-25 14:48 PDT
,
Beth Dakin
mjs
: review-
Details
Formatted Diff
Diff
New patch
(13.11 KB, patch)
2011-03-28 15:36 PDT
,
Beth Dakin
darin
: review-
Details
Formatted Diff
Diff
Patch
(13.08 KB, patch)
2011-03-28 15:59 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Patch that builds
(13.07 KB, patch)
2011-03-28 16:04 PDT
,
Beth Dakin
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2011-03-25 14:48:06 PDT
Created
attachment 86984
[details]
Patch
Maciej Stachowiak
Comment 2
2011-03-25 14:56:44 PDT
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
Comment 3
2011-03-25 16:45:45 PDT
(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
Comment 4
2011-03-25 18:17:28 PDT
I would think that setNeedsRecalcStyleInAllFrames would work.
Beth Dakin
Comment 5
2011-03-28 15:36:28 PDT
Created
attachment 87222
[details]
New patch
Darin Adler
Comment 6
2011-03-28 15:51:13 PDT
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
Comment 7
2011-03-28 15:57:07 PDT
(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
Comment 8
2011-03-28 15:59:37 PDT
Created
attachment 87228
[details]
Patch
Beth Dakin
Comment 9
2011-03-28 16:04:58 PDT
Created
attachment 87229
[details]
Patch that builds
Beth Dakin
Comment 10
2011-03-28 16:15:36 PDT
Thanks! Fixed with r 82171.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug