Bug 52735 - [Qt] painting of windowed plugins faulty on certain scroll events
Summary: [Qt] painting of windowed plugins faulty on certain scroll events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-01-19 13:00 PST by Robert Hogan
Modified: 2011-02-22 21:31 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2011-02-06 07:45 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-01-19 13:00:03 PST
I noticed this while fixing the specific issue in bug 52734. The patch at bug 52734 doesn't fix it.

If you open plugins/iframe-shims.html in QtTestBrowser and resize to narrow rectangle, then shorten the window so that you can scroll up and down about 100 pixels in one step, you'll notice that after scrolling up and down once or twice (single steps, gradual scrolling won't reproduce) that the blue plugins disappear from the first two boxes in the top row. Instead they get rendered over the plugins on the two rows beneath the top row. 

I can't take a screenshot of this because ksnapshot repaints the entire screen first, which results in the plugins getting repainted properly.

This alone suggests we need to trigger an extra paint event on the frame at some point, but I'm not sure when.
Comment 1 Robert Hogan 2011-01-31 12:35:34 PST
This is happening in:


void RenderWidget::paint(PaintInfo& paintInfo, int tx, int ty)
{
    if (!shouldPaint(paintInfo, tx, ty))
        return;

    tx += x();
    ty += y();

    if (hasBoxDecorations() && (paintInfo.phase == PaintPhaseForeground || paintInfo.phase == PaintPhaseSelection))
        paintBoxDecorations(paintInfo, tx, ty);

    if (paintInfo.phase == PaintPhaseMask) {
        paintMask(paintInfo, tx, ty);
        return;
    }

    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
        return;


Commenting out both:


    if (!shouldPaint(paintInfo, tx, ty))
        return;

and


    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
        return;

fixes it. Commenting out one or the other does not. It's very hard to debug paint events so I don't really stand a chance without a few pointers, but this 'fixes' it:

--- a/Source/WebCore/rendering/RenderReplaced.cpp
+++ b/Source/WebCore/rendering/RenderReplaced.cpp
@@ -157,18 +157,17 @@ void RenderReplaced::paint(PaintInfo& paintInfo, int tx, int ty)
 
 bool RenderReplaced::shouldPaint(PaintInfo& paintInfo, int& tx, int& ty)
 {
    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseOutline && paintInfo.phase != PaintPhaseSelfOutline
             && paintInfo.phase != PaintPhaseSelection && paintInfo.phase != PaintPhaseMask)
         return false;

     if (!paintInfo.shouldPaintWithinRoot(this))
         return false;
        
     // if we're invisible or haven't received a layout yet, then just bail.
     if (style()->visibility() != VISIBLE)
         return false;
 
-    int currentTX = tx + x();
     int currentTY = ty + y();
 
     // Early exit if the element touches the edges.
@@ -180,10 +179,9 @@ bool RenderReplaced::shouldPaint(PaintInfo& paintInfo, int& tx, int& ty)
         top = min(selTop, top);
         bottom = max(selBottom, bottom);
     }

     int os = 2 * maximalOutlineSize(paintInfo.phase);
-    if (currentTX + leftVisualOverflow() >= paintInfo.rect.right() + os || currentTX + rightVisualOverflow() <= paintInfo.rect.x() - os)
-        return false;
     if (top >= paintInfo.rect.bottom() + os || bottom <= paintInfo.rect.y() - os)
         return false;
 
diff --git a/Source/WebCore/rendering/RenderWidget.cpp b/Source/WebCore/rendering/RenderWidget.cpp
index 13b572d..8f07d40 100644
--- a/Source/WebCore/rendering/RenderWidget.cpp
+++ b/Source/WebCore/rendering/RenderWidget.cpp
@@ -260,7 +260,7 @@ void RenderWidget::paint(PaintInfo& paintInfo, int tx, int ty)
         return;
     }
 
-    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
+    if (!m_frameView /*|| paintInfo.phase != PaintPhaseForeground */|| style()->visibility() != VISIBLE)
         return;


I'm not even sure what:


-    if (currentTX + leftVisualOverflow() >= paintInfo.rect.right() + os || currentTX + rightVisualOverflow() <= paintInfo.rect.x() - os)
-        return false;

is doing.
Comment 2 Dave Hyatt 2011-01-31 12:51:22 PST
Just a guess but make sure the dirty rect you pass in is correct.  There's nothing wrong with the cross-platform code there.
Comment 3 Robert Hogan 2011-01-31 13:37:03 PST
(In reply to comment #2)
> Just a guess but make sure the dirty rect you pass in is correct.  There's nothing wrong with the cross-platform code there.

Right enough, another 'fix' is:


--- a/Source/WebKit/qt/Api/qwebframe.cpp
+++ b/Source/WebKit/qt/Api/qwebframe.cpp
@@ -374,6 +374,7 @@ void QWebFramePrivate::renderRelativeCoords(GraphicsContext* context, QWebFrame:
             QRect rect = intersectedRect;
             context->translate(x, y);
             rect.translate(-x, -y);
+            view->paintContents(context, rect);
             context->translate(-scrollX, -scrollY);
             rect.translate(scrollX, scrollY);
             context->clip(view->visibleContentRect());

It looks like RenderWidget::paint() needs to test the rect using its absolute co-ordinates in the frame but QWebFrame only supplies relative co-ordinates. I imagine this is pretty unique to plugins. And Qt is the only port calling frameview->paintContents() directly when rendering the page.

So I think Qt needs to test the page for plugins/widgets and make sure they always get painted.
Comment 4 Robert Hogan 2011-02-05 14:48:46 PST
There is something I don't understand about RenderWidget::paint(). If a Scroll event has moved the ScrollView down by exactly or just slightly more than the height of the RenderWidget shouldPaint() in RenderWidget::paint() will never pass.

In the case of this bug the result is that the windowed plugin does not move when a single scroll event moves the viewport down by the same amount as (or slightly more than) the height of the plugin. 

So doesn't ScrollView need to take account of this somehow?
Comment 5 Robert Hogan 2011-02-06 07:45:34 PST
Created attachment 81403 [details]
Patch
Comment 6 Robert Hogan 2011-02-12 05:03:41 PST
(In reply to comment #5)
> Created an attachment (id=81403) [details]
> Patch

I've look at PluginViewSymbian and PluginViewWin - neither have the code I'm altering here. I don't have the set-up for testing either of them unfortunately.
Comment 7 Andreas Kling 2011-02-22 02:40:16 PST
Comment on attachment 81403 [details]
Patch

r=me, thanks for digging into this!
Comment 8 Alexis Menard (darktears) 2011-02-22 06:17:19 PST
Comment on attachment 81403 [details]
Patch

Sounds good to me. I agree with you conclusion investigating myself. It's really complex to reproduce, not sure we can do it through a tests. I ran the LayoutTests on my machine and didn't catch any regressions.
Comment 9 WebKit Commit Bot 2011-02-22 21:31:10 PST
Comment on attachment 81403 [details]
Patch

Clearing flags on attachment: 81403

Committed r79397: <http://trac.webkit.org/changeset/79397>
Comment 10 WebKit Commit Bot 2011-02-22 21:31:15 PST
All reviewed patches have been landed.  Closing bug.