Bug 49184 – [Qt] Flawed rendering design for QtWebKit resulting in artifacts being displayed

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Carol Szabo]

It is a policy in WebCore that layout is being postponed whenever possible, which leads to cases when the page has an incorrect/incomplete layout at the beginning of processing of a paint event. This is the case for example when JavaScript changes the style of a node such that its position or size changes as a result.
Of course, during paint event processing the layout is completed, before rendering begins, thus what is painted is consistent with the changed layout, except that Qt restricts painting to the area that was marked as needing update before the paint event processing started, thus even though as a result of the layout change a larger/different area needs repaint, the user sees on the screen changes related only to the areas that were known as dirty and these changes are inconsistent with the previous image of the document and artifacts result. Later on, a second paint event may fix the issue, but in the mean time on slow systems, such as cell phones, this causes flicker. In some particular cases such as when paint events are dropped, or when the document image is transformed (i.e. scrolled) between the first and second painting, persistent artifacts may result.
The painter used for painting is not clipped (at list if QGraphicsWebView is used), but painting happens on an intermediary canvas and only the initially dirty region is blitted ultimately to the screen.
Given these constraints, the solutions I see to the problem are the following (all with performance impacts or interface changes):
1. Allow the paint method of QGraphicsItem to adjust the exposedRect to cover the newly dirtied areas and blit this entire new area to the screen:
Advantage: Probably best performance.
Disadvantage: Requires Qt API change.
2. Every time when there is a pending Layout dirty the entire WebKitView:
Advantage: Minimized performance impact, no Qt Api change
Disadvantage: It may be hard to track all cases of pending layout.
3. Indiscriminately dirty the entire WebKitView every time when there is a dirty portion of it.
Advantage: It is easy to implement and reliably solves the artifacts problem (potentially 33 webkit bugs logged on the subject).
Disadvantage: It may significantly reduce performance.
4. Do not postpone layout beyond yielding the WebCore thread.
Advantage: Probably no heavier performance impact then solution 2, maybe even lighter.
Disadvantage: Hard to implement.
5. Adding an extra View managed buffer, such that the entire QtGraphicsItem will be dirtied every time, but only necessary portions will be rendered onto the view managed buffer.
Advantage: Easy to implement, safe.
Disadvantage: Performance impact, memory usage.

Reviewers, please comment on these proposals and help me pick one.

[Benjamin Poulain]

I don't understand the problem you describe, and all the solutions you suggest seems either strange or have a big performance impact.

Could you please add a test case so one can figures what is the "flawed rendering design"?
Why is this problem QtWebKit specific?
You mention QGraphicsItem, is this problem related to accelerated compositing or is it a general issue?

[Carol Szabo]

Created attachment 73389 [details]
Test case

(In reply to comment #1)
> I don't understand the problem you describe, and all the solutions you suggest seems either strange or have a big performance impact.
> 
> Could you please add a test case so one can figures what is the "flawed rendering design"?
> Why is this problem QtWebKit specific?
> You mention QGraphicsItem, is this problem related to accelerated compositing or is it a general issue?
The problem is that in certain cases the image of a QGraphicsWebView or a QWebView at the end of a paint event does not reflect any valid state of the web page (it has artifacts).
The artifacts show up rather randomly, so the problem is pretty hard to reproduce especially on the desktop. On a slow device such as a cell phone, the issue is way more frequent.
The way I was able to reproduce the issue with a reasonable frequency and debugability was to put a 2 second delay in QGraphicsWebView::paint, and show the attached webpage. After a couple of minutes of watching the page, artifacts start to show up.
What happens in this particular example is that when the style of the "downstate" element changes both its background color and size. This leads to the old area of the element being invalidated and a layout being scheduled. The invalidated area leads to a paint event being scheduled for that area.
Sometimes this paint event handling starts before the layout event handling. During the handling of the paint event, the layout is done, but due to the nature of the QGraphicsItem::paint interface that is used by QGraphicsWebView for painting, only the initial area specified before the paint event handling started is shown on the screen.
So when the div grows the new background is applied only to the old area and the div appears to have a border of the color of the old background area, until a new paint event repaints the entire area occupied now by the div.
Ordinarily these 2 paint events come in quick succession and the flicker is not perceived, but on slow systems the problem is significant. Also during animations (such as scrolling) and other cases when some paint events are dropped (due to insufficient time to complete them or queue overflow) the artifacts produced by incomplete paints are long lived.
I have described the behavior mainly with QGraphicsWebView, because this is the scenario I studied more closely, but QWebView exhibits the same behavior, but depending on the interfaces a different better solution may exist for QWebView.
The issue may be present on non-Qt platforms as well, but it is platform specific as far as I can tell since the generic WebView::render, does what it is supposed to do, paints what it is told. The problem is that it is not told to paint enough and, in Qt, even if it were told to paint more, the underlying Qt engine will not forward the extra painting to the screen.

[Carol Szabo]

(In reply to comment #1)
> Could you please add a test case so one can figures what is the "flawed rendering design"?

The flaw in the rendering design is that while areas to be rendered in an atomic operation (such as when style changes are applied to an HTML element), may not be known until after the paint event handling starts (due to delayed layout) yet, the area to be rendered during that operation is frozen before the paint event handling starts. Therefore changes in the image of a document that are supposed to be atomic do not appear as such and cannot appear as such if all elements of the design (such as delaying the layout and deciding the area to render upfront and keeping the area to render to the minimal bounding rect) are maintained.

> Why is this problem QtWebKit specific?
The problem may or may not be QtWebKit specific, depending on what design decisions other platforms made. The key decisions here are to repaint only the minimal bounding rect and to not allow altering the painted area during the paint event. Both of these decisions are in Qt specific code (the first in QtWebKit specific code the second is in Qt itself).
> You mention QGraphicsItem, is this problem related to accelerated compositing or is it a general issue?
The issue is not restricted to accelerated compositing. It is a generic layout policy/painting policy issue, but I have researched it in this context.

[Benjamin Poulain]

I was curious about why there is no such artifact on Cocoa so I look at it at home :)

What happens is that NSView send the message viewWillDraw() before calling drawRect(). The layout is done in response to viewWillDraw(), any update region changed there will appear in the incoming call to drawRect().

I can't think of any clean way to solve that in Qt without causing useless extra layout. You should talk with someone from the graphics team, they might have an idea to change the original clip region.

[Carol Szabo]

(In reply to comment #4)
> I was curious about why there is no such artifact on Cocoa so I look at it at home :)
> 
> What happens is that NSView send the message viewWillDraw() before calling drawRect(). The layout is done in response to viewWillDraw(), any update region changed there will appear in the incoming call to drawRect().
> 
> I can't think of any clean way to solve that in Qt without causing useless extra layout. You should talk with someone from the graphics team, they might have an idea to change the original clip region.

Options 2, 3 and 5 in the list that I proposed avoid any extra layouts, but they cause some extra painting whenever there is a pending Layout.

[Benjamin Poulain]

(In reply to comment #5)
> Options 2, 3 and 5 in the list that I proposed avoid any extra layouts, but they cause some extra painting whenever there is a pending Layout.

:D
The cost of rendering extra pixels is so important that I did not even consider those. I would rather have the current small artefact than slow down every web app.

[Carol Szabo]

Created attachment 74413 [details]
Tentative patch

After consulting with Benjamin and Laszlo, I decided to propose this patch that uses repaint to enlarge the cliping area if required by the layout.
In QWebFramePrivate the bounding rect of the dirty area is captured before and after the layout request. If the "after" bounding rect is not the same as the before one, repaint is called on the view (and children if existing and needed) and the current paint is aborted as unnecessary.

[Carol Szabo]

Created attachment 75046 [details]
Tentative patch bringing up-to-date

[Carol Szabo]

Created attachment 75073 [details]
Fixed conflicts with trunk

[WebKit Review Bot]

Attachment 75073 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/qt/QWebPageClient.h', u'WebKit/qt/Api/qgraphicswebview.cpp', u'WebKit/qt/Api/qwebframe.cpp', u'WebKit/qt/Api/qwebframe.h', u'WebKit/qt/Api/qwebframe_p.h', u'WebKit/qt/Api/qwebpage.h', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/PageClientQt.cpp']" exit_code: 1
WebKit/qt/Api/qwebframe.cpp:91:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/Api/qwebframe.cpp:343:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/qt/Api/qwebframe.cpp:344:  Declaration has space between type name and * in QWebPageClient *client  [whitespace/declaration] [3]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carol Szabo]

Created attachment 75151 [details]
Proposed Patch. Fixed style issues

[WebKit Review Bot]

Attachment 75151 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/qt/QWebPageClient.h', u'WebKit/qt/Api/qgraphicswebview.cpp', u'WebKit/qt/Api/qwebframe.cpp', u'WebKit/qt/Api/qwebframe.h', u'WebKit/qt/Api/qwebframe_p.h', u'WebKit/qt/Api/qwebpage.h', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/PageClientQt.cpp']" exit_code: 1
WebKit/qt/Api/qwebframe.cpp:83:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carol Szabo]

Created attachment 75154 [details]
Fixed some more style issues

[Benjamin Poulain]

Comment on attachment 75154 [details]
Fixed some more style issues

View in context: https://bugs.webkit.org/attachment.cgi?id=75154&action=review

> WebCore/ChangeLog:12
> +        No behavior changes, hence no new tests.
> +        This patch fixes flickers and artifacts that are hard to reproduce
> +        on desktop computers.
> +        The goal of this patch is that every paint call on the WebView
> +        produces a correct image of the page with no artifacts.

Changelog does not clearly explain the problem, just the consequences. The following sentence comes out of nowhere without more explanation.

> WebCore/platform/qt/QWebPageClient.h:98
> +    virtual QRect getDirtyRect() const { return dirtyRect;}

Missing space after the semicolon.

> WebCore/platform/qt/QWebPageClient.h:100
> +    virtual void clearDirtyRect() { dirtyRect.setWidth(0); }

This should be dirtyRect = QRect();

> WebCore/platform/qt/QWebPageClient.h:107
> +    QRect dirtyRect;

Why this variable misses the prefix? "m_".

Personally, I would also prefer the variable to be private, and have a protected method addToDirtyRect(const QRect&). That one could be inline, but at least we would have some encapsulation.

> WebKit/qt/Api/qgraphicswebview.cpp:309
>  #if USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER)
> -    page()->mainFrame()->render(painter, d->overlay() ? QWebFrame::ContentsLayer : QWebFrame::AllLayers, option->exposedRect.toAlignedRect());
> +    GraphicsContext context(painter);
> +    page()->mainFrame()->d->renderRelativeCoords(&context, d->overlay() ? QWebFrame::ContentsLayer : QWebFrame::AllLayers,
> +                                                 QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
>  #else
> -    page()->mainFrame()->render(painter, QWebFrame::AllLayers, option->exposedRect.toRect());
> +    page()->mainFrame()->d->renderRelativeCoords(&context, QWebFrame::AllLayers,
> +                                                 QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
>  #endif

I think this would crash if the painter is null, while it would not with the call to QWebFrame::render().

> WebKit/qt/Api/qwebframe.cpp:364
> +            return;

Why would you stop rendering if there is no page client?

> WebKit/qt/Api/qwebframe.h:228
> +    friend class QWebView;

I don't get it, why do you add QWebView as a friend of the class here?



I stop here, the patch seems to have been a bit rushed. Please polish it so one doesn't have to stop at each file :)

[Carol Szabo]

(In reply to comment #14)
> (From update of attachment 75154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75154&action=review
> 
> > WebCore/ChangeLog:12
> > +        No behavior changes, hence no new tests.
> > +        This patch fixes flickers and artifacts that are hard to reproduce
> > +        on desktop computers.
> > +        The goal of this patch is that every paint call on the WebView
> > +        produces a correct image of the page with no artifacts.
> 
> Changelog does not clearly explain the problem, just the consequences. The following sentence comes out of nowhere without more explanation.

I thought that the changelog should explain what the change does and for what purpose, and that the problem is explained in detail in the bug description. If you look at other ChangeLog entries around mine, you will find that mine is way more explicit and detailed.
The "No behavior changes, no new tests", is a standard phrase in ChangeLog explaining the reason why LayoutTests do not accompany this change.

> 
> > WebCore/platform/qt/QWebPageClient.h:98
> > +    virtual QRect getDirtyRect() const { return dirtyRect;}
> 
> Missing space after the semicolon.

This is not required by the WebKit style guidelines as published at https://webkit.org/coding/coding-style.html
but I shall actually make it comply with the guidelines, that is break it into separate lines.

> 
> > WebCore/platform/qt/QWebPageClient.h:100
> > +    virtual void clearDirtyRect() { dirtyRect.setWidth(0); }
> 
> This should be dirtyRect = QRect();

I find setWidth(0) way faster, but you're the boss.

> 
> > WebCore/platform/qt/QWebPageClient.h:107
> > +    QRect dirtyRect;
> 
> Why this variable misses the prefix? "m_".

I know that the style would require it and shall fix. I missed the m_lastCursor member that conforms to the style, I looked at the derived classes which do not so I tried to blend in.

> 
> Personally, I would also prefer the variable to be private, and have a protected method addToDirtyRect(const QRect&). That one could be inline, but at least we would have some encapsulation.

QWebPageClient is more of an interface class, providing very little in terms of implementation (it does not even have a cpp file. The encapsulation is done at the WebKit level in the two derived classes: PageClientQt and PageClientQGraphicsWidget. The reason why I made the data member protected, was to provide maximum flexibility to the implementors of the interface. Conceptually QWebPageClient does not provide any restrictions on how the data member may be manipulated, thus I see no advantage in making it private, but shall comply with your preference.

> 
> > WebKit/qt/Api/qgraphicswebview.cpp:309
> >  #if USE(ACCELERATED_COMPOSITING) && !USE(TEXTURE_MAPPER)
> > -    page()->mainFrame()->render(painter, d->overlay() ? QWebFrame::ContentsLayer : QWebFrame::AllLayers, option->exposedRect.toAlignedRect());
> > +    GraphicsContext context(painter);
> > +    page()->mainFrame()->d->renderRelativeCoords(&context, d->overlay() ? QWebFrame::ContentsLayer : QWebFrame::AllLayers,
> > +                                                 QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
> >  #else
> > -    page()->mainFrame()->render(painter, QWebFrame::AllLayers, option->exposedRect.toRect());
> > +    page()->mainFrame()->d->renderRelativeCoords(&context, QWebFrame::AllLayers,
> > +                                                 QRegion(option->exposedRect.toRect()), QWebFramePrivate::UseDirtyRect);
> >  #endif
> 
> I think this would crash if the painter is null, while it would not with the call to QWebFrame::render().

I beg to disagree with the above statement as QWebFrame::render does not make any checks on painter either before calling renderRelativeCoords, but shall check painter in the interest of robustness.

> 
> > WebKit/qt/Api/qwebframe.cpp:364
> > +            return;
> 
> Why would you stop rendering if there is no page client?

My understanding is that a PageClient is required for a page to be displayed (for example without a page client a page cannot report dirty areas or learn the windowRect).

> 
> > WebKit/qt/Api/qwebframe.h:228
> > +    friend class QWebView;
> 
> I don't get it, why do you add QWebView as a friend of the class here?
> 

I addded QWebView as a friend so that the QWebView::paint method can access the private rendering method on QWebFramePrivate that can expand the painting rect. The reason why I did this is so that the QWebFrame::render methods continue to do what they are supposed to, that is to render the area requested, never more or less.

> 
> 
> I stop here, the patch seems to have been a bit rushed. Please polish it so one doesn't have to stop at each file :)

[Carol Szabo]

Created attachment 75286 [details]
Refined patch

Made changes to the original submission to comply with Benjamins observations. I have only checked the painter in debug mode since the check was not there before and makes no sense to call a paint function without a painter anyway.

[Carol Szabo]

Created attachment 75290 [details]
Proposed Patch. Added lost changes to QWebView

[Benjamin Poulain]

>  QWidget* ownerWidget = client->ownerWidget();
This is incorrect. You could have 2 views on a scene, you need to get the one currently being rendered, not the first view.

Given the previous problems, it might be nice to add a few autotests.

> WebKit/qt/Api/qgraphicswebview.cpp:294
> +    ASSERT(painter);
> +    ASSERT(option);
> +    GraphicsContext context(painter);

This is on Qt side, I think Q_ASSERT() is prefered.

Anyway, this changes the behavior. You get a crash at runtime where the method use to do nothing. Please look at why the check as added to render in the first place.

>     frame->render(&p, ev->region());
>     GraphicsContext context(&p);
>     d->page->mainFrame()->d->renderRelativeCoords(&context, QWebFrame::AllLayers,
>         ev->region(), QWebFramePrivate::UseDirtyRect);

This won't do it. You need the same feature on QWebView and any QWidget implementation written on top of QWebPage/QWebFrame. Writting your own Widget as the view is unfortunatelly supported.

> WebKit/qt/Api/qwebframe.cpp:351
> +                QList<QWidget*> children = ownerWidget->findChildren<QWidget*>();

This will get the full hierarchy of children, while you only need the top level children.
I would personally just do a full update when the view has any QWidget child since it is a corner case.

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:184
>  void PageClientQWidget::update(const QRect & dirtyRect)
>  {
> +    addToDirtyRect(dirtyRect);
>      view->update(dirtyRect);
>  }

I might misunderstand but this looks wrong.

Let's say:
1) The QWebFramePrivate::renderRelativeCoords() call the layout
2) We get an update during a layout, you add the rect to dirty rect AND you trigger an update on the view.
3) The QWebFramePrivate::renderRelativeCoords() call repaint() since the dirty region changed
4) Next time we get in the event loop, we get a duplicate Update event since we called update() here.

[Carol Szabo]

(In reply to comment #18)
> > WebKit/qt/Api/qgraphicswebview.cpp:294
> > +    ASSERT(painter);
> > +    ASSERT(option);
> > +    GraphicsContext context(painter);
> 
> This is on Qt side, I think Q_ASSERT() is prefered.
If you only think this, then I would leave the ASSERT alone since it is consistent with the other assert in the file.

> 
> Anyway, this changes the behavior. You get a crash at runtime where the method use to do nothing. Please look at why the check as added to render in the first place.


> 
> >     frame->render(&p, ev->region());
> >     GraphicsContext context(&p);
> >     d->page->mainFrame()->d->renderRelativeCoords(&context, QWebFrame::AllLayers,
> >         ev->region(), QWebFramePrivate::UseDirtyRect);
> 
> This won't do it. You need the same feature on QWebView and any QWidget implementation written on top of QWebPage/QWebFrame. Writting your own Widget as the view is unfortunatelly supported.

Then, would you recommend changing the interface of QWebFrame::render to support the extra flag needed to do a full update?

> I might misunderstand but this looks wrong.
> 
> Let's say:
> 1) The QWebFramePrivate::renderRelativeCoords() call the layout
> 2) We get an update during a layout, you add the rect to dirty rect AND you trigger an update on the view.
> 3) The QWebFramePrivate::renderRelativeCoords() call repaint() since the dirty region changed
> 4) Next time we get in the event loop, we get a duplicate Update event since we called update() here.

All these update() calls are done before the repaint, and are necessary to enlarge the clipping region.
When the repaint is called all these dirty areas are cleared.
I shall doublecheck.

[Carol Szabo]

Created attachment 75410 [details]
Proposed Patch

This new patch answers Benjamin's concerns:
- Introduces a public method on QWebFrame to expose the repaint logic instead of relying on QWebView and QGraphicsView accessing methods in QWebFramePrivate.
- Introduces a new flag in QWebPageClientQt that controls whether updates are forwarded to QWidget or not, to prevent repaints.

[Simon Hausmann]

Comment on attachment 75410 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75410&action=review

None of the functions in QWebPageClient need to be virtual.

In general I think the approach is correct, but I see one major problem:

The updates that come in during the layout should not be sent to a QWidget (ownerWidget). First of all,
the coordinates will be wrong (line 352). Think of a QGraphicsView setup where that widget will be the
viewport and the coordinates need to be adjusted to the view's transformation and scrollbars.

Secondly we should call QWidget::update() instead of repaint().

Lastly I don't think QWidget should be used at all and instead in QWebFramePrivate::renderRelativeCoords you should call
void ChromeClientQt::invalidateContentsAndWindow(const IntRect& windowRect, bool immediate).

That will take care of coordinate issues (QGraphicsWidget::update() will be called with item-local coordinates
and they will be translate to the correct viewport ones). It will also support use-cases where people rely
on the correct emission of the QWebPage::repaintRequested() signal. And lastly it will allow you to do the change
without introducing a new function in the public QWebFrame API.

> WebCore/platform/qt/QWebPageClient.h:98
> +    virtual QRegion getDirtyRegion() const

Just call it dirtyRegion(), we usually don't use the get suffix.

> WebCore/platform/qt/QWebPageClient.h:113
> +    inline bool skipWidgetUpdates()

This function should be const.

> WebCore/platform/qt/QWebPageClient.h:119
> +    inline void addToDirtyRegion(const QRect &rect)

Coding style, the & placement is incorrect.

[Simon Hausmann]

(In reply to comment #21)

> Secondly we should call QWidget::update() instead of repaint().

Darn, of course that invalidates the whole problem. I'll think a bit more about it :)

[Carol Szabo]

(In reply to comment #21)

Simon, thanks for your review.

> None of the functions in QWebPageClient need to be virtual.

You are right, but since all other QWebPageClient functions were virtual, I made these virtual as well just for consistency. In my next installment of the fix I made them non-virtual, except for the one that now needs to be virtual (see below).

> The updates that come in during the layout should not be sent to a QWidget (ownerWidget).

I assume that you are referring here to the updates during the layout that is happening during paint. I already took care of that with the skipWidgetUpdates flag. 

> the coordinates will be wrong (line 352). Think of a QGraphicsView setup where that widget will be the
> viewport and the coordinates need to be adjusted to the view's transformation and scrollbars.

You are right here again (no surprise). I have a new fix that takes care of the coordinate transformations in the dirtyRegion() function. I kept this function virtual and I added the widget that is the real view as a parameter. for QPageClientWidgetQt, no transformations are necessary, but in QPageClientGraphicsWidgetQt, I use the sceneTransform() * viewportTransform().inverted() to transform the region before returning it, which should yield the corect coordinates.

> 
> Secondly we should call QWidget::update() instead of repaint().
> 

Comment 22 shows me that you realized that this does not work.

> Lastly I don't think QWidget should be used at all and instead in QWebFramePrivate::renderRelativeCoords you should call
> void ChromeClientQt::invalidateContentsAndWindow(const IntRect& windowRect, bool immediate).

The immediate parameter does not work on invalidateContentsAndWindow. It is not implemented. QPageClientGraphicsWidget is pretty hacked when it comes to handling the view (it only works with the first view in some cases, which may not be the view currently painted (as Benjamin pointed out)). I'd rather go with the Widget.

> on the correct emission of the QWebPage::repaintRequested() signal. And lastly it will allow you to do the change
> without introducing a new function in the public QWebFrame API.

The repaintRequested signal is not clear to me as to when it should be triggered.
I have originally proposed a fix without API changes by calling directly QWebFramePrivate::renderRelativeCoords from QWebWidget and QGraphicsWebWidget, but Benjamin pointed out that QWebWidget and QGraphicsWebWidget are just examples of Views, the users of QtWebKit should be able to make their own view derived from QWidget, or QGraphicsItem. In order to not hinder this use case further then it is already hindered by other hacks which access private functions from QWebWidget and QGraphicsWebView, I created the public API to expose this functionality of QWebFramePrivate::renderRelativeCoords.

Before I can submit my new patch that addresses the issues that you pointed out, I wanted to fix some style issues in QPageClientWidgetQt and QPageClientGraphicsWidgetQt that interfere with my updated patch.
Please see my patch for bug 52020.

[Carol Szabo]

Created attachment 78245 [details]
Proposed Patch. Fixed problems pointed out by Simon.

[Carol Szabo]

Created attachment 78602 [details]
Partial Patch - solves problem for TiledBackingStore

[Carol Szabo]

Created attachment 78605 [details]
Proposed Patch. Fixes problem in the absence of a TiledBackingStore.

Detected a problem with Patch 78245 (would cause crash if a Backing store is used) and so I submitted this patch which remedies the oversight.

[Simon Hausmann]

Comment on attachment 78602 [details]
Partial Patch - solves problem for TiledBackingStore

I don't think the comment should be removed, because your patch only addresses the determination of the dirty tiles (it corrects that determination).

Also with the PaintBegin() call moved up, it is now possible that PaintBegin() is called and the early return is hit, causing us to _not_ call PaintEnd() on the client. I think we should maintain the symmetry there.

[Carol Szabo]

Created attachment 79221 [details]
Proposed Patch - Fixes problem when webkit tiling is used.

Fixed problems pointed out by Simon.

[Simon Hausmann]

Comment on attachment 79221 [details]
Proposed Patch - Fixes problem when webkit tiling is used.

LGTM :)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79221 [details]:

http/tests/xmlhttprequest/basic-auth-nopassword.html bug 53170
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 79221 [details]
Proposed Patch - Fixes problem when webkit tiling is used.

Clearing flags on attachment: 79221

Committed r76689: <http://trac.webkit.org/changeset/76689>

[Carol Szabo]

Created attachment 80502 [details]
Proposed Patch. Updated to match current Webkit Source Code structure

[Andreas Kling]

Comment on attachment 80502 [details]
Proposed Patch. Updated to match current Webkit Source Code structure

View in context: https://bugs.webkit.org/attachment.cgi?id=80502&action=review

> Source/WebKit/qt/Api/qwebframe.cpp:1219
> +  \since unreleased

We don't use this, better use \since 4.8 or nothing at all. :)

> Source/WebKit/qt/Api/qwebframe.cpp:1234
> +void QWebFrame::updateView(QWidget* view, QPainter* painter,  RenderLayer layer, const QRegion& currentDirtyRegion)

Double spaces before RenderLayer

> Source/WebKit/qt/Api/qwebframe.cpp:1238
> +    GraphicsContext context(painter);
> +    if (context.paintingDisabled() && !context.updatingControlTints())
> +        return;

This code looks strange. Since you're creating the GraphicsContext here, just do if (!painter) return;

> Source/WebKit/qt/Api/qwebframe.h:176
> +    void updateView(QWidget* view, QPainter*,  RenderLayer, const QRegion& currentDirtyRegion);

Two spaces after QPainter*,

[Noam Rosenthal]

Obsolete.

[rahmanih]

Created attachment 130309 [details]
TV screenshot showing the issue.

Hi all,

I'm using qt-4.8.0 on a SetTop Box.
attached is a TV screenshot showing the issue reported by Carol in this bug.

As you can see some images are not fully displayed.
This happens when using the css transitions to move image from one side to another.

I tried to apply the latest attached patch here, but I didn't succeed.

So is it possible to reopen this bug, and may align the patch with the QtWebKit-4.9.0 source code

regards.
Haithem.