Right now tiling works on the non-composited content only, which creates an unnatural distinction between "composited" and "non-composited" content. This results in hard-to-detect bugs, such as composited content looking different than non-composited content. The idea is to use the tiling functionality in TiledBackingStore and TiledDrawingAreaProxy in each layer, with some optimizations for the non-composited content, and to force compositing mode so that the rendering only goes through a single path.
Created attachment 109313 [details] Not for review. WIP patch.
Comment on attachment 109313 [details] Not for review. View in context: https://bugs.webkit.org/attachment.cgi?id=109313&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:247 > + bool found = false; exists? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:256 > + if (!m_transforms.target.isInvertible()) // not visible? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:389 > + > + only one newline > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:393 > + HashMap<int, ManagedTile>::iterator endIterator = m_managedTiles.end(); normally just called end in webkit > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:400 > + else if (opacity > 0.95) > + tiles.prepend(&it->second); comment? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:404 > + TransformationMatrix scaledMatrix = m_transforms.target; > + TransformationMatrix replicaMatrix; is this needed? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:719 > - m_tiles.clear(); > + m_basicTiles.clear(); owned/adopted tiles. externallyManagedTiles > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:111 > + enum TileMode { TileOwnership { Externally , I... > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:245 > + bool backBufferUpdated; isBack....
Created attachment 110060 [details] Patch
Comment on attachment 110060 [details] Patch This is the WebCore part of the patch. It can stand on its own, though functionality is still disabled.
Comment on attachment 110060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110060&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:360 > + targetRect.x() + (tileRect.x() - targetRect.x()) / m_state.contentsScale, It would be great if you could do such renames in separate patches... I can quickly r=cq=me them. It will keep the patch size down. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:394 > + > + double newline > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:395 > + if (m_state.tileOwnership == ExternallyManagedTiles) { This indeed looks nicer! > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:405 > + else if (opacity > 0.95) > + tiles.prepend(&it->second); You forgot adding the comment here that we talked about > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:426 > + > + Maybe for the sake of clearness you could add // InternallyManagedTiles: here > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:598 > +int TextureMapperNode::createContentTile(float scale) Contents? I wonder if there is a nice way to make it clear which methods are for the external managed tiles only > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:607 > +void TextureMapperNode::removeContentTile(int id) Contents? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:612 > +void TextureMapperNode::setTileBackBufferTextureForDirectlyCompositedImage(int id, const IntRect& sourceRect, const FloatRect& targetRect, BitmapTexture* texture) You use FloatRect& targetRect here... in setContentTileBackBuffer you use IntRect... why the inconsistency? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:628 > +void TextureMapperNode::clearDirectlyCompositedImageTiles() It could be a bit more clear if you add "All" in there. ie. clearAllDirec... > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:657 > +void TextureMapperNode::swapContentBuffers() Contents*? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:659 > + HashMap<int, ExternallyManagedTile>::iterator endIterator = m_externallyManagedTiles.end(); We normally just use "end" and not "endIterator" > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:772 > > + Wny this newline? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:153 > + void setTileOwnership(TileOwnership o) { m_state.tileOwnership = o; } please write "ownership" We normally use "o" for object in the Render* classes. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:248 > + ExternallyManagedTile(float newScale = 1.0) =1 ? Why newScale... isn't it just scale as this is a ctor
Created attachment 110383 [details] Patch
Comment on attachment 110383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110383&action=review > Source/WebCore/ChangeLog:9 > + Enable "externally managed" tiles in TextureMapperNodes. > + Currently, TextureMapperNodes manage tiles themselves, the tiles being there > + only to overcome the 2k texture size limitation. For WebKit2, we want those tiles to be managed > + externally, namely through the web process via the remote tile backend for TiledBackingStore. Why is the line length so different? This is a changelog.. and I would wrap around 80-100 chars and try to keep all lines at the about same length > Source/WebCore/ChangeLog:18 > + Reviewed by NOBODY (OOPS!). Move this up > Source/WebCore/ChangeLog:21 > + No new tests, code is still disabled. Eventually existing compositing tests running on WK2 would > + test this. Better write: Code is disabled for now, but covered by existing compositing tests. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:391 > + > + double newline > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:406 > + // This code creates a transitional effect, showing tiles rendered in the "old" contents scale behind tiles rendered in the current contents scale. > + // This effect looks good only when there's no transparency, so we only use it when the opacity for the layer is above a certain threshold. I would keep the comment to around 100 chars > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:255 > + struct ExternallyManagedTile { > + bool isBackBufferUpdated; > + bool isDirectlyCompositedImage; > + ExternallyManagedTileBuffer frontBuffer; > + ExternallyManagedTileBuffer backBuffer; > + ExternallyManagedTile(float scale = 1.0) > + : isBackBufferUpdated(false) > + , isDirectlyCompositedImage(false) > + , scale(scale) > + { > + } > + float scale; > + }; your ordering here is a bit weird
Created attachment 110525 [details] Patch
Comment on attachment 110525 [details] Patch Clearing flags on attachment: 110525 Committed r97163: <http://trac.webkit.org/changeset/97163>
All reviewed patches have been landed. Closing bug.
Not finished just yet.
Guys, it broke the minimal build ... :-S http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/34135/steps/compile-webkit/logs/stdio You should have watch the bots ... Could you fix it ASAP or should we roll it out?
(In reply to comment #12) > Guys, it broke the minimal build ... :-S > > http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/34135/steps/compile-webkit/logs/stdio > > You should have watch the bots ... Could you fix it ASAP or should we roll it out? Should be simple to fix, it's just about #if ENABLE(TILED_BACKING_STORE).
On second thought, after Noam's change the texture mapper doesn't make sense anymore if tiling is disabled. So either the tiled backing store is added to the minimal config (does that make sense?) or the texture mapper should be disabled if there's no backing store.
(In reply to comment #14) > On second thought, after Noam's change the texture mapper doesn't make sense anymore if tiling is disabled. So either the tiled backing store is added to the minimal config (does that make sense?) or the texture mapper should be disabled if there's no backing store. For WebKit2... but for WebKit1 TextureMapper can still potentially work with TILED_BACKING_STORE disabled. I'd rather just fix the #ifdefs for now.
Created attachment 110576 [details] Unbreak the minimal Qt bot
Comment on attachment 110576 [details] Unbreak the minimal Qt bot Clearing flags on attachment: 110576 Committed r97186: <http://trac.webkit.org/changeset/97186>
Created attachment 110595 [details] Patch This is still disabled, but most of the code is there. When enabled, it will allow using AC for all layers, including the main layer. We'd use "force compositing" mode to ensure that. This would in turn obsolete TiledDrawingArea and SGAgent, as those functionalities now reside in TiledBackingStore+TextureMapperNode.
This patch appear to have broken Qt minimal build. Fix attempted in http://trac.webkit.org/changeset/97210.
(In reply to comment #20) > This patch appear to have broken Qt minimal build. Fix attempted in http://trac.webkit.org/changeset/97210. Fixed in http://trac.webkit.org/changeset/97213.
Comment on attachment 110595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110595&action=review Some comments from my first look thru... still need to make sense of this patch > Source/WebKit2/ChangeLog:12 > + Reviewed by NOBODY (OOPS!). > + > + Layer-tree synchronization is done by passing WebKit2 messages between a LayerTreeHost > + on the WebProcess and a LayerTreeHostProxy on the UI process. Every layer in the tree has > + its own tiled backing store, and uses the LayerTreeHost communication channel to pass > + content up to the UI process. The UI process will later creates its own GraphicsLayer tree, > + based on TextureMapper, which can be painted directly with OpenGL. Would be nice if you started with a reasoning for the patch, before descripting what it is doing > Source/WebKit2/Shared/WebLayerTreeInfo.h:76 > + bool imageUpdated: 1; imageWasUpdated? > Source/WebKit2/UIProcess/DrawingAreaProxy.h:87 > + virtual void updateWebView(); Wouldn't updateViewport be a better name? WebView is totally overloaded > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:48 > +class LayerTreeMessageToRenderer; > +class LayerTreeHostProxy : public GraphicsLayerClient { I would adda newline between these > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:75 > +protected: > + > + PassOwnPtr<GraphicsLayer> createLayer(WebLayerID); I would remove that newline > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:83 > + virtual bool showDebugBorders() const { return false; } > + virtual bool showRepaintCounter() const { return false; } #if !NDEBUG ? > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:103 > + int remoteTileIDToNodeTileID(int tileID) const; I would have called it "nodeTileIDFromRemoteTileID" > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:121 > + void ensureLayer(WebLayerID); > + > +#endif remove that newline > Source/WebKit2/UIProcess/LayerTreeHostProxy.messages.in:1 > +# Copyright (C) 2010, 2011 Apple Inc. All rights reserved. s/Apple/Nokia ?? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:39 > +#include <cairo/OpenGLShims.h> cairo? > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:87 > + m_layerInfo.id = sID++; s_id? :-) s means static right > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:297 > + setContentsToImage(0); setImageAsContents? the name is a bit confusing especially when passing 0 > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:416 > + if (m_mainBackingStore && m_mainBackingStore->contentsScale() == scale) no fuzzy comparison needed here? > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:524 > + if (!drawsContent() || m_image) so you dont do this if you have an m_image? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:63 > + , m_suspended(false) isSuspended? or isPageSuspended ? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:192 > +void LayerTreeHostQt::didInstallPageOverlay() Attach? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:199 > +void LayerTreeHostQt::didUninstallPageOverlay() Detach? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:291 > + m_pageOverlayLayer = nullptr; Why not use that everywhere then instead of 0? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:294 > + > + double newlines
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:39 > > +#include <cairo/OpenGLShims.h> > > cairo? We're reusing Cairo's OpenGL shims, in other places as well. > > > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:192 > > +void LayerTreeHostQt::didInstallPageOverlay() > Attach? > > +void LayerTreeHostQt::didUninstallPageOverlay() > Detach? These are copied from LayerTreeHostCA.
Created attachment 110705 [details] Patch Addressed Kenneth's comments, though some of them are addressed in the previous reply.
Comment on attachment 110705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110705&action=review > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:1 > + /* Stray tab? :) > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:31 > +#include "LayerTreeHostQt.h" This header file seems missing?
Created attachment 110873 [details] Patch
Attachment 110873 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:69: The parameter name "layerID" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:70: The parameter name "layerID" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:71: The parameter name "layerID" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 110873 [details] Patch Great stuff :)
Comment on attachment 110873 [details] Patch r=me but please fix the style issues before landing :)
Created attachment 110878 [details] Patch Fixed style issues and last comments from Simon.
Created attachment 110938 [details] Patch for landing
Still some patches to go.
Comment on attachment 110878 [details] Patch Attachment 110878 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10068216
Comment on attachment 110878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110878&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:35 > +#include "LayerTreeHostProxyMessages.h" As the EWS indicated: This header inclusion should be either guarded with the appropriate #ifdefs or the LTHPM.in file is added to the build systems of the other ports.
(In reply to comment #34) > (From update of attachment 110878 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110878&action=review > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:35 > > +#include "LayerTreeHostProxyMessages.h" > > As the EWS indicated: This header inclusion should be either guarded with the appropriate #ifdefs or the LTHPM.in file is added to the build systems of the other ports. Yup. I'll push another patch with #ifdef guards, and make sure it passes EWS.
Created attachment 111020 [details] Patch This should fix it, will let go through EWS and then land.
Created attachment 111051 [details] Patch
Comment on attachment 111051 [details] Patch Clearing flags on attachment: 111051 Committed r97549: <http://trac.webkit.org/changeset/97549>
(In reply to comment #38) > (From update of attachment 111051 [details]) > Clearing flags on attachment: 111051 > > Committed r97549: <http://trac.webkit.org/changeset/97549> It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout.
Rollout patch landed in http://trac.webkit.org/changeset/97554
> It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout. Easy to say. It took over 6 hours for the commit queue to land in that patch. I'm ok with rolling this out for now.
(In reply to comment #42) > > It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout. > > Easy to say. It took over 6 hours for the commit queue to land in that patch. > I'm ok with rolling this out for now. It didn't actually break any bot. Flaky tests.... recommitting. Please don't roll out unless bot failures have something to do with the bug.
Comment on attachment 111051 [details] Patch Re-committing, patch was rolled out because it was mistakenly blamed for flaky tests.
Comment on attachment 111051 [details] Patch Clearing flags on attachment: 111051 Committed r97559: <http://trac.webkit.org/changeset/97559>
Breaking stuff... more offensive #includes to move around.
(In reply to comment #42) > > It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout. > > Easy to say. It took over 6 hours for the commit queue to land in that patch. > I'm ok with rolling this out for now. Using commit-queue isn't compulsory. You can land your patch manually or with webkit-patch land-from-bug if you can't wait for CQ. But you have to ensure that you don't break build on any platform. It is another question why Mac port doesn't have EWS as other ports.
Created attachment 111274 [details] Patch One more try... moved some headers around and adapted to trunk. Should build on everything now - will follow the bots.
Attachment 111274 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource From git://git.webkit.org/WebKit bc67541..f85897f master -> origin/master M Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/garden-o-matic.html M Tools/ChangeLog r97634 = 07d47256354548be7595c405835f38f40c1b61e9 (refs/remotes/trunk) M LayoutTests/fast/replaced/table-percent-width-expected.txt M LayoutTests/ChangeLog M Source/WebCore/ChangeLog M Source/WebCore/rendering/RenderBox.h M Source/WebCore/rendering/RenderBox.cpp r97635 = f85897f3766189463c74375cf745a6d69f470fbd (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 111289 [details] Patch
Comment on attachment 111289 [details] Patch Clearing flags on attachment: 111289 Committed r97639: <http://trac.webkit.org/changeset/97639>
Created attachment 111706 [details] Patch This patch effectively enables the previous patches. It's a "big" change in terms of rendering and might cause some instability. However, I hope we'd be able to fix those by going forwards and not backwards, as it's very hard to fix all those issues in advance on a moving target like QTouchWebView.
Comment on attachment 111706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review Looks good, but I want jocelyn to have a quick look > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:48 > +void QTouchWebPage::initSceneGraphConnections() initialize*? > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53 > + QSGCanvas* canvas = this->canvas(); Any reason for this? > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:173 > +static void computeOverlaps(const QSGItem* curItem, const QSGItem* item, bool& hasBehind, bool& hasInFront, const QRectF& itemBoundingRect, bool nowInFront = false) currentItem. What is the other item? That could be more clear. hasOverlapInFront? maybe someone has better suggestions > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:181 > + QList<QSGItem *> childItems = curItem->childItems(); coding style > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:190 > + (nowInFront ? hasInFront : hasBehind) = true; That is pretty ugly :-) bnut well > Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:58 > + QWebPreferencesPrivate::get(preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, false); Maybe we should just add a public setting? > Source/WebKit2/UIProcess/qt/qtouchwebpageproxy.cpp:60 > + DrawingAreaProxy* drawingArea = m_webPageProxy->drawingArea(); > + if (drawingArea) This could be one line
Comment on attachment 111706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review LGTM beside a few details. > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:92 > if (!oldNode) > oldNode = new QSGNode; You could remove updatePaintNode completely and remove "setFlag(ItemHasContents);" from the constructor. > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:259 > + QObject::connect(engine, SIGNAL(beforeRendering()), q, SLOT(_q_onBeforeSceneRender()), Qt::DirectConnection); > + QObject::connect(engine, SIGNAL(afterRendering()), q, SLOT(_q_onAfterSceneRender()), Qt::DirectConnection); I'd prefer if it was hard-coded to one of the two to make the limitation obvious (probably beforeRendering to allow overlays) until we find a proper way to integrate with the scene graph, or decide that this is the way to go. > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:70 > - > + This one slipped in.
(In reply to comment #56) > > Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:58 > > + QWebPreferencesPrivate::get(preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, false); > > Maybe we should just add a public setting? See comment in code... There's no point in a public setting if we're going to rely on AC as the main rendering code path.
> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:92 > > if (!oldNode) > > oldNode = new QSGNode; > > You could remove updatePaintNode completely and remove "setFlag(ItemHasContents);" from the constructor. OK, though eventually the idea would be to implement this if the item is in the middle of the scene. > > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:259 > > + QObject::connect(engine, SIGNAL(beforeRendering()), q, SLOT(_q_onBeforeSceneRender()), Qt::DirectConnection); > > + QObject::connect(engine, SIGNAL(afterRendering()), q, SLOT(_q_onAfterSceneRender()), Qt::DirectConnection); > > I'd prefer if it was hard-coded to one of the two to make the limitation obvious (probably beforeRendering to allow overlays) until we find a proper way to integrate with the scene graph, or decide that this is the way to go. Right now, though, there are items behind the web-view, background and such. But I can make it work with "behind" I believe by changing the QML in MiniBrowser. It would make the code much simpler because computeOverlap can be omitted.
(In reply to comment #56) > (From update of attachment 111706 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53 > > + QSGCanvas* canvas = this->canvas(); > > Any reason for this? QTouchWebView/QTouchWebPage can be constructed without attaching to canvas initially. In this case initialization will be delayed until QTouchWebPage::itemChange with change == ItemSceneChange .
(In reply to comment #60) > (In reply to comment #56) > > (From update of attachment 111706 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review > > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53 > > > + QSGCanvas* canvas = this->canvas(); > > > > Any reason for this? > > QTouchWebView/QTouchWebPage can be constructed without attaching to canvas initially. In this case initialization will be delayed until QTouchWebPage::itemChange with change == ItemSceneChange . I think the question was about using a local variable, which is probably not necessary. We should add a comment along the lines of your reply here, though.
Created attachment 111978 [details] Patch For now, the easiest way forward is to render the web view after the rest of the scene. Later on we can use FBO for the rest of the cases.
Created attachment 111979 [details] Patch
Attachment 111978 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:193: QTouchWebPagePrivate::_q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:199: QTouchWebPagePrivate::_q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:41: _q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42: _q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #64) > Attachment 111978 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:193: QTouchWebPagePrivate::_q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:199: QTouchWebPagePrivate::_q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:41: _q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42: _q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > Total errors found: 4 in 14 files > False positives, those things are supposed to work with the Qt coding style.
Comment on attachment 111979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review > Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54 > + m_pageView->update(); This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me.
(In reply to comment #66) > (From update of attachment 111979 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review > > > Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54 > > + m_pageView->update(); > > This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me. Should we fix it with setting ItemHasContents to true, or by updating the canvas some other way?
(In reply to comment #67) > (In reply to comment #66) > > (From update of attachment 111979 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review > > > > > Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54 > > > + m_pageView->update(); > > > > This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me. > > Should we fix it with setting ItemHasContents to true, or by updating the canvas some other way? I think setting ItemHasContents to true is ok. It did the trick for me. I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same?
> I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same? I've seen this before, and it might have come back... it's hard to notice crispness bugs sometimes. Does it go back to crispness once you zoom in and out? and is it crisp on other layers? No'am
(In reply to comment #69) > > I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same? > > I've seen this before, and it might have come back... it's hard to notice crispness bugs sometimes. > Does it go back to crispness once you zoom in and out? and is it crisp on other layers? It goes away when I zoom in enough. Ah well, that shouldn't prevent us from moving forward.
Comment on attachment 111979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53 > +void QTouchWebPage::initSceneGraphConnections() > +{ > + if (d->paintingIsInitialized) > + return; > + d->paintingIsInitialized = true; > + if (!canvas()) > + return; With the QML mini browser I'm running into the situation where when this function is called from the constructor, we do not have a canvas() yet. Later when the canvas is assigned and we get notified through itemChange(), this function is called again. Unfortunately paintingIsInitialized was set to true and therefore we don't end up connecting to the scene graph signal(s). Moving the paintingIsInitialized = true assigned to _after_ the "if (!canvas()) return" block fixes that.
Yay, I got it to run on the N9. The only "error" I saw was this: ERROR: [TextureMapper GL] Command failed: glBindFramebuffer(GL_FRAMEBUFFER, 0) (501) ../../../Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp(725) : void WebCore::debugGLCommand(const char*, int) from void TextureMapperGL::bindSurface(BitmapTexture *surfacePointer) { BitmapTextureGL* surface = static_cast<BitmapTextureGL*>(surfacePointer); if (!surface) { GL_CMD(glBindFramebuffer(GL_FRAMEBUFFER, 0)) Anyway, I don't think that's a "showstopper" :)
(In reply to comment #72) > Yay, I got it to run on the N9. > > The only "error" I saw was this: > > ERROR: [TextureMapper GL] Command failed: glBindFramebuffer(GL_FRAMEBUFFER, 0) (501) > > ../../../Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp(725) : void WebCore::debugGLCommand(const char*, int) Sounds familiar. Some of the N9-specific stuff was not upstreamed yet, namely going to software mode when going to the background and other similar candies.
Comment on attachment 111979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review r=me, with two changes: 1) Revert the ItemHasContents piece to avoid the ASSERT 2) Fix the location of the d->paintingIsInitialized = true assignment We plan on doing the next qt5 update tomorrow, which will require us to adapt to source incompatible changes (QSG* -> QQuick* renaming in qtdeclarative). So perhaps you can land this today your time? Otherwise the patch will need a rebase :) > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:160 > +float computeEffectiveOpacity(const QSGItem* item) Missing 'static' keyword :)
Landing soon would be awesome, so then we can go ahead with more cleanups around the qt wk2 classes. nice work btw.
Committed r98706: <http://trac.webkit.org/changeset/98706>