[webkit-reviews] review granted: [Bug 5909] overlapping element leaves trail when scrolling iframe (affects Google Docs) : [Attachment 29762] Test for overlapping layers during painting and enable/disable blitting accordingly

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted mitz at webkit.org's request for review:
Bug 5909: overlapping element leaves trail when scrolling iframe (affects
Google Docs)
https://bugs.webkit.org/show_bug.cgi?id=5909

Attachment 29762: Test for overlapping layers during painting and
enable/disable blitting accordingly
https://bugs.webkit.org/attachment.cgi?id=29762&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
This is great!	It looks very low risk to me as well, so I think it will be
safe to take this patch now.  I have some minor comments, but you can just fix
them and land with the changes:

>  
> +    if (overlapTestRequests) {
> +	    Vector<OverlapTestRequester*> overlappedRequesters;

Fix indentation ^^^^^^

> +	   RenderObject::OverlapTestRequestMap::iterator end =
overlapTestRequests->end();
> +	   for (RenderObject::OverlapTestRequestMap::iterator it =
overlapTestRequests->begin(); it != end; ++it) {
> +	       if (!layerBounds.intersects(it->second))
> +		   continue;
> +
> +	       it->first->setOverlapTestResult(true);
> +	       overlappedRequesters.append(it->first);
> +	   }
> +	   for (size_t i = 0; i < overlappedRequesters.size(); ++i)
> +	       overlapTestRequests->remove(overlappedRequesters[i]);
> +    }
> +

The above code should be moved into a separate helper function.


> Index: WebCore/rendering/RenderWidget.cpp
> ===================================================================
> --- WebCore/rendering/RenderWidget.cpp	(revision 42834)
> +++ WebCore/rendering/RenderWidget.cpp	(working copy)
> @@ -211,6 +211,11 @@ void RenderWidget::paint(PaintInfo& pain
>	   // Tell the widget to paint now.  This is the only time the widget
is allowed
>	   // to paint itself.	That way it will composite properly with
z-indexed layers.
>	   m_widget->paint(paintInfo.context, paintInfo.rect);
> +
> +	   if (m_widget->isFrameView() && paintInfo.overlapTestRequests) {
> +	       ASSERT(!paintInfo.overlapTestRequests->contains(this));
> +	       paintInfo.overlapTestRequests->set(this, m_widget->frameRect());

> +	   }
>

Slow repaints are frequently already turned on for iframes (since they commonly
have pages that don't specify a background in them).  You should avoid issuing
an overlapTestRequest in this case if slow repaints (excluding overlap) are
enabled already.

r=me!