Bug 73338 – [Qt] [WK2] QQuickWebView covers QML elements that should be rendered on top.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

QQuickWebPage renders contents in canvas afterRendering() handler and covers all QML elements.

[Viatcheslav Ostapenko]

Created attachment 117009 [details]
Patch

[Noam Rosenthal]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Viatcheslav Ostapenko]

(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.

[Noam Rosenthal]

(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.

[Viatcheslav Ostapenko]

(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.

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

(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).

[Viatcheslav Ostapenko]

Created attachment 117214 [details]
Painting only patch

[Caio Marcelo de Oliveira Filho]

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?

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

Created attachment 117231 [details]
Fix Changelog mess

[Caio Marcelo de Oliveira Filho]

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.

[Viatcheslav Ostapenko]

(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.

[Noam Rosenthal]

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.

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

Created attachment 117313 [details]
QSGMaterial based painting

[WebKit Review Bot]

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.

[Viatcheslav Ostapenko]

Created attachment 117338 [details]
Fix style.

[Noam Rosenthal]

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.

[Noam Rosenthal]

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)

[Viatcheslav Ostapenko]

Created attachment 117418 [details]
Final comment fixes. Reviewed by Noam.

[WebKit Review Bot]

Comment on attachment 117418 [details]
Final comment fixes. Reviewed by Noam.

Clearing flags on attachment: 117418

Committed r101683: <http://trac.webkit.org/changeset/101683>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.