Bug 73338 - [Qt] [WK2] QQuickWebView covers QML elements that should be rendered on top.
Summary: [Qt] [WK2] QQuickWebView covers QML elements that should be rendered on top.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-11-29 11:06 PST by Viatcheslav Ostapenko
Modified: 2011-12-01 10:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (23.51 KB, patch)
2011-11-29 11:37 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Painting only patch (10.62 KB, patch)
2011-11-30 10:18 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix Changelog mess (9.33 KB, patch)
2011-11-30 11:11 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
QSGMaterial based painting (9.78 KB, patch)
2011-11-30 19:42 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix style. (9.81 KB, patch)
2011-11-30 22:16 PST, Viatcheslav Ostapenko
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff
Final comment fixes. Reviewed by Noam. (9.96 KB, patch)
2011-12-01 07:43 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2011-11-29 11:06:47 PST
QQuickWebPage renders contents in canvas afterRendering() handler and covers all QML elements.
Comment 1 Viatcheslav Ostapenko 2011-11-29 11:37:22 PST
Created attachment 117009 [details]
Patch
Comment 2 Noam Rosenthal 2011-11-29 11:48:37 PST
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 3 Caio Marcelo de Oliveira Filho 2011-11-29 11:59:13 PST
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.
Comment 4 Viatcheslav Ostapenko 2011-11-29 14:52:37 PST
(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.
Comment 5 Noam Rosenthal 2011-11-29 15:00:26 PST
(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.
Comment 6 Viatcheslav Ostapenko 2011-11-29 15:06:47 PST
(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 7 Jocelyn Turcotte 2011-11-30 09:39:12 PST
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.
Comment 8 Noam Rosenthal 2011-11-30 10:01:52 PST
(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).
Comment 9 Viatcheslav Ostapenko 2011-11-30 10:18:36 PST
Created attachment 117214 [details]
Painting only patch
Comment 10 Caio Marcelo de Oliveira Filho 2011-11-30 10:28:13 PST
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?
Comment 11 Viatcheslav Ostapenko 2011-11-30 10:53:38 PST
(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.
Comment 12 Viatcheslav Ostapenko 2011-11-30 11:11:55 PST
Created attachment 117231 [details]
Fix Changelog mess
Comment 13 Caio Marcelo de Oliveira Filho 2011-11-30 11:14:21 PST
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.
Comment 14 Viatcheslav Ostapenko 2011-11-30 11:38:27 PST
(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 15 Noam Rosenthal 2011-11-30 13:10:40 PST
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.
Comment 16 Viatcheslav Ostapenko 2011-11-30 19:35:11 PST
(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.
Comment 17 Viatcheslav Ostapenko 2011-11-30 19:42:38 PST
Created attachment 117313 [details]
QSGMaterial based painting
Comment 18 WebKit Review Bot 2011-11-30 19:50:39 PST
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.
Comment 19 Viatcheslav Ostapenko 2011-11-30 22:16:18 PST
Created attachment 117338 [details]
Fix style.
Comment 20 Noam Rosenthal 2011-11-30 22:28:03 PST
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 21 Noam Rosenthal 2011-12-01 00:39:01 PST
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)
Comment 22 Viatcheslav Ostapenko 2011-12-01 07:43:04 PST
Created attachment 117418 [details]
Final comment fixes. Reviewed by Noam.
Comment 23 WebKit Review Bot 2011-12-01 10:02:35 PST
Comment on attachment 117418 [details]
Final comment fixes. Reviewed by Noam.

Clearing flags on attachment: 117418

Committed r101683: <http://trac.webkit.org/changeset/101683>
Comment 24 WebKit Review Bot 2011-12-01 10:02:41 PST
All reviewed patches have been landed.  Closing bug.