Bug 39691

Apparently right now QtWebkit crashes when using a QGLWidget as a viewport on a site that big, even without AC.

Created attachment 57822 [details] AC + Tiling

Attachment 57822 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:391: Missing spaces around = [whitespace/operators] [4] WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:836: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 57824 [details] AC + tiling (style fix)

Created attachment 57825 [details] Tiling performance issue I've noticed this while using valgrind - we're iterating over each pixel to get the tile for it out of a map. This is a bottleneck for a large web-page. In the patch I jump over the pixels based on the tile size but I'm not sure it's correct. Antti?

Created attachment 57826 [details] AC + Tiling

Created attachment 57827 [details] AC+ tiling (~uploaded wrong file)

Comment on attachment 57827 [details] AC+ tiling (~uploaded wrong file) Generally looks good to me, but it would be nice if Antti could have a look. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:50 + #define GRAPHICS_LAYER_TILING_THRESHOLD 2000 We shouldnt use defines, you can use a static const. and also please do not use capitalized naming, gGraphicsLayerTilingThreshold would be fine. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:119 + , public virtual TiledBackingStoreClient Should have indentation of 4 WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:841 + return mapFromScene(view->mapToScene(view->viewport()->visibleRegion().boundingRect()).boundingRect()).boundingRect().toAlignedRect(); Wow, lots of bounding here :-) Antti, is there no better way of doing this?

> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:841 > + return mapFromScene(view->mapToScene(view->viewport()->visibleRegion().boundingRect()).boundingRect()).boundingRect().toAlignedRect(); Actually looking at it again one boundingRect is redundant, return mapFromScene( view->mapToScene(view->viewport()->visibleRegion().boundingRect()) ).boundingRect().toAlignedRect(); would also work. There's no other way I found to get the visible portion of a QGraphicsItem.

Comment on attachment 57827 [details] AC+ tiling (~uploaded wrong file) r- due to my earlier comments. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:534 + // FIXME: There's currently no Qt API to know if a new region of an item is exposed, apart from overloading paint Have a bug been filed? Would be nice to add the link here.

Are we getting an updated patch? :)

Kenneth: I want anttik to look at this first to see if the content is right, especially the changes I made to tiling code itself, unless you're comfortable reviewing that code yourself.

if you are going to init the backbuffer with fill, i think it would be nice to fix it properly and call fill on the non-transparent case too. Quoting from qpixmap class reference "Warning: This will create a QPixmap with uninitialized data. Call fill() to fill the pixmap with an appropriate color before drawing onto it with QPainter" http://doc.qt.nokia.com/4.6/qpixmap.html#QPixmap-3 I noticed this uninitialized behavior on symbian, while using a test browser with tiles on.

No'am is this still relevant?

Apparently this is causing some issues, reopening

Created attachment 72770 [details] Patch

Comment on attachment 72770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72770&action=review LGTM > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:52 > +#define GRAPHICS_LAYER_TILING_THRESHOLD 2000 Maybe this needs change on a mobile device? Did you check the iphone source dump?

> > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:52 > > +#define GRAPHICS_LAYER_TILING_THRESHOLD 2000 > > Maybe this needs change on a mobile device? Did you check the iphone source dump? It's 2000 on relevant mobile devices :)

Comment on attachment 72770 [details] Patch Committed manually, r71253

committed manually, http://trac.webkit.org/changeset/71253.

Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137

(In reply to comment #21) > Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137 As discussed there on bug 41137#c13, this patch doesn't fix the issue, so I'm removing it as a blocker for 2.1.