Bug 33373 – [Qt] Enable scrolling optimization for pages with embedded widgets

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Andreas Kling]

Currently QtWebKit repaints the entire viewport when scrolling any page with embedded widgets (e.g flash plugins)

[Andreas Kling]

Created attachment 46127 [details]
Proposed patch

This patch enables optimized scrolling and makes sure that (Qt) PluginViews are invalidated when scrolling. (This is needed for them to move themselves properly.)

No regressions in the existing auto and manual tests on Qt/X11. Comments very welcome!

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Comment on attachment 46127 [details]
Proposed patch

This looks sane to me.  It seems whoever put in the previous Qt scrolling code might want to look at this though.

[Simon Hausmann]

Holger, Kenneth: Could you take a look at this one? It appears that the workaround for full repaints on pages with widgets isn't needed anymore.

[Holger Freyther]

First thing. Have you encountered a crash or why are you adding a null check? If you have encountered a crash please create a LayoutTest for this if not please remove it.

[Holger Freyther]

The second thing starts with an apology. The commit message I used when adding this it total crap, it lacks a reference to a testcase, a bug report... I'm really sorry about this.

IIRC I added the code before we even supported Netscape Plugins in QtWebKIt. This means we will need to check this patach not only against Netscape Plugins support but also the QWidget embedding.

Now to the second apology. We currently do not have any (automatic/manual) test for scrolling of embedded Qt plugins. Could you help out on this?

The manual test should be a testshell that embeds a QLabel as "alien" and non-alien widget and then asks the user to scroll it.

[Kenneth Rohde Christiansen]

Yes, I agree that we need to make sure that there are no regressions with this patch with regard to Qt plugins. I already wanted to remove this earlier as it is not really required for Netscape plugins, so the patch is heading in the right direction.

[Andreas Kling]

I added the null pointer check due to a crash that occurs when loading WebCore/manual-tests/qt/qtplugin.html:

WebCore::PluginView::invalidateRect(WebCore::IntRect const&) (PluginViewQt.cpp:653)
WebCore::Widget::invalidate() (Widget.h:147)
WebCore::PluginView::updatePluginWidget() (PluginViewQt.cpp:133)
WebCore::PluginView::frameRectsChanged() (PluginView.cpp:140)
WebCore::Widget::setFrameRect(WebCore::IntRect const&) (WidgetQt.cpp:75)
WebCore::PluginView::setFrameRect(WebCore::IntRect const&) (PluginView.cpp:124)
WebCore::RenderWidget::setWidgetGeometry(WebCore::IntRect const&) (RenderWidget.cpp:159)
WebCore::RenderWidget::updateWidgetPosition() (RenderWidget.cpp:308)
WebCore::RenderView::updateWidgetPositions() (RenderView.cpp:543)
WebCore::FrameView::performPostLayoutTasks() (FrameView.cpp:1380)
WebCore::FrameView::layout(bool) (FrameView.cpp:732)
WebCore::FrameView::visibleContentsResized() (FrameView.cpp:1010)
WebCore::ScrollView::updateScrollbars(WebCore::IntSize const&) (ScrollView.cpp:403)
WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) (ScrollView.cpp:239)
WebCore::FrameView::setContentsSize(WebCore::IntSize const&) (FrameView.cpp:372)
WebCore::FrameView::adjustViewSize() (FrameView.cpp:392)
WebCore::FrameView::layout(bool) (FrameView.cpp:698)
WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView>*) (FrameView.cpp:1119)
WebCore::Timer<WebCore::FrameView>::fired() (Timer.h:98)
WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:112)
WebCore::ThreadTimers::sharedTimerFired() (ThreadTimers.cpp:90)
WebCore::SharedTimerQt::timerEvent(QTimerEvent*) (SharedTimerQt.cpp:105)
QObject::event(QEvent*) (qobject.cpp:1204)
...
QApplication::exec() (qapplication.cpp:3576)
main (main.cpp:37)

[Andreas Kling]

Created attachment 46284 [details]
Same patch, with a manual test added

Added a simple manual test with one alien and one non-alien QLabel as per Holger's suggestion.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/QtLauncher/main.cpp:656:  Declaration has space between type name and * in QLabel *l  [whitespace/declaration] [3]
Total errors found: 1

[Andreas Kling]

Created attachment 46285 [details]
Same patch, style complaints addressed

[Holger Freyther]

Comment on attachment 46285 [details]
Same patch, style complaints addressed

Cool. The patch looks ready. I wish we could automate the crasher too. :)

[WebKit Commit Bot]

Comment on attachment 46285 [details]
Same patch, style complaints addressed

Clearing flags on attachment: 46285

Committed r53259: <http://trac.webkit.org/changeset/53259>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

(In reply to comment #13)
> (From update of attachment 46285 [details])
> Clearing flags on attachment: 46285
> 
> Committed r53259: <http://trac.webkit.org/changeset/53259>

Cherry-picked into qtwebkit-4.6 with commit ca7b2e1e1ca558050cf49dd8f7c9b35e4b9d4df5