Bug 33373

Summary: [Qt] Enable scrolling optimization for pages with embedded widgets
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, girish, hausmann, kenneth, skyul, webkit.review.bot, zecke
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Proposed patch
none
Same patch, with a manual test added
none
Same patch, style complaints addressed none

Description Andreas Kling 2010-01-08 03:27:33 PST
Currently QtWebKit repaints the entire viewport when scrolling any page with embedded widgets (e.g flash plugins)
Comment 1 Andreas Kling 2010-01-08 03:34:32 PST
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!
Comment 2 WebKit Review Bot 2010-01-08 03:38:46 PST
style-queue ran check-webkit-style on attachment 46127 [details] without any errors.
Comment 3 Eric Seidel (no email) 2010-01-09 15:56:34 PST
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.
Comment 4 Simon Hausmann 2010-01-10 01:35:03 PST
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.
Comment 5 Holger Freyther 2010-01-10 05:24:48 PST
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.
Comment 6 Holger Freyther 2010-01-10 05:31:02 PST
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.
Comment 7 Kenneth Rohde Christiansen 2010-01-11 02:24:33 PST
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.
Comment 8 Andreas Kling 2010-01-11 03:41:19 PST
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)
Comment 9 Andreas Kling 2010-01-11 09:36:51 PST
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.
Comment 10 WebKit Review Bot 2010-01-11 09:42:40 PST
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
Comment 11 Andreas Kling 2010-01-11 09:48:09 PST
Created attachment 46285 [details]
Same patch, style complaints addressed
Comment 12 Holger Freyther 2010-01-12 01:19:58 PST
Comment on attachment 46285 [details]
Same patch, style complaints addressed

Cool. The patch looks ready. I wish we could automate the crasher too. :)
Comment 13 WebKit Commit Bot 2010-01-14 04:15:13 PST
Comment on attachment 46285 [details]
Same patch, style complaints addressed

Clearing flags on attachment: 46285

Committed r53259: <http://trac.webkit.org/changeset/53259>
Comment 14 WebKit Commit Bot 2010-01-14 04:15:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Simon Hausmann 2010-01-29 05:36:34 PST
(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