QQuickWebPage renders contents in canvas afterRendering() handler and covers all QML elements.
Created attachment 117009 [details] Patch
Comment on attachment 117009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review This needs to be two patches; 1 for purging the tiled-backingstore, and one for using the QSG node. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:240 > + return QSize(1, 1); Comment > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:285 > + if (!m_textureId) { > + glGenTextures(1, &m_textureId); > + glBindTexture(GL_TEXTURE_2D, m_textureId); > + QImage image(1, 1, QImage::Format_ARGB32); > + image.fill(Qt::transparent); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, image.constBits()); > + } else > + glBindTexture(GL_TEXTURE_2D, m_textureId); nitpick: swap the directives and use early return > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:645 > + callOnMainThread(&NeedTileBuffersRecreateReq, > + new NeedTileBuffersRecreateReqData(m_drawingAreaProxy->page()->process(), > + m_drawingAreaProxy->page()->pageID())); For now LayerTreeHostProxyQt is not thread safe anyway; so maybe we should keep this for later.
Comment on attachment 117009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:276 > +void PageProxyTexture::bind() > +{ > + if (m_node->m_pagePrivate) > + m_node->m_pagePrivate->paintToCurrentGLContext(); Since we are making a "strange" use of QSGTexture::bind(), I think it deserves a comment. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:173 > + if (q_ptr->isVisible()) > + pageView->setFlag(QQuickItem::ItemHasContents); From IRC discussion I've understood that this will be used as hidden API. I suggest that you explain this either here or in the check for ItemHasContents.
(In reply to comment #2) > (From update of attachment 117009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review > > This needs to be two patches; 1 for purging the tiled-backingstore, and one for using the QSG node. Should it be some extra API for purging tiled backing store, or just separate functionality form current patch? I, actually, tried to do memory saving patch 1st and it is quite tricky to invoke something on paint thread without having paint node. > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:645 > > + callOnMainThread(&NeedTileBuffersRecreateReq, > > + new NeedTileBuffersRecreateReqData(m_drawingAreaProxy->page()->process(), > > + m_drawingAreaProxy->page()->pageID())); > > For now LayerTreeHostProxyQt is not thread safe anyway; so maybe we should keep this for later. I'm not sure, can I do process->send() from paint thread? I briefly looked around and all send() calls are invoked form main thread.
(In reply to comment #4) > Should it be some extra API for purging tiled backing store, or just separate functionality form current patch? > I, actually, tried to do memory saving patch 1st and it is quite tricky to invoke something on paint thread without having paint node. See below re. paint thread. I was actually thinking to do the QSG-paint-on-bind trick first, and add another patch for memory saving later as it's a different bug. > I'm not sure, can I do process->send() from paint thread? I briefly looked around and all send() calls are invoked form main thread. Let me rephrase: right now in mini-browser we disable QSG multi-thread, and code in QtWebKit is not expected (yet) to be safe for multithread. That's because webKit2 already parallelizes everything for us, so adding yet another thread for painting creates complexity and doesn't solve much.
(In reply to comment #3) > (From update of attachment 117009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:276 > > +void PageProxyTexture::bind() > > +{ > > + if (m_node->m_pagePrivate) > > + m_node->m_pagePrivate->paintToCurrentGLContext(); > > Since we are making a "strange" use of QSGTexture::bind(), I think it deserves a comment. I'll add comment. > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:173 > > + if (q_ptr->isVisible()) > > + pageView->setFlag(QQuickItem::ItemHasContents); > > From IRC discussion I've understood that this will be used as hidden API. I suggest that you explain this either here or in the check for ItemHasContents. The kind of API is to clear flag ItemHasContents when page is not visible. This way it would cause drop of paint node. The question is, where to add this comment? And, actually, this might be not necessary. It can be done also by removing item from canvas - it will also drop all nodes associated with item.
Comment on attachment 117009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review > Source/WebKit2/ChangeLog:12 > + This considered to be temporary until QSGNode::UserNodeType will be available. FWI we'll try to get this GL painted user node thing solved as soon as possible, maybe January or February.
(In reply to comment #7) > (From update of attachment 117009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117009&action=review > > > Source/WebKit2/ChangeLog:12 > > + This considered to be temporary until QSGNode::UserNodeType will be available. > > FWI we'll try to get this GL painted user node thing solved as soon as possible, maybe January or February. That's great. Any objections to using this temporary hack until then? I'm inclined to let it in (once the comments are addressed).
Created attachment 117214 [details] Painting only patch
Comment on attachment 117214 [details] Painting only patch View in context: https://bugs.webkit.org/attachment.cgi?id=117214&action=review ChangeLog is duplicated. Also some comments. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:241 > + // Return size of this texture. This line can be removed. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:294 > + if (!(flags() & ItemHasContents)) { This new version doesn't set ItemHasContents. Is that intentional? About where to comment on the hidden API, I think here is a fine place to comment if you want to have it. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:296 > + if (oldNode) > + delete oldNode; Should we clear m_paintNode here too?
(In reply to comment #10) > (From update of attachment 117214 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117214&action=review > > ChangeLog is duplicated. Also some comments. Oops! :( > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:241 > > + // Return size of this texture. > > This line can be removed. Ok > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:294 > > + if (!(flags() & ItemHasContents)) { > > This new version doesn't set ItemHasContents. Is that intentional? About where to comment on the hidden API, I think here is a fine place to comment if you want to have it. Yes, it is intentional. 1. It has no use in this patch because it is painting only. 2. It can be done other way (like removeFromCanvas test does), so I'll skip it completely. > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:296 > > + if (oldNode) > > + delete oldNode; > > Should we clear m_paintNode here too? No. Destructor will be called and it there resetPaintNode will be called.
Created attachment 117231 [details] Fix Changelog mess
Comment on attachment 117214 [details] Painting only patch View in context: https://bugs.webkit.org/attachment.cgi?id=117214&action=review >>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:296 >>> + delete oldNode; >> >> Should we clear m_paintNode here too? > > No. > Destructor will be called and it there resetPaintNode will be called. Thanks for explaining. I still have a last question: Is resetPaintNode necessary? IOW, can a node be destroyed before the item behind our back? If not I think it is better to explicitly clear m_paintNode here and get rid of resetPaintNode.
(In reply to comment #13) > (From update of attachment 117214 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117214&action=review > > >>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:296 > >>> + delete oldNode; > >> > >> Should we clear m_paintNode here too? > > > > No. > > Destructor will be called and it there resetPaintNode will be called. > > Thanks for explaining. I still have a last question: Is resetPaintNode > necessary? IOW, can a node be destroyed before the item behind our back? > If not I think it is better to explicitly clear m_paintNode here and get > rid of resetPaintNode. Yes, it can - when the item get removed from canvas. Check removeFromCanvas test in 1st patch. I fixed bug like this on internal branch for old (no AC) tiles implementation.
Comment on attachment 117231 [details] Fix Changelog mess View in context: https://bugs.webkit.org/attachment.cgi?id=117231&action=review Talked to the QSG guys about this; If we do a hack, we should use the material's virtual functions instead of the texture's. > Source/WebKit2/ChangeLog:12 > + This considered to be temporary until QSGNode::UserNodeType will be available. This is considered to be temporary until QSGNode::UserNodeType will be available. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:166 > + , m_paintNode(0) Any chance we could use OwnPtr here? > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:223 > + PageProxyTexture(PageProxyNode* node) : m_node(node), m_textureId(0) { } Remove double spacing, new lines. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:240 > + // We use transparent 1x1 texture to satisfy scene graph painter. We use a transparent 1x1 texture to satisfy the scene graph painter. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:288 > + glGenTextures(1, &m_textureId); > + glBindTexture(GL_TEXTURE_2D, m_textureId); > + QImage image(1, 1, QImage::Format_ARGB32); > + image.fill(Qt::transparent); > + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, image.constBits()); Comment (FIXME: temporary until Qt Scenegraph support custom painting). You can add that comment in several places, e.g. here and before the class definitions.
(In reply to comment #15) > (From update of attachment 117231 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117231&action=review > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:166 > > + , m_paintNode(0) > > Any chance we could use OwnPtr here? No way. Paint nodes are owned by QSGCanvas and deallocated usually by QSGCanvas.
Created attachment 117313 [details] QSGMaterial based painting
Attachment 117313 [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/qquickwebpage.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 117338 [details] Fix style.
Comment on attachment 117338 [details] Fix style. View in context: https://bugs.webkit.org/attachment.cgi?id=117338&action=review Some nitpicks.... not r'ing it yet, I feel like I need a second opinion on this. > Source/WebKit2/ChangeLog:10 > + updateState() method to draw texture mapper graphics layers. texture mapper -> TextureMapper > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:224 > +// FIXME: temporary until Qt Scenegraph will support custom painting Period at end of sentence. > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:227 > + virtual char const* const* attributeNames() const Weird signature. But it's a QSG reimp so nothing I can do about it :( > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:234 > + // vertexShader and fragmentShader are no-op shaders > + // All real painting is gone by texture mapper through LayerTreeHostProxy Ditto > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:291 > + if (node->m_pagePrivate) > + node->m_pagePrivate->paintToCurrentGLContext(); Comment something like "FIXME: Normally we wouldn't paint inside QSGMaterialShader::updateState, but this is a temporary hack until custom paint nodes are available.
Comment on attachment 117338 [details] Fix style. View in context: https://bugs.webkit.org/attachment.cgi?id=117338&action=review Talked to Simon, let's go ahead with this, please fix the style nitpicks before committing... >> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:234 >> + // All real painting is gone by texture mapper through LayerTreeHostProxy > > Ditto (Period at end of sentence)
Created attachment 117418 [details] Final comment fixes. Reviewed by Noam.
Comment on attachment 117418 [details] Final comment fixes. Reviewed by Noam. Clearing flags on attachment: 117418 Committed r101683: <http://trac.webkit.org/changeset/101683>
All reviewed patches have been landed. Closing bug.