33150 – Do not render the full frame when there is some elements with fixed positioning

CLOSED FIXED 33150

Do not render the full frame when there is some elements with fixed positioning

https://bugs.webkit.org/show_bug.cgi?id=33150

Summary Do not render the full frame when there is some elements with fixed positioning

Benjamin Poulain

Reported 2010-01-04 07:11:53 PST

Currently, when an element has a fixed position, ScrollViewcanBlitOnScroll() is false and the full view is updated on scrolling. This slow down the scrolling of any page with fixed elements (facebook, slashdot, etc). We could update the part that appear on the screen, and the pixels of the fixed elements that has been blitted.

Attachments
Repaint only the invalidated area after scrolling (5.55 KB, patch) 2010-01-04 07:16 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (6.44 KB, patch) 2010-01-05 08:24 PST, Benjamin Poulain	hyatt: review-	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (9.60 KB, patch) 2010-01-06 14:22 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (9.58 KB, patch) 2010-01-06 14:40 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (9.63 KB, patch) 2010-01-07 02:51 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (9.67 KB, patch) 2010-01-07 05:13 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (10.52 KB, patch) 2010-01-20 01:20 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (9.97 KB, patch) 2010-01-20 03:11 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (10.38 KB, patch) 2010-01-20 07:34 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (10.39 KB, patch) 2010-01-21 06:04 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (10.40 KB, patch) 2010-01-21 06:05 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
crash log from commit bot (50.85 KB, text/plain) 2010-01-21 15:38 PST, Eric Seidel (no email)	no flags	Details
Repaint only the invalidated area after scrolling (10.41 KB, patch) 2010-01-22 02:48 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.49 KB, patch) 2010-01-23 04:42 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.43 KB, patch) 2010-01-23 15:43 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.46 KB, patch) 2010-01-24 11:27 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.43 KB, patch) 2010-01-25 01:10 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (14.30 KB, patch) 2010-01-28 08:50 PST, Benjamin Poulain	hyatt: review-	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (15.18 KB, patch) 2010-02-01 07:55 PST, Benjamin Poulain	hyatt: review-	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (13.66 KB, patch) 2010-02-02 11:32 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.43 KB, patch) 2010-02-02 12:20 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.28 KB, patch) 2010-02-03 00:52 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.28 KB, patch) 2010-03-09 15:04 PST, Benjamin Poulain	manyoso: review-	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.34 KB, patch) 2010-03-10 02:45 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Repaint only the invalidated area after scrolling (11.33 KB, patch) 2010-03-10 03:01 PST, Benjamin Poulain	no flags	Details Formatted Diff Diff
Screenshot with Win Safari (229.39 KB, image/jpeg) 2010-03-29 02:41 PDT, Shinichiro Hamaji	no flags	Details
Patch (7.85 KB, patch) 2010-04-06 13:35 PDT, James Robinson	no flags	Details Formatted Diff Diff
Updated version of the patch (9.75 KB, patch) 2010-05-03 08:02 PDT, Benjamin Poulain	hyatt: review- hyatt: commit-queue-	Details Formatted Diff Diff
Patch (9.76 KB, patch) 2010-06-17 13:33 PDT, Benjamin Poulain	kenneth: review+ kenneth: commit-queue+	Details Formatted Diff Diff
Show Obsolete (25) View All Add attachment proposed patch, testcase, etc.

Benjamin Poulain

Comment 1 2010-01-04 07:16:04 PST

Created attachment 45799 [details] Repaint only the invalidated area after scrolling First patch to improve the performance with fixed elements.

Antti Koivisto

Comment 2 2010-01-04 08:50:59 PST

Seems like a good optimization. You should remove the fixed objects from the hash on destructor. You should figure out if the area to update ends up covering most of the view and just do full repaint in that case (avoiding the unnecessary copy scroll blit and performance regression). It might be worthwhile the track the covered region as rectangles (like QRegion) and update when positioned objects move. This would avoid having to iterate over all fixed objects. Dave Hyatt should take a look too.

Benjamin Poulain

Comment 3 2010-01-05 08:24:21 PST

Created attachment 45892 [details] Repaint only the invalidated area after scrolling (In reply to comment #2) > You should remove the fixed objects from the hash on destructor. Absolutely. I have done that in this patch. > You should figure out if the area to update ends up covering most of the view > and just do full repaint in that case (avoiding the unnecessary copy scroll > blit and performance regression). I this version, I have used a simple heuristic: "more than 4 fixed elements -> slow path". I have used the same kind of rule for the backingstore of Qt, but I had benchmark to justify the number. I'll wait for the point of Dave Hyatt on this limit. > It might be worthwhile the track the covered region as rectangles (like > QRegion) and update when positioned objects move. This would avoid having to > iterate over all fixed objects. I have looked at that and this can get difficult to update (for example when a child with absolute positioning is manipulated by javascript, updating the rects can be cumbersome).

WebKit Review Bot

Comment 4 2010-01-05 08:26:19 PST

Attachment 45892 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/ScrollView.cpp:529: Missing space before { [whitespace/braces] [5] Total errors found: 1

Dave Hyatt

Comment 5 2010-01-05 15:17:10 PST

Comment on attachment 45892 [details] Repaint only the invalidated area after scrolling There are a number of issues with this patch: (1) ScrollView is not designed to know anything about the render tree or RenderObjects directly. I'd move the HashSet to FrameView and then make a virtual method that is called in ScrollView. (2) Please make a constant for the cutoff number in the code. Don't just use 5 inline like that. (3) You should disregard fixed positioned objects inside transforms, as they aren't actually fixed to the ScrollView. You can verify this by checking if the containingBlock is actually the RenderView for each RenderObject before allowing it to be registered. (4) Why not make fixed background images work too? They are just as common (if not moreso) than fixed positioning.

Benjamin Poulain

Comment 6 2010-01-06 14:22:43 PST

Created attachment 45994 [details] Repaint only the invalidated area after scrolling Fixed the patch thanks to the comments of Dave Hyatt.

WebKit Review Bot

Comment 7 2010-01-06 14:25:26 PST

style-queue ran check-webkit-style on attachment 45994 [details] without any errors.

Benjamin Poulain

Comment 8 2010-01-06 14:30:53 PST

Thanks for the reviews. (In reply to comment #5) > (From update of attachment 45892 [details]) > (1) ScrollView is not designed to know anything about the render tree or > RenderObjects directly. I'd move the HashSet to FrameView and then make a > virtual method that is called in ScrollView. I have moved the logic to a function called ScrollView::fastScrollContents() that is reimplemented in FrameView. > (2) Please make a constant for the cutoff number in the code. Don't just use 5 > inline like that. Done :) > (3) You should disregard fixed positioned objects inside transforms, as they > aren't actually fixed to the ScrollView. You can verify this by checking if > the containingBlock is actually the RenderView for each RenderObject before > allowing it to be registered. You are right. I have updated the patch to take transformations to register only non-transformed objects. About verifying if the containingBlock is the RenderView, I now do that in the scrolling code. I don't think I can do that at the time of registering the object because its parent might get a transformation later. > (4) Why not make fixed background images work too? They are just as common (if > not moreso) than fixed positioning. I try to keep the patches small and I have another idea for fixed elements. I can fix background images later.

WebKit Review Bot

Comment 9 2010-01-06 14:32:53 PST

Attachment 45994 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/164930

Benjamin Poulain

Comment 10 2010-01-06 14:40:10 PST

Created attachment 45996 [details] Repaint only the invalidated area after scrolling (In reply to comment #9) > Attachment 45994 [details] did not build on chromium: > Build output: http://webkit-commit-queue.appspot.com/results/164930 My bad.

WebKit Review Bot

Comment 11 2010-01-06 14:41:41 PST

style-queue ran check-webkit-style on attachment 45996 [details] without any errors.

Kenneth Rohde Christiansen

Comment 12 2010-01-06 20:56:20 PST

Comment on attachment 45996 [details] Repaint only the invalidated area after scrolling > +void FrameView::fastScrollContents(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) I dislike the method name. Shouldn't it always be fast? Does FrameView already have a scrollContents method? > +{ > + const int fixedObjectNumberThreshold = 5; > + > + if (m_fixedObjects.isEmpty()) > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + else { > + Vector<RenderObject*> objectsFixedInViewport; We say fixedObjects elsewhere, right? > + objectsFixedInViewport.reserveCapacity(m_fixedObjects.size()); > + > + bool updateInvalidSubRect = true; > + // Get a list of fixed objects that are not in transformations > + HashSet<RenderObject*>::const_iterator end = m_fixedObjects.end(); > + HashSet<RenderObject*>::const_iterator it = m_fixedObjects.begin(); > + for (; it != end; ++it) { > + RenderObject* obj = *it; > + if (obj->containingBlock() == obj->view()) { > + objectsFixedInViewport.append(obj); > + if (objectsFixedInViewport.size() > fixedObjectNumberThreshold) { > + updateInvalidSubRect = false; > + break; > + } > + } > + } Instead of the updateInvalidSubRect, couldn't you clear the objectsFixedInViewport instead? and ask if (!objectsFixedInViewport.isEmpty()) below? I find that less mysterious. > + > + // scroll the content > + if (updateInvalidSubRect) { > + // 1) scroll > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + > + // 2) update the area of fixed objets that has been invalidated > + for (int i = 0; i < objectsFixedInViewport.size(); ++i) { > + IntRect topLevelRect; > + IntRect updateRect = (objectsFixedInViewport[i])->paintingRootRect(topLevelRect); Are the () really needed? > + updateRect.move(-scrollX(), -scrollY()); > + IntRect scrolledRect = updateRect; > + scrolledRect.move(scrollDelta); > + updateRect.unite(scrolledRect); > + updateRect.intersect(rectToScroll); > + hostWindow()->repaint(updateRect, true, false, true); > + } > + } else { > + // there are too many fixed objects, repaint everything might be easier // the number of fixed objects exceed the threshold, so we repaint everything. > + IntRect updateRect = clipRect; > + updateRect.intersect(rectToScroll); > + hostWindow()->repaint(updateRect, true, false, true); > + } > + } > +} > + > if (canBlitOnScroll() && !rootPreventsBlitting()) { // The main frame can just blit the WebView window > - // FIXME: Find a way to blit subframes without blitting overlapping content > - hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect); > + // FIXME: Find a way to blit subframes without blitting overlapping content > + fastScrollContents(-scrollDelta, scrollViewRect, clipRect); Wrong indentation

Benjamin Poulain

Comment 13 2010-01-07 02:51:55 PST

Created attachment 46040 [details] Repaint only the invalidated area after scrolling (In reply to comment #12) > (From update of attachment 45996 [details]) > > > +void FrameView::fastScrollContents(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) > > I dislike the method name. Shouldn't it always be fast? No, there are some cases when you cannot avoid the slow path. For example, if the view is not opaque, you need full repaint. > Does FrameView already > have a scrollContents method? Yep. > > + Vector<RenderObject*> objectsFixedInViewport; > > We say fixedObjects elsewhere, right? Right :) Fixed > Instead of the updateInvalidSubRect, couldn't you clear the > objectsFixedInViewport instead? and ask > if (!objectsFixedInViewport.isEmpty()) below? I find that less mysterious. This would not be correct. You can have updateInvalidSubRect == true and objectsFixedInViewport.isEmpty() in the case where all fixed object are under a transformed layer in the hierarchy. > > + IntRect updateRect = (objectsFixedInViewport[i])->paintingRootRect(topLevelRect); > > Are the () really needed? Oups. Fixed :) > > + // there are too many fixed objects, repaint everything might be easier > > // the number of fixed objects exceed the threshold, so we repaint everything. Changed. > > if (canBlitOnScroll() && !rootPreventsBlitting()) { // The main frame can just blit the WebView window > > - // FIXME: Find a way to blit subframes without blitting overlapping content > > - hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect); > > + // FIXME: Find a way to blit subframes without blitting overlapping content > > + fastScrollContents(-scrollDelta, scrollViewRect, clipRect); > > Wrong indentation Is it? The coding convention says 4 spaces, not 3.

WebKit Review Bot

Comment 14 2010-01-07 02:52:45 PST

style-queue ran check-webkit-style on attachment 46040 [details] without any errors.

Kenneth Rohde Christiansen

Comment 15 2010-01-07 04:20:19 PST

> > I dislike the method name. Shouldn't it always be fast? > > No, there are some cases when you cannot avoid the slow path. For example, if > the view is not opaque, you need full repaint. scrollContentsFastPath :-) ? > This would not be correct. You can have updateInvalidSubRect == true and > objectsFixedInViewport.isEmpty() in the case where all fixed object are under a > transformed layer in the hierarchy. OK. maybe invalidated is better than invalid. Maybe add a comment as the above as well? > > Wrong indentation > > Is it? The coding convention says 4 spaces, not 3. Yes your change is right. I just noticed that you changed the indentation.

Kenneth Rohde Christiansen

Comment 16 2010-01-07 04:25:35 PST

Comment on attachment 46040 [details] Repaint only the invalidated area after scrolling > + (WebCore::FrameView::registerFixedObject): > + (WebCore::FrameView::unregisterFixedObject): I wonder if it would be better to use addFixedObject and removeFixedObject instead as we use add/remove elsewhere (like addSlowRepaintObject)

Antti Koivisto

Comment 17 2010-01-07 04:29:48 PST

(In reply to comment #5) > (4) Why not make fixed background images work too? They are just as common (if > not moreso) than fixed positioning. Fixed background seems to be often used on the root element (covering the entire view), making this optimization ineffective. Optimizing the case would require moving copy-scroll implementation to WebKit too.

Kenneth Rohde Christiansen

Comment 18 2010-01-07 04:50:40 PST

(In reply to comment #15) > > > I dislike the method name. Shouldn't it always be fast? > > > > No, there are some cases when you cannot avoid the slow path. For example, if > > the view is not opaque, you need full repaint. > > scrollContentsFastPath :-) ? The thing is that for me, if there is a fastScrollContents it is not obvious why you would not always use that. If you have a fast path, it sounds as it is something that you can not use in all situations.

Benjamin Poulain

Comment 19 2010-01-07 05:13:08 PST

Created attachment 46047 [details] Repaint only the invalidated area after scrolling Patch updates to apply the comments of Kenneth: > > > I dislike the method name. Shouldn't it always be fast? > > > scrollContentsFastPath :-) ? Done. > OK. maybe invalidated is better than invalid. Done > I wonder if it would be better to use addFixedObject and removeFixedObject > instead as we use add/remove elsewhere (like addSlowRepaintObject) I choose the name register to emphasize the fact that you only register some fixed object. I am afraid that with addFixedObject(), it might be understood that all objects have to be registered.

WebKit Review Bot

Comment 20 2010-01-07 05:17:53 PST

style-queue ran check-webkit-style on attachment 46047 [details] without any errors.

Adam Treat

Comment 21 2010-01-19 12:39:48 PST

(In reply to comment #17) > (In reply to comment #5) > > (4) Why not make fixed background images work too? They are just as common (if > > not moreso) than fixed positioning. > > Fixed background seems to be often used on the root element (covering the > entire view), making this optimization ineffective. Optimizing the case would > require moving copy-scroll implementation to WebKit too. Indeed. I was not aware of this patch, but we've 'optimized' this case by disabling fixed background images given a define: FAST_MOBILE_SCROLLING See here: https://bugs.webkit.org/show_bug.cgi?id=33408

Benjamin Poulain

Comment 22 2010-01-20 01:20:49 PST

Created attachment 46990 [details] Repaint only the invalidated area after scrolling Here is the same patch, just updated so it merge (because of 33373 and 33408). Any chance to get a review? This patch has been there for a long time.

Benjamin Poulain

Comment 23 2010-01-20 03:11:23 PST

Created attachment 46999 [details] Repaint only the invalidated area after scrolling Oops, the previous was a wrong attachment

Andreas Kling

Comment 24 2010-01-20 03:24:37 PST

Works beautifully, Benjamin! Noticeable improvement on Facebook.

Antti Koivisto

Comment 25 2010-01-20 04:02:45 PST

Comment on attachment 46999 [details] Repaint only the invalidated area after scrolling Looks good, just one question below. > +void FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) > +{ > + const int fixedObjectNumberThreshold = 5; > + > + if (m_fixedObjects.isEmpty()) > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + else { > + Vector<RenderObject*> fixedObjectsInViewport; > + fixedObjectsInViewport.reserveCapacity(m_fixedObjects.size()); > + > + bool updateInvalidatedSubRect = true; > + // Get a list of fixed objects that are not in transformations > + HashSet<RenderObject*>::const_iterator end = m_fixedObjects.end(); > + HashSet<RenderObject*>::const_iterator it = m_fixedObjects.begin(); > + for (; it != end; ++it) { > + RenderObject* obj = *it; > + if (obj->containingBlock() == obj->view()) { Why is this test needed? Isn't it true for all fixed positioned objects in the hash? > + // Methods to manage the objects that are fixed > + // in the view when scrolling > + void registerFixedObject(RenderObject* object); > + void unregisterFixedObject(RenderObject* object); (un)registerFixedPositionedObject would be a better name > + HashSet<RenderObject*> m_fixedObjects; m_fixedPositionedObjects

Adam Treat

Comment 26 2010-01-20 05:47:47 PST

(In reply to comment #22) > Created an attachment (id=46990) [details] > Repaint only the invalidated area after scrolling > > Here is the same patch, just updated so it merge (because of 33373 and 33408). > Any chance to get a review? This patch has been there for a long time. Need to remove the FIXME that this patch fixes: --// FIXME: A better solution would be to only invalidate the fixed regions when scrolling. It's overkill to --// prevent the entire view from blitting on a scroll.

Benjamin Poulain

Comment 27 2010-01-20 07:34:36 PST

Created attachment 47029 [details] Repaint only the invalidated area after scrolling Update from the comments: > Why is this test needed? Isn't it true for all fixed positioned objects in the > hash? I added a comment as you suggested on IRC. > (un)registerFixedPositionedObject would be a better name > m_fixedPositionedObjects Updated. > Need to remove the FIXME that this patch fixes: > > --// FIXME: A better solution would be to only invalidate the fixed regions > when scrolling. It's overkill to > --// prevent the entire view from blitting on a scroll. Good point :)

Antti Koivisto

Comment 28 2010-01-20 07:42:03 PST

r=me

WebKit Commit Bot

Comment 29 2010-01-20 18:35:48 PST

Comment on attachment 47029 [details] Repaint only the invalidated area after scrolling Rejecting patch 47029 from commit-queue. Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Last 500 characters of output: ers/eseidel/Projects/CommitQueue/WebCore/page/mac/FrameMac.mm -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/FrameMac.o ** BUILD FAILED ** The following build commands failed: WebCore: Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/FrameView.o /Users/eseidel/Projects/CommitQueue/WebCore/page/FrameView.cpp normal i386 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://webkit-commit-queue.appspot.com/results/201794

Benjamin Poulain

Comment 30 2010-01-21 06:04:03 PST

Created attachment 47113 [details] Repaint only the invalidated area after scrolling Same patch with size_t for the return type of Vector::size(). I do not have "-Wno-sign-conversion" on my configuration and did not notice this warning.

Benjamin Poulain

Comment 31 2010-01-21 06:05:38 PST

Created attachment 47114 [details] Repaint only the invalidated area after scrolling The previous has \t for indent

WebKit Commit Bot

Comment 32 2010-01-21 11:20:40 PST

Comment on attachment 47114 [details] Repaint only the invalidated area after scrolling Rejecting patch 47114 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12023 test cases. http/tests/misc/acid3.html -> crashed Exiting early after 1 failures. 11308 tests run. 431.48s total testing time 11307 test cases (99%) succeeded 1 test case (<1%) crashed 6 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/204145

Benjamin Poulain

Comment 33 2010-01-21 11:27:57 PST

(In reply to comment #32) > Exiting early after 1 failures. 11308 tests run. > 431.48s total testing time > > 11307 test cases (99%) succeeded > 1 test case (<1%) crashed > 6 test cases (<1%) had stderr output This is strange. I will start a new build on Mac tomorrow and investigate.

Eric Seidel (no email)

Comment 34 2010-01-21 15:37:19 PST

It's possible acid3.html was just flakey. I will attach the crash log.

Eric Seidel (no email)

Comment 35 2010-01-21 15:38:01 PST

Created attachment 47153 [details] crash log from commit bot

Benjamin Poulain

Comment 36 2010-01-22 02:48:05 PST

Created attachment 47186 [details] Repaint only the invalidated area after scrolling (In reply to comment #35) > Created an attachment (id=47153) [details] > crash log from commit bot Thanks Eric. Unregistering the RenderObject in the destructor was a bad idea. For some node types, the node is reseted before the destructor (causing this crash). Working around that problem in the destructor might cause leaks since the reference would never be unregistered. So I have moved the code: if (m_style && m_style->position() == FixedPosition) { FrameView* frameView = document()->view(); if (frameView) frameView->unregisterFixedPositionedObject(this); } from the destructor to RenderObject::destroy(), which is called by Node::detach(), making sure the object is unregistered as soon as the node is detached. This is good news because I was not totally happy with this code in the destructor (because the frame was not always available).

WebKit Commit Bot

Comment 37 2010-01-22 04:01:36 PST

Comment on attachment 47186 [details] Repaint only the invalidated area after scrolling Clearing flags on attachment: 47186 Committed r53693: <http://trac.webkit.org/changeset/53693>

WebKit Commit Bot

Comment 38 2010-01-22 04:01:46 PST

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 39 2010-01-22 13:31:56 PST

Fails on <http://reviews.cnet.com/network-storage/apple-time-capsule-1tb/4505-3382_7-33786176.html?tag=centerColumnArea1.1>. Needs to be rolled out or fixed ASAP.

Darin Adler

Comment 40 2010-01-22 13:43:34 PST

This patch is fine on other platforms, but not on Mac. On Mac the fast path does not involve ScrollView or FrameView at all!

Adam Treat

Comment 41 2010-01-22 14:02:27 PST

(In reply to comment #40) > This patch is fine on other platforms, but not on Mac. On Mac the fast path > does not involve ScrollView or FrameView at all! Can you explain? Last time I looked mac has '!canBlitOnScroll()' for these as well...

mitz

Comment 42 2010-01-22 14:04:35 PST

Reverted the change in <http://trac.webkit.org/projects/webkit/changeset/53713>, because it broke scrolling of pages with fixed elements on Mac OS X, for example <http://reviews.cnet.com/network-storage/apple-time-capsule-1tb/4505-3382_7-33786176.html?tag=centerColumnArea1.1>. This optimization needs to be made in a way that works with Mac OS X, or done in a way that doesn’t affect it for now.

Benjamin Poulain

Comment 43 2010-01-22 15:17:13 PST

(In reply to comment #40) > This patch is fine on other platforms, but not on Mac. On Mac the fast path > does not involve ScrollView or FrameView at all! I was not aware of that. What do you use instead? Do you have a suggestion to fix that or should I #ifdef to have the old behavior on Mac?

Darin Adler

Comment 44 2010-01-22 16:47:47 PST

The relevant code here is the function platformSetCanBlitOnScroll, called from ScrollView::setCanBlitOnScroll. This should have been the clue that there was some issue on at least one platform. Once that’s called with the value set to true, on the Mac, WebKit has told AppKit it can scroll by blitting without even calling through to WebCore. Generally speaking, we can't easily change the ScrollView-level setCanBlitOnScroll function to mean some sort of smarter blitting and retain the current behavior on Mac OS X, so maybe we should have a separate bit to reflect this new intermediate state. Some sort of ifdef solution might be OK. Or some kind of runtime solution.

Benjamin Poulain

Comment 45 2010-01-23 04:42:25 PST

Created attachment 47270 [details] Repaint only the invalidated area after scrolling The same patch with minor modification for Mac. As Darin Adler pointed out, there is a different path for scrolling when the view is a platformWidget(): bool ScrollView::scroll(ScrollDirection direction, ScrollGranularity granularity) { if (platformWidget()) return platformScroll(direction, granularity); In this case, ScrollView::scrollContents() is never used. So, if platformWidget() we need the same behavior as before. I have change the object registration function to check for platformWidget(): void FrameView::registerFixedPositionedObject(RenderObject* object) { if (platformWidget() && m_fixedPositionedObjects.isEmpty()) setCanBlitOnScroll(false); m_fixedPositionedObjects.add(object); } void FrameView::unregisterFixedPositionedObject(RenderObject* object) { bool wasEmpty = m_fixedPositionedObjects.isEmpty(); m_fixedPositionedObjects.remove(object); if (platformWidget() && !wasEmpty && m_fixedPositionedObjects.isEmpty()) setCanBlitOnScroll(!useSlowRepaints()); } And the same for FrameView::useSlowRepaints: bool FrameView::useSlowRepaints() const { return m_useSlowRepaints || m_slowRepaintObjectCount > 0 || (platformWidget() && !m_fixedPositionedObjects.isEmpty()) || m_isOverlapped || !m_contentIsOpaque; } bool FrameView::useSlowRepaintsIfNotOverlapped() const { return m_useSlowRepaints || m_slowRepaintObjectCount > 0 || (platformWidget() && !m_fixedPositionedObjects.isEmpty()) || !m_contentIsOpaque; }

Simon Hausmann

Comment 46 2010-01-23 09:25:27 PST

Comment on attachment 47270 [details] Repaint only the invalidated area after scrolling Small suggestion :) > +void FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) > +{ > + const size_t fixedObjectNumberThreshold = 5; > + > + if (m_fixedPositionedObjects.isEmpty()) > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + else { > + Vector<RenderObject*> fixedObjectsInViewport; > + fixedObjectsInViewport.reserveCapacity(m_fixedPositionedObjects.size()); AFAICT here we are allocating the vector buffer on the speak. Once only, but it's still a (fastmalloc'ed) heap allocation, that is technically not needed. We know the size of the buffer, an allocation on the stack is cheaper, especially for the little amount it takes (5 pointers + alignment). Why not use Vector's inline capacity feature: Vector<RenderObject*, fixedObjectNumberThreshold> fixedObjectsInViewport; If I understand the wtf::Vector code correctly that will allocate the vector's buffer nicely on the stack.

Simon Hausmann

Comment 47 2010-01-23 09:26:27 PST

(In reply to comment #46) > AFAICT here we are allocating the vector buffer on the speak. Once only, but speap = heap :)

Benjamin Poulain

Comment 48 2010-01-23 15:43:21 PST

Created attachment 47283 [details] Repaint only the invalidated area after scrolling Updated the patch with the comment of Simon (In reply to comment #46) > AFAICT here we are allocating the vector buffer on the speak. Once only, but > it's still a (fastmalloc'ed) heap allocation, that is technically not needed. > We know the size of the buffer, an allocation on the stack is cheaper, > especially for the little amount it takes (5 pointers + alignment). That's a very good idea. > Why not use Vector's inline capacity feature: > > Vector<RenderObject*, fixedObjectNumberThreshold> fixedObjectsInViewport; > > If I understand the wtf::Vector code correctly that will allocate the vector's > buffer nicely on the stack. That's a very cool feature :)

Simon Hausmann

Comment 49 2010-01-24 03:25:19 PST

Comment on attachment 47283 [details] Repaint only the invalidated area after scrolling > +void FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) > +{ > + const size_t fixedObjectNumberThreshold = 5; > + > + if (m_fixedPositionedObjects.isEmpty()) > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + else { > + Vector<RenderObject*, fixedObjectNumberThreshold> fixedObjectsInViewport; > + > + bool updateInvalidatedSubRect = true; > + // Get a list of fixed objects that are not in transformations > + HashSet<RenderObject*>::const_iterator end = m_fixedPositionedObjects.end(); > + HashSet<RenderObject*>::const_iterator it = m_fixedPositionedObjects.begin(); > + for (; it != end; ++it) { > + RenderObject* obj = *it; > + // make sure the parent layer has not been transformed > + if (obj->containingBlock() == obj->view()) { > + fixedObjectsInViewport.append(obj); > + if (fixedObjectsInViewport.size() > fixedObjectNumberThreshold) { After my suggestion there is now one little problem with the above code: append() will be called for the 6th render object, and then the vector will be re-allocated on the heap. Right afterwards we find out that we crossed the threshold. I suggest to either allocate the vector with fixedObjectNumberThreshold + 1 or check size against capacity later on.

Benjamin Poulain

Comment 50 2010-01-24 07:20:44 PST

Comment on attachment 47283 [details] Repaint only the invalidated area after scrolling Remove review flag. Need to update the patch.

Benjamin Poulain

Comment 51 2010-01-24 11:27:10 PST

Created attachment 47297 [details] Repaint only the invalidated area after scrolling Change the if() to avoid the problem Simon pointed out.

WebKit Review Bot

Comment 52 2010-01-24 11:34:26 PST

Attachment 47297 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/page/FrameView.cpp:849: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/page/FrameView.cpp:854: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.

Benjamin Poulain

Comment 53 2010-01-25 01:10:39 PST

Created attachment 47325 [details] Repaint only the invalidated area after scrolling Stupid style guideline, I like the other version better ;)

WebKit Commit Bot

Comment 54 2010-01-25 04:37:43 PST

Comment on attachment 47325 [details] Repaint only the invalidated area after scrolling Clearing flags on attachment: 47325 Committed r53797: <http://trac.webkit.org/changeset/53797>

WebKit Commit Bot

Comment 55 2010-01-25 04:37:56 PST

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

Comment 56 2010-01-25 21:29:47 PST

bug 34153 suggests that this caused a regression.

Darin Adler

Comment 57 2010-01-26 07:54:46 PST

Hyatt needs to comment on this. Yesterday he told me there were many small things wrong with this patch that he intended to remark on before it was landed, and that a proper implementation would work well for Mac OS X too. But it got reviewed and landed quickly before he had a chance to identify those items.

Benjamin Poulain

Comment 58 2010-01-26 08:35:16 PST

(In reply to comment #57) > Hyatt needs to comment on this. Yesterday he told me there were many small > things wrong with this patch that he intended to remark on before it was > landed, and that a proper implementation would work well for Mac OS X too. Great. I was hoping he could look at this patch.

Benjamin Poulain

Comment 59 2010-01-27 00:06:20 PST

Patch reverted by 34153. RenderWidget::destroy() must have a copy of what is added to RenderObject::destroy().

Benjamin Poulain

Comment 60 2010-01-28 08:50:23 PST

Created attachment 47622 [details] Repaint only the invalidated area after scrolling I have added the change to RenderWidget that caused https://bugs.webkit.org/show_bug.cgi?id=34153 I have also added a test case for the RenderWidget crash.

Kenneth Rohde Christiansen

Comment 61 2010-01-28 09:00:45 PST

Hyatt, does this new patch look good to you? or do you have any further comments?

Dave Hyatt

Comment 62 2010-01-28 17:20:27 PST

Comment on attachment 47622 [details] Repaint only the invalidated area after scrolling Some comments: (1) You don't really need a hash of RenderObjects on FrameView. A count is sufficient. Then you can just walk the m_positionedObjects list of the RenderView and if the RenderBox is fixed positioned, it's one you care about. (This also bypasses the need to do any transform checking, since you'll only be in the RenderView list if your containing block was the RenderView). (2) Fixed positioned objects are always RenderBoxes, so you need to tighten up the code here. You can rename your methods to refer to use FixedPositionedBox rather than FixedPositionedObject. (3) You've duplicated code in RenderObject::destroy() and RenderWidget::destroy(). If you add a helper function to RenderBox, e.g., unregisterFixedPositionedBox, then RenderWidget could call it in its destroy method. The code in RenderObject should be moved to RenderBox. (3) All the code in RenderObject::styleWillChange is misplaced, since only RenderBoxes can be fixed positioned. You should move the code into RenderBox. (Yes, some of the code in there was already misplaced, but by adding a big new pile of code, you've made the problem worse. :) (4) I dislike "scrollContentsFastPath" when we know what we mean is blitting. scrollContentsWithBlit would be my suggestion, although I don't like my own choice of name much better.

Benjamin Poulain

Comment 63 2010-02-01 07:55:57 PST

Created attachment 47844 [details] Repaint only the invalidated area after scrolling Updated the patch thanks to Dave Hyatt's comments: (In reply to comment #62) > (1) You don't really need a hash of RenderObjects on FrameView. A count is > sufficient. Then you can just walk the m_positionedObjects list of the > RenderView and if the RenderBox is fixed positioned, it's one you care about. > (This also bypasses the need to do any transform checking, since you'll only be > in the RenderView list if your containing block was the RenderView). I have changed the code to use the list contained in RenderBlock. I like that lot better than what I had done previously. To access it from RenderView, I had to change: - void insertPositionedObject(RenderBox*); - void removePositionedObject(RenderBox*); + virtual void insertPositionedObject(RenderBox*); + virtual void removePositionedObject(RenderBox*); void removePositionedObjects(RenderBlock*); + ListHashSet<RenderBox*>* positionedObjects() const { return m_positionedObjects; } > (2) Fixed positioned objects are always RenderBoxes, so you need to tighten up > the code here. You can rename your methods to refer to use FixedPositionedBox > rather than FixedPositionedObject. > (3) You've duplicated code in RenderObject::destroy() and > RenderWidget::destroy(). If you add a helper function to RenderBox, e.g., > unregisterFixedPositionedBox, then RenderWidget could call it in its destroy > method. The code in RenderObject should be moved to RenderBox. I have moved the (un)registration to RenderView::insertPositionedObject() RenderView::removePositionedObject() So the problem of the transformations disappears. > (3) All the code in RenderObject::styleWillChange is misplaced, since only > RenderBoxes can be fixed positioned. You should move the code into RenderBox. > (Yes, some of the code in there was already misplaced, but by adding a big new > pile of code, you've made the problem worse. :) Right. Fixed :) > (4) I dislike "scrollContentsFastPath" when we know what we mean is blitting. > scrollContentsWithBlit would be my suggestion, although I don't like my own > choice of name much better. I have not changed the name yet. I don't know about putting "blit" in the name because we don't blit if we reach the threshold. What about scrollContentsWithBlitIfPossible(), tryScrollContentsWithBlit()? ;)

Dave Hyatt

Comment 64 2010-02-01 12:21:50 PST

Comment on attachment 47844 [details] Repaint only the invalidated area after scrolling This is not quite right. Both fixed and absolute positioned objects can be added to RenderView. so you can't just use the list as is like that. I wasn't suggesting that you change how you detected and added the objects. The way you did that in your old patch is still necessary. The count just lets you not keep a whole separate list, since you can walk the RenderView's list and check if the object is fixed positioned (and if so use it). Rename the variable in ScrollView to m_fixedObjectCount rather than m_positionedObjectCount.

Benjamin Poulain

Comment 65 2010-02-02 11:32:16 PST

Created attachment 47957 [details] Repaint only the invalidated area after scrolling (In reply to comment #64) > (From update of attachment 47844 [details]) > This is not quite right. Both fixed and absolute positioned objects can be > added to RenderView. so you can't just use the list as is like that. I wasn't > suggesting that you change how you detected and added the objects. The way you > did that in your old patch is still necessary. The count just lets you not > keep a whole separate list, since you can walk the RenderView's list and check > if the object is fixed positioned (and if so use it). I did not get that difference "positioned = fixed | absolute". I have changed FrameView::scrollContentsFastPath() to skip the RenderBox that are not in fixed position. For increasing m_fixedObjectCount, I have stayed with doing it in RenderView::insertPositionedObject(). If I do it in RenderBox::styleWillChange(), the counter of fix objects will not be decreased when the parent get a transformation. As I understand, this is not a problem with the current implementation (I have added "if (o->style()->position() == FixedPosition)" though). Your input is welcome on that issue. > Rename the variable in ScrollView to m_fixedObjectCount rather than > m_positionedObjectCount. Fixed. I have also removed the test. It is not longer relevant.

Benjamin Poulain

Comment 66 2010-02-02 12:20:43 PST

Created attachment 47964 [details] Repaint only the invalidated area after scrolling Update the patch after discussions with Dave Hyatt on IRC. As a side note: m_fixedObjectCount is only used for disabling blitting on Mac. The platforms using FrameView always go to scrollContentsFastPath() and figure if the number of fixed rects is higher than the threshold. I guess I could cache the result of this computation until the render tree is invalidated, but I don't think this computation is an issue in front of the time spent in rendering.

Kenneth Rohde Christiansen

Comment 67 2010-02-02 19:44:40 PST

Comment on attachment 47964 [details] Repaint only the invalidated area after scrolling > +void FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) > +{ > + const size_t fixedObjectNumberThreshold = 5; > + > + ListHashSet<RenderBox*>* positionedObjects = 0; > + if (RenderView* root = m_frame->contentRenderer()) > + positionedObjects = root->positionedObjects(); > + > + if (!positionedObjects || positionedObjects->isEmpty()) > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + else { Wouldn't a return above be nicer? instead of the else > + // Get the rects of the fixed objects visible in the rectToScroll > + Vector<IntRect, fixedObjectNumberThreshold> subRectToUpdate; > + bool updateInvalidatedSubRect = true; > + ListHashSet<RenderBox*>::const_iterator end = positionedObjects->end(); > + for (ListHashSet<RenderBox*>::const_iterator it = positionedObjects->begin(); it != end; ++it) { I would prefer "ListHashSet<RenderBox*>::const_iterator it = positionedObjects->begin()" on its own line, followed by "for (; it != end; ++it)" > + RenderBox* r = *it; I find box a nicer variable name than r. r reminds me of rectangle! > + if (!(r->style()->position() == FixedPosition)) Why not just != ? > + continue; > + IntRect topLevelRect; > + IntRect updateRect = r->paintingRootRect(topLevelRect); > + updateRect.move(-scrollX(), -scrollY()); > + updateRect.intersect(rectToScroll); > + if (!updateRect.isEmpty()) { > + if (subRectToUpdate.size() >= fixedObjectNumberThreshold) { number threshold? why not just fixedObjectThreshold. > + updateInvalidatedSubRect = false; > + break; > + } > + subRectToUpdate.append(updateRect); > + } > + } > + > + // Scroll the view > + if (updateInvalidatedSubRect) { > + // 1) scroll > + hostWindow()->scroll(scrollDelta, rectToScroll, clipRect); > + > + // 2) update the area of fixed objets that has been invalidated > + size_t fixObjectsCount = subRectToUpdate.size(); > + for (size_t i = 0; i < fixObjectsCount; ++i) { > + IntRect updateRect = subRectToUpdate[i]; > + IntRect scrolledRect = updateRect; > + scrolledRect.move(scrollDelta); > + updateRect.unite(scrolledRect); > + updateRect.intersect(rectToScroll); > + hostWindow()->repaint(updateRect, true, false, true); > + } > + } else { > + // the number of fixed objects exceed the threshold, so we repaint everything. > + IntRect updateRect = clipRect; > + updateRect.intersect(rectToScroll); > + hostWindow()->repaint(updateRect, true, false, true); > + } > + } > +}

Benjamin Poulain

Comment 68 2010-02-03 00:52:46 PST

Created attachment 48001 [details] Repaint only the invalidated area after scrolling Patch updated (better variable name and code quirks fixed). Thanks Kenneth for noticing those problems. Apparently I was a bit lazy yesterday.

Benjamin Poulain

Comment 69 2010-02-04 05:52:33 PST

Comment on attachment 48001 [details] Repaint only the invalidated area after scrolling The updated area seems not correct for pages with top-level fixed elements with transformations.

Benjamin Poulain

Comment 70 2010-02-05 08:16:40 PST

Comment on attachment 48001 [details] Repaint only the invalidated area after scrolling Put back to review, I think the problem is in paintingRootRect(), not in this patch. With the following HTML: "<html> <body style="height: 5000px;"> <div style="width: 100px; height: 100px; background-color: green; opacity: .5; position: fixed; top: 60px; left: 60px;"></div> <div style="width: 100px; height: 100px; background-color: green; opacity: .5; position: fixed; top: 70px; left: 70px; -webkit-transform: rotate(30deg);"></div> </body> </html>" The rect returned by paintingRootRect() is empty for the second rect (the one with a transformation). The behavior for this case is also different between Safari and webkit trunk. On Safari, the fixed element is not fixed if transformed. On trunk, it is. I have absolutely no idea which behavior is the correct one. Anyway, Dave Hyatt, any chance get a review and your opinion on the good behavior for fixed transformed elements?

Eric Seidel (no email)

Comment 71 2010-02-08 15:13:00 PST

Comment on attachment 47029 [details] Repaint only the invalidated area after scrolling Cleared Antti Koivisto's review+ from obsolete attachment 47029 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Eric Seidel (no email)

Comment 72 2010-02-08 15:13:11 PST

Comment on attachment 47114 [details] Repaint only the invalidated area after scrolling Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 47114 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Adam Barth

Comment 73 2010-03-09 08:34:45 PST

Comment on attachment 48001 [details] Repaint only the invalidated area after scrolling This patch has been waiting for dhyatt's review for over a month. In the meantime, the bug has gone epic and requires half an hour to understand what's going on with this code. In the interest of moving the project forward, I'm marking this patch r+. Benjamin has gotten significant feedback from a number of reviewers and has diligently addressed all the feedback he's received (with the possible exception of a name change requested by dhyatt). Based on our experience with this patch, I suspect this patch will break things when landed again. However, I don't know how else to move this issue forward. Having patches sitting around waiting for review for a month is bad for the project. I would encourage folks working in this area to improve the test coverage of our scrolling code. The regressions caused by earlier iterations of this patch point to a distinct lack of test coverage. I was tempted to block this patch on better test coverage, but that seemed unfair to Benjamin to appears to have been working on this patch in ernest.

Adam Treat

Comment 74 2010-03-09 08:53:16 PST

Note: this patch will need to be updated since the methods in HostWindow have changed with regard to invalidation/repaint before it'll build against ToT.

Benjamin Poulain

Comment 75 2010-03-09 09:54:27 PST

(In reply to comment #73) > (From update of attachment 48001 [details]) > In the interest of moving the project forward, I'm marking this patch r+. > Benjamin has gotten significant feedback from a number of reviewers and has > diligently addressed all the feedback he's received (with the possible > exception of a name change requested by dhyatt). Thank you very much. I had given up the hope to see that patch landed. (In reply to comment #74) > Note: this patch will need to be updated since the methods in HostWindow have > changed with regard to invalidation/repaint before it'll build against ToT. I will update the patch, I'll try to do that today.

Benjamin Poulain

Comment 76 2010-03-09 15:04:08 PST

Created attachment 50355 [details] Repaint only the invalidated area after scrolling The same patch updated to use the new functions added by Adam Treat to HostWindow. I will be happy to fix it if any new problem is discovered :)

Adam Treat

Comment 77 2010-03-09 15:33:08 PST

Comment on attachment 50355 [details] Repaint only the invalidated area after scrolling > + // 2) update the area of fixed objets that has been invalidated Typo. This should be 'objects'. > + size_t fixObjectsCount = subRectToUpdate.size(); > + for (size_t i = 0; i < fixObjectsCount; ++i) { > + IntRect updateRect = subRectToUpdate[i]; > + IntRect scrolledRect = updateRect; > + scrolledRect.move(scrollDelta); > + updateRect.unite(scrolledRect); > + updateRect.intersect(rectToScroll); > + hostWindow()->invalidateContentsForSlowScroll(updateRect, false); I don't think you want to use 'invalidateContentsForSlowScroll' here. Rather, I think 'invalidateContentsAndWindow' is the appropriate method call. Otherwise you need to do it synchronously. > + } > + } else { > + // the number of fixed objects exceed the threshold, so we repaint everything. > + IntRect updateRect = clipRect; > + updateRect.intersect(rectToScroll); > + hostWindow()->invalidateContentsForSlowScroll(updateRect, false); > + } > +} I don't like that 'invalidateContentsForSlowScroll' is being called from a method named 'scrollContentsFastPath.' This is confusing and I wonder if it points to a larger issue. How about if scrollContentsFastPath returned true if it was possible, but false if not? Then the latter could be invoked.

Benjamin Poulain

Comment 78 2010-03-10 02:45:57 PST

Created attachment 50386 [details] Repaint only the invalidated area after scrolling > (From update of attachment 50355 [details]) > > + // 2) update the area of fixed objets that has been invalidated > > Typo. This should be 'objects'. Fixed :) > > + size_t fixObjectsCount = subRectToUpdate.size(); > > + for (size_t i = 0; i < fixObjectsCount; ++i) { > > + IntRect updateRect = subRectToUpdate[i]; > > + IntRect scrolledRect = updateRect; > > + scrolledRect.move(scrollDelta); > > + updateRect.unite(scrolledRect); > > + updateRect.intersect(rectToScroll); > > + hostWindow()->invalidateContentsForSlowScroll(updateRect, false); > > I don't think you want to use 'invalidateContentsForSlowScroll' here. Rather, > I think 'invalidateContentsAndWindow' is the appropriate method call. > Otherwise you need to do it synchronously. I have to admit I don't get the difference between invalidateContentsForSlowScroll() and invalidateContentsAndWindow(). It seems specific to the way Windows handle scrolling, I trust you on that one. > I don't like that 'invalidateContentsForSlowScroll' is being called from a > method named 'scrollContentsFastPath.' This is confusing and I wonder if it > points to a larger issue. How about if scrollContentsFastPath returned true if > it was possible, but false if not? Then the latter could be invoked. I agree, it is clearer if scrollContentsFastPath() never invoke the slow path. I have made the modification you suggested.

WebKit Review Bot

Comment 79 2010-03-10 02:52:22 PST

Attachment 50386 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/ScrollView.cpp:516: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Benjamin Poulain

Comment 80 2010-03-10 03:01:02 PST

Created attachment 50387 [details] Repaint only the invalidated area after scrolling Code style fix.

Adam Treat

Comment 81 2010-03-10 06:56:37 PST

> I have to admit I don't get the difference between > invalidateContentsForSlowScroll() and invalidateContentsAndWindow(). It seems > specific to the way Windows handle scrolling, I trust you on that one. The latter (in the way you are using it) notifies that the provided rect needs to be repainted to the backingstore and then copied to the screen. Asynchronously. The former (in the way you were using it) notifies that the provided rect needs to be repainted to the backingstore. Asynchronously. I believe that it should be done synchronously, but hyatt and apple's windows port disagree. But that is for another day.

Adam Treat

Comment 82 2010-03-10 08:07:38 PST

Comment on attachment 50387 [details] Repaint only the invalidated area after scrolling You've addressed all the problems I specifically saw with the updated patch. For this reason I am falling back to Adam Barth's previous status of r+.

WebKit Commit Bot

Comment 83 2010-03-11 23:07:11 PST

Comment on attachment 50387 [details] Repaint only the invalidated area after scrolling Clearing flags on attachment: 50387 Committed r55890: <http://trac.webkit.org/changeset/55890>

WebKit Commit Bot

Comment 84 2010-03-11 23:07:22 PST

All reviewed patches have been landed. Closing bug.

Simon Hausmann

Comment 85 2010-03-22 04:45:23 PDT

Cherry-picked into qtwebkit-4.6 with commit cd258386aa3564cd4c961933401d0f86a4c872f8

Simon Hausmann

Comment 86 2010-03-22 04:46:25 PDT

(In reply to comment #85) > Cherry-picked into qtwebkit-4.6 with commit > cd258386aa3564cd4c961933401d0f86a4c872f8 Err, that is d95c54951e7af2aa7def4346a142b2162bd89bbd

Shinichiro Hamaji

Comment 87 2010-03-29 01:15:00 PDT

It seemed r55890 caused a regression at least for Qt and Chromium (I guess this issue will happen all environments where platformWidget() == NULL). I confirmed reverting r55890 fixes the issue. How to reproduce: - Run Chromium or QtLauncher - Download http://chromium.googlecode.com/issues/attachment?aid=8184776065656844667&name=testcase.xht and open it with your browser - You would notice the image will be wrongly rendered when you scroll I didn't understand the patch yet, but it seems we are using the fast path when we cannot use it. Related Chromium bugs: http://crbug.com/39075 http://crbug.com/39394 By the way, I couldn't understand two changes in the patch. They might not be related to this issue though. 1. Comment 45 said we won't change the behavior when platformWidget is non-NULL, but the change for useSlowRepaints() seems to be changing the behavior. 2. I wasn't sure why we call invalidateContentsAndWindow() instead of scroll() when fast scroll fails (this should not be related the issue I'm reporting because scrollContentsFastPath returns true with the testcase). > - // FIXME: Find a way to scroll subframes with this faster path > - hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect); > + // FIXME: Find a way to scroll subframes with this faster path > + if (!scrollContentsFastPath(-scrollDelta, scrollViewRect, clipRect)) > + hostWindow()->invalidateContentsForSlowScroll(updateRect, false); Again, I didn't understand the patch well, I'm sorry if my uneducated guesses were completely pointless. Benjamin, could you look into this? If it will take a few days to fix this issue, I'd like to revert this patch for now (and hopefully a refined patch will be landed with thorough tests). Thanks,

Benjamin Poulain

Comment 88 2010-03-29 01:39:29 PDT

(In reply to comment #87) > How to reproduce: > > - Run Chromium or QtLauncher > - Download > http://chromium.googlecode.com/issues/attachment?aid=8184776065656844667&name=testcase.xht > and open it with your browser > - You would notice the image will be wrongly rendered when you scroll I cannot reproduce the problem with QtLauncher (use FrameView) nor Safari trunk (no FrameView, platformWidget). I don't have Chronium trunk, I will ask Andreas if he can test the issue. The code should not go at all to scrollContentsFastPath() if you have a platformWidget, I need to investigate that. It is strange I don't have the problem with Safari. > 1. Comment 45 said we won't change the behavior when platformWidget is > non-NULL, but the change for useSlowRepaints() seems to be changing the > behavior. It shouldn't. Before the patch, WebKit was doing a full update as soon as a fixed element was in the page (check the change of RenderObject). Now it only does so if you have a PlatformWidget (that is why I needed addFixedObject/removeFixedObject) and changed useSlowRepaints(). > 2. I wasn't sure why we call invalidateContentsAndWindow() instead of scroll() > when fast scroll fails (this should not be related the issue I'm reporting > because scrollContentsFastPath returns true with the testcase). > > > - // FIXME: Find a way to scroll subframes with this faster path > > - hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect); > > + // FIXME: Find a way to scroll subframes with this faster path > > + if (!scrollContentsFastPath(-scrollDelta, scrollViewRect, clipRect)) > > + hostWindow()->invalidateContentsForSlowScroll(updateRect, false); The call to scrollContentsFastPath() returns false if it is not possible to use the fast path. In that case, we go the old way and do a full repaint. > Benjamin, could you look into this? Sure. I have to look at 36686, 36652 and this one. I will try to reproduce the issue as soon as I get my hand on a recent Chronium. > If it will take a few days to fix this > issue, I'd like to revert this patch for now (and hopefully a refined patch > will be landed with thorough tests). I would really, really prefer if we can avoid a revert. Landing this patch as been a pain in the ass, nobody was willing to review it, it stayed in queue for months.

Shinichiro Hamaji

Comment 89 2010-03-29 02:11:55 PDT

> I cannot reproduce the problem with QtLauncher (use FrameView) nor Safari trunk > (no FrameView, platformWidget). > I don't have Chronium trunk, I will ask Andreas if he can test the issue. I forgot to notice this doesn't happen with Mac Safari. It seemed Chromium and Qt don't have platformWidget so I'm guessing this happens only when platformWidget is NULL. Hmm... it's strange you cannot reproduce this issue with QtLauncher. I've just confirmed again this issue happens with my debug build QtLauncher (r56710) on Linux. Please let me know if there are any other info I can tell. To make sure we are testing the same HTML, I've put the testcase HTML here: http://shinh.skr.jp/t/testcase.html > Sure. I have to look at 36686, 36652 and this one. I will try to reproduce the issue as soon as I get my hand on a recent Chronium. Thanks! The easiest way to see what is happening with Chromium would be using our latest continuous build: http://build.chromium.org/buildbot/continuous/ If this only happens with Chromium and I'm doing something wrong with my QtLauncher, it would be nice if we can safely disable this optimization with messy #if PLATFORM(CHROMIUM) for now. Could you tell me the easiest way to disable this?

Shinichiro Hamaji

Comment 90 2010-03-29 02:41:26 PDT

Created attachment 51893 [details] Screenshot with Win Safari I've seen a similar, but different issue with Win Safari. See the attached screenshot. It didn't happen with r55872 but happened with r55961. I used nightly build (http://nightly.webkit.org/builds/trunk/win/1) so I cannot find the exact version soon. I'll check if reverting the patch also fixes this issue.

Benjamin Poulain

Comment 91 2010-03-29 03:13:12 PDT

(In reply to comment #89) > The easiest way to see what is happening with Chromium would be using > our latest continuous build: http://build.chromium.org/buildbot/continuous/ I can reproduce the issue with the latest build. > Hmm... it's strange you cannot reproduce this issue with QtLauncher. I've just > confirmed again this issue happens with my debug build QtLauncher (r56710) on > Linux. Please let me know if there are any other info I can tell. > > To make sure we are testing the same HTML, I've put the testcase HTML here: > http://shinh.skr.jp/t/testcase.html > > If this only happens with Chromium and I'm doing something wrong with my > QtLauncher, it would be nice if we can safely disable this optimization with > messy #if PLATFORM(CHROMIUM) for now. Could you tell me the easiest way to > disable this? I found why I could not reproduce the issue. My branch of WebKit have the patch of 36652 applied. If you can wait a day, I will try to do 36686 and resubmit 36652 today.

Shinichiro Hamaji

Comment 92 2010-03-29 03:41:50 PDT

> I found why I could not reproduce the issue. My branch of WebKit have the patch > of 36652 applied. > > If you can wait a day, I will try to do 36686 and resubmit 36652 today. Great. I confirmed the patch in Bug 36652 fixed the chromium's issue. I hope you'll get r+ soon. Thanks for your investigation!

James Robinson

Comment 93 2010-04-06 13:28:06 PDT

I am planning to revert this patch (except for part that keeps a count of position:fixed elements in a frame since later patches depend on it). Scrolling has been broken on pages with fixed position elements with overflow on all platforms except for Safari Mac for almost a month now. The patch in 36652 is wrong and it's not immediately obvious that it can be fixed in short amount of time.

James Robinson

Comment 94 2010-04-06 13:35:22 PDT

Created attachment 52663 [details] Patch

James Robinson

Comment 95 2010-04-06 13:45:09 PDT

Comment on attachment 52663 [details] Patch Clearing flags on attachment: 52663 Committed r57165: <http://trac.webkit.org/changeset/57165>

James Robinson

Comment 96 2010-04-06 13:45:23 PDT

All reviewed patches have been landed. Closing bug.

James Robinson

Comment 97 2010-04-06 13:46:23 PDT

Revert landed. Reopening, I'm very excited about having this optimization landed (when it's correct, of course).

Eric Seidel (no email)

Comment 98 2010-04-06 23:44:33 PDT

Comment on attachment 48001 [details] Repaint only the invalidated area after scrolling Cleared Adam Barth's review+ from obsolete attachment 48001 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Benjamin Poulain

Comment 99 2010-05-03 08:02:38 PDT

Created attachment 54927 [details] Updated version of the patch The patch now use the cached repaint rect of RenderLayer. This is a lot faster than the previous one, and takes into account overflow/border/etc.

Benjamin Poulain

Comment 100 2010-05-10 11:12:44 PDT

James, could you have a look if you can find a way to break this patch? This would probably help reviewers.

Simon Hausmann

Comment 101 2010-05-20 05:00:53 PDT

James: ping

James Robinson

Comment 102 2010-05-20 13:47:55 PDT

I will take a look at it this week and provide feedback. Note that you'll still need someone else to r+ it as I am not a reviewer (most likely hyatt, mitz, or smfr).

Darin Fisher (:fishd, Google)

Comment 103 2010-05-20 14:22:33 PDT

I notice rendering problems on cnet.com with this patch applied to a chromium build. The fixed position bar at the bottom of the page jumps up and down as I scroll the page. It looks like some of the repaints associated with the fixed position elements may be getting scheduled asynchronously. I haven't investigated yet, so I'm not sure how to explain this bug.

Benjamin Poulain

Comment 104 2010-05-21 02:49:00 PDT

(In reply to comment #103) > It looks like some of the repaints associated with the fixed position elements may be getting scheduled asynchronously. I haven't investigated yet, so I'm not sure how to explain this bug. It sounds like your implementation of Chrome::scroll() is synchronous, and somehow Chrome::invalidateContentsAndWindow() is asynchronous. On Qt Linux/Mac, the first expose event after Chrome::scroll() is the one triggered by Chrome::invalidateContentsAndWindow(). Andreas is building Chrome for Linux with this patch, hoping to reproduce the issue.

Benjamin Poulain

Comment 105 2010-05-21 03:26:50 PDT

(In reply to comment #103) > I notice rendering problems on cnet.com with this patch applied to a chromium build. The fixed position bar at the bottom of the page jumps up and down as I scroll the page. Andreas built Chronium for Linux with this patch and we do not see the problem you describe. Which platform are you using?

Darin Fisher (:fishd, Google)

Comment 106 2010-06-02 22:22:51 PDT

(In reply to comment #105) > (In reply to comment #103) > > I notice rendering problems on cnet.com with this patch applied to a chromium build. The fixed position bar at the bottom of the page jumps up and down as I scroll the page. > > Andreas built Chronium for Linux with this patch and we do not see the problem you describe. Which platform are you using? I was testing on Windows. Our implementations of scroll and invalidateContentsAndWindow are both asynchronous. They get added to a queue, which we process later. We merge damage rects and account for overlapping scroll rects appropriately. I was testing cnet.com and pages like it that have a prominent fixed position element. I can try a Linux build as well when I get the chance. I can't explain why you wouldn't see the same behavior on Linux as I see on Windows since the rendering model is the same at this level.

Benjamin Poulain

Comment 107 2010-06-04 10:24:10 PDT

(In reply to comment #106) > I was testing on Windows. Our implementations of scroll and invalidateContentsAndWindow are both asynchronous. They get added to a queue, which we process later. We merge damage rects and account for overlapping scroll rects appropriately. I was testing cnet.com and pages like it that have a prominent fixed position element. I can try a Linux build as well when I get the chance. I can't explain why you wouldn't see the same behavior on Linux as I see on Windows since the rendering model is the same at this level. Could it be that the update() is hitting the event loop after the scroll() on Windows? That could be a bug in the event delivery of Chrome. I begin to wonder if this patch should be "#ifdef QT". Would it be a solution to unblock the situation?

Darin Fisher (:fishd, Google)

Comment 108 2010-06-09 15:59:01 PDT

An ENABLE flag seems reasonable, as it would allow ports to opt-in once they are ready to do so. I'd be surprised if this is a bug on the Chrome side given that we already exercise scrolling with overlapped paint rects, but I need to debug it further to be sure. I suspect that WebKit is not synchronously reporting all invalidations.

Darin Fisher (:fishd, Google)

Comment 109 2010-06-09 23:04:07 PDT

(In reply to comment #108) > An ENABLE flag seems reasonable, as it would allow ports to opt-in once they > are ready to do so. > > I'd be surprised if this is a bug on the Chrome side given that we already > exercise scrolling with overlapped paint rects, but I need to debug it further > to be sure. I suspect that WebKit is not synchronously reporting all > invalidations. OK, I understand the issue now. This is a problem caused by Chromium's use of ScrollWindowEx. Sometimes a v-sync can happen between the ScrollWindowEx and the RedrawWindow(RGW_UPDATENOW) call that paints the exposed region. This results in the intermediate state being shown to the user. I think I need to address this issue on the Chromium side. Sorry for the hassle!

Darin Fisher (:fishd, Google)

Comment 110 2010-06-10 09:00:44 PDT

By the way, I confirm that this patch works properly for focus rings around text inputs and css box shadows.

Noam Rosenthal

Comment 111 2010-06-11 13:05:50 PDT

*** Bug 38418 has been marked as a duplicate of this bug. ***

Noam Rosenthal

Comment 112 2010-06-11 15:33:18 PDT

It could have been 39918 then. The point is the bug is no longer there.

Kenneth Rohde Christiansen

Comment 113 2010-06-13 07:57:39 PDT

Hyatt, Simon Fraser, any one of you having time to review this patch now?

Simon Hausmann

Comment 114 2010-06-16 23:20:00 PDT

So should we still have an ENABLE() flag or !PLATFORM(CHROMIUM) or is it safe to land the current patch?

Darin Fisher (:fishd, Google)

Comment 115 2010-06-17 09:46:50 PDT

(In reply to comment #114) > So should we still have an ENABLE() flag or !PLATFORM(CHROMIUM) or is it safe to land the current patch? No need! I landed a patch in Chromium that addresses the issue I'm seeing. I'm looking forward to this patch landing as it really improves scrolling performance on popular sites like nytimes.com.

Dave Hyatt

Comment 116 2010-06-17 11:47:46 PDT

Comment on attachment 54927 [details] Updated version of the patch Looks pretty close. There's one bogus code section though: The offsetX / offsetY computation that just loops up through the parent ScrollViews. There's more complexity to converting to window coordinates than just adding in offsets like that. Fortunately you can just use contentsToWindow on the rect to convert it. This will work in the presence of transforms, unlike the code you wrote.

Benjamin Poulain

Comment 117 2010-06-17 13:33:18 PDT

Created attachment 59031 [details] Patch > (From update of attachment 54927 [details]) > Looks pretty close. There's one bogus code section though: The offsetX / offsetY computation that just loops up through the parent ScrollViews. There's more complexity to converting to window coordinates than just adding in offsets like that. Fortunately you can just use contentsToWindow on the rect to convert it. This will work in the presence of transforms, unlike the code you wrote. Sweet. I have updated the patch to use contentsToWindow in order to convert the update rect. Thanks for the review!

Kenneth Rohde Christiansen

Comment 118 2010-06-21 13:17:12 PDT

Comment on attachment 59031 [details] Patch As you have fixed the only issue Hyatt pointed out, it should be OK to land this now. r=me.

Kenneth Rohde Christiansen

Comment 119 2010-06-21 13:17:32 PDT

Comment on attachment 59031 [details] Patch As you have fixed the only remaining issue Hyatt pointed out, it should be OK to land this now. r=me.

Benjamin Poulain

Comment 120 2010-06-23 07:12:27 PDT

Committed r61686: <http://trac.webkit.org/changeset/61686>

Simon Hausmann

Comment 121 2010-06-28 01:28:30 PDT

Revision r61686 cherry-picked into qtwebkit-2.0 with commit bf2d8aa3dc238a9f893f8d7b35fa06c083cbb9b5

Simon Hausmann

Comment 122 2010-06-28 01:57:54 PDT

Revision r61686 cherry-picked into qtwebkit-2.0 with commit bf2d8aa3dc238a9f893f8d7b35fa06c083cbb9b5

Note You need to log in before you can comment on or make changes to this bug.

Status CLOSED

Resolution FIXED

Priority P2

Severity Enhancement

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS OS X 10.5

Product WebKit

Component Layout and Rendering

Assignee

QtWebKit Unassigned

Reported

2010-01-04 07:11 PST

Modified

2010-06-28 01:58 PDT History

CC List

22 users Show

URL

Keywords Performance, Qt, QtTriaged

Duplicates (1)

38418 View as bug list

Depends on

Blocks

34208 35784 36652 37148

Dependencies

tree graph

benjamin
commit-queue
darin
dbates
dglazkov
eric
fishd
hamaji
hausmann
hyatt
jamesr
jturcotte
kenneth
kling
koivisto
laszlo.gombos
manyoso
mitz
noam
simon.fraser
skyul
webkit.review.bot