Bug 33150 - Do not render the full frame when there is some elements with fixed positioning
Summary: Do not render the full frame when there is some elements with fixed positioning
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: QtWebKit Unassigned
URL:
Keywords: Performance, Qt, QtTriaged
: 38418 (view as bug list)
Depends on:
Blocks: 34208 35784 36652 37148
  Show dependency treegraph
 
Reported: 2010-01-04 07:11 PST by Benjamin Poulain
Modified: 2010-06-28 01:58 PDT (History)
22 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 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.
Comment 1 Benjamin Poulain 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.
Comment 2 Antti Koivisto 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.
Comment 3 Benjamin Poulain 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).
Comment 4 WebKit Review Bot 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
Comment 5 Dave Hyatt 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.
Comment 6 Benjamin Poulain 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.
Comment 7 WebKit Review Bot 2010-01-06 14:25:26 PST
style-queue ran check-webkit-style on attachment 45994 [details] without any errors.
Comment 8 Benjamin Poulain 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.
Comment 9 WebKit Review Bot 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
Comment 10 Benjamin Poulain 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.
Comment 11 WebKit Review Bot 2010-01-06 14:41:41 PST
style-queue ran check-webkit-style on attachment 45996 [details] without any errors.
Comment 12 Kenneth Rohde Christiansen 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
Comment 13 Benjamin Poulain 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.
Comment 14 WebKit Review Bot 2010-01-07 02:52:45 PST
style-queue ran check-webkit-style on attachment 46040 [details] without any errors.
Comment 15 Kenneth Rohde Christiansen 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.
Comment 16 Kenneth Rohde Christiansen 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)
Comment 17 Antti Koivisto 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.
Comment 18 Kenneth Rohde Christiansen 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.
Comment 19 Benjamin Poulain 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.
Comment 20 WebKit Review Bot 2010-01-07 05:17:53 PST
style-queue ran check-webkit-style on attachment 46047 [details] without any errors.
Comment 21 Adam Treat 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
Comment 22 Benjamin Poulain 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.
Comment 23 Benjamin Poulain 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
Comment 24 Andreas Kling 2010-01-20 03:24:37 PST
Works beautifully, Benjamin! Noticeable improvement on Facebook.
Comment 25 Antti Koivisto 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
Comment 26 Adam Treat 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.
Comment 27 Benjamin Poulain 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 :)
Comment 28 Antti Koivisto 2010-01-20 07:42:03 PST
r=me
Comment 29 WebKit Commit Bot 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
Comment 30 Benjamin Poulain 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.
Comment 31 Benjamin Poulain 2010-01-21 06:05:38 PST
Created attachment 47114 [details]
Repaint only the invalidated area after scrolling

The previous has \t for indent
Comment 32 WebKit Commit Bot 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
Comment 33 Benjamin Poulain 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.
Comment 34 Eric Seidel (no email) 2010-01-21 15:37:19 PST
It's possible acid3.html was just flakey.  I will attach the crash log.
Comment 35 Eric Seidel (no email) 2010-01-21 15:38:01 PST
Created attachment 47153 [details]
crash log from commit bot
Comment 36 Benjamin Poulain 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).
Comment 37 WebKit Commit Bot 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>
Comment 38 WebKit Commit Bot 2010-01-22 04:01:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 Darin Adler 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.
Comment 40 Darin Adler 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!
Comment 41 Adam Treat 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...
Comment 42 mitz 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.
Comment 43 Benjamin Poulain 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?
Comment 44 Darin Adler 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.
Comment 45 Benjamin Poulain 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;
}
Comment 46 Simon Hausmann 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.
Comment 47 Simon Hausmann 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 :)
Comment 48 Benjamin Poulain 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 :)
Comment 49 Simon Hausmann 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.
Comment 50 Benjamin Poulain 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.
Comment 51 Benjamin Poulain 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.
Comment 52 WebKit Review Bot 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.
Comment 53 Benjamin Poulain 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 ;)
Comment 54 WebKit Commit Bot 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>
Comment 55 WebKit Commit Bot 2010-01-25 04:37:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 56 Eric Seidel (no email) 2010-01-25 21:29:47 PST
bug 34153 suggests that this caused a regression.
Comment 57 Darin Adler 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.
Comment 58 Benjamin Poulain 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.
Comment 59 Benjamin Poulain 2010-01-27 00:06:20 PST
Patch reverted by 34153.

RenderWidget::destroy() must have a copy of what is added to RenderObject::destroy().
Comment 60 Benjamin Poulain 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.
Comment 61 Kenneth Rohde Christiansen 2010-01-28 09:00:45 PST
Hyatt, does this new patch look good to you? or do you have any further comments?
Comment 62 Dave Hyatt 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.
Comment 63 Benjamin Poulain 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()? ;)
Comment 64 Dave Hyatt 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.
Comment 65 Benjamin Poulain 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.
Comment 66 Benjamin Poulain 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.
Comment 67 Kenneth Rohde Christiansen 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);
> +        }
> +    }
> +}
Comment 68 Benjamin Poulain 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.
Comment 69 Benjamin Poulain 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.
Comment 70 Benjamin Poulain 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?
Comment 71 Eric Seidel (no email) 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.
Comment 72 Eric Seidel (no email) 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.
Comment 73 Adam Barth 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.
Comment 74 Adam Treat 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.
Comment 75 Benjamin Poulain 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.
Comment 76 Benjamin Poulain 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 :)
Comment 77 Adam Treat 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.
Comment 78 Benjamin Poulain 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.
Comment 79 WebKit Review Bot 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.
Comment 80 Benjamin Poulain 2010-03-10 03:01:02 PST
Created attachment 50387 [details]
Repaint only the invalidated area after scrolling

Code style fix.
Comment 81 Adam Treat 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.
Comment 82 Adam Treat 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+.
Comment 83 WebKit Commit Bot 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>
Comment 84 WebKit Commit Bot 2010-03-11 23:07:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 85 Simon Hausmann 2010-03-22 04:45:23 PDT
Cherry-picked into qtwebkit-4.6 with commit cd258386aa3564cd4c961933401d0f86a4c872f8
Comment 86 Simon Hausmann 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
Comment 87 Shinichiro Hamaji 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,
Comment 88 Benjamin Poulain 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.
Comment 89 Shinichiro Hamaji 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?
Comment 90 Shinichiro Hamaji 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.
Comment 91 Benjamin Poulain 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.
Comment 92 Shinichiro Hamaji 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!
Comment 93 James Robinson 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.
Comment 94 James Robinson 2010-04-06 13:35:22 PDT
Created attachment 52663 [details]
Patch
Comment 95 James Robinson 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>
Comment 96 James Robinson 2010-04-06 13:45:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 97 James Robinson 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).
Comment 98 Eric Seidel (no email) 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.
Comment 99 Benjamin Poulain 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.
Comment 100 Benjamin Poulain 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.
Comment 101 Simon Hausmann 2010-05-20 05:00:53 PDT
James: ping
Comment 102 James Robinson 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).
Comment 103 Darin Fisher (:fishd, Google) 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.
Comment 104 Benjamin Poulain 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.
Comment 105 Benjamin Poulain 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?
Comment 106 Darin Fisher (:fishd, Google) 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.
Comment 107 Benjamin Poulain 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?
Comment 108 Darin Fisher (:fishd, Google) 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.
Comment 109 Darin Fisher (:fishd, Google) 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!
Comment 110 Darin Fisher (:fishd, Google) 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.
Comment 111 Noam Rosenthal 2010-06-11 13:05:50 PDT
*** Bug 38418 has been marked as a duplicate of this bug. ***
Comment 112 Noam Rosenthal 2010-06-11 15:33:18 PDT
It could have been 39918 then. The point is the bug is no longer there.
Comment 113 Kenneth Rohde Christiansen 2010-06-13 07:57:39 PDT
Hyatt, Simon Fraser, any one of you having time to review this patch now?
Comment 114 Simon Hausmann 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?
Comment 115 Darin Fisher (:fishd, Google) 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.
Comment 116 Dave Hyatt 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.
Comment 117 Benjamin Poulain 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!
Comment 118 Kenneth Rohde Christiansen 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.
Comment 119 Kenneth Rohde Christiansen 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.
Comment 120 Benjamin Poulain 2010-06-23 07:12:27 PDT
Committed r61686: <http://trac.webkit.org/changeset/61686>
Comment 121 Simon Hausmann 2010-06-28 01:28:30 PDT
Revision r61686 cherry-picked into qtwebkit-2.0 with commit bf2d8aa3dc238a9f893f8d7b35fa06c083cbb9b5
Comment 122 Simon Hausmann 2010-06-28 01:57:54 PDT
Revision r61686 cherry-picked into qtwebkit-2.0 with commit bf2d8aa3dc238a9f893f8d7b35fa06c083cbb9b5