[webkit-reviews] review denied: [Bug 57124] When the scroller style is changed via delegate method, the page needs a full relayout and repaint : [Attachment 87222] New patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 28 15:51:12 PDT 2011
Darin Adler <darin at apple.com> has denied Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 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
Attachment 87222: New patch
https://bugs.webkit.org/attachment.cgi?id=87222&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
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.
More information about the webkit-reviews
mailing list