Bug 66557 – [Qt] Make QDesktopWebView a QSGItem.

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Andreas Kling]

[Qt] Make QDesktopWebView a QSGItem.

[Andreas Kling]

Created attachment 104510 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 104510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104510&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:47
> +    , geometry(QSGGeometry::defaultAttributes_TexturedPoint2D(), 4)

maybe add a comment before the 4?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:56
> +static QImage createCrashedPageImage(int width, int height)

so the page image crashed? :-) Darn we need a better name

[Jocelyn Turcotte]

Comment on attachment 104510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104510&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:67
> +    QSGGeometryNode* node = reinterpret_cast<QSGGeometryNode*>(oldNode);

static_cast?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:83
> +    QRectF textureSourceRect(0, 0, width() / backingStoreImage.width(), height() / backingStoreImage.height());

QSGTexture::convertToNormalizedSourceRect can do that for you.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:86
> +    delete d->material.texture();

Would be cleaner to keep it in a local variable and delete it later, in cases where QSGMaterial::setTexture would decides to access it's current texture.

[Benjamin Poulain]

Comment on attachment 104510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104510&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:51
> -class QWEBKIT_EXPORT QDesktopWebView : public QSGPaintedItem {
> +class QWEBKIT_EXPORT QDesktopWebView : public QSGItem {

The include <QtDeclarative/qsgpainteditem.h> can be removed.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:101
> +    virtual QSGNode *updatePaintNode(QSGNode *, UpdatePaintNodeData *);

Currently we use WebKit style for public headers.

[Andreas Kling]

Comment on attachment 104510 [details]
Patch

Will make an updated patch with everyone's comments addressed, thanks.

[Andreas Kling]

Created attachment 104662 [details]
Proposed patch v2

Updated patch according to comments.
Didn't end up using QSGTexture::convertToNormalizedSourceRect() since that got more verbose than the old code.
And we now delete the texture in ~QDesktopWebView().

[Andreas Kling]

Created attachment 104663 [details]
Proposed patch v3

D'oh. Fixed a last-minute bug when QDesktopWebView is not at (0,0) in the scene.
We shouldn't include the item position in the rect passed to updateTexturedRectGeometry().

[Jocelyn Turcotte]

Comment on attachment 104663 [details]
Proposed patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=104663&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:249
>  QDesktopWebView::~QDesktopWebView()
>  {
> +    delete d->material.texture();

Textures should only be destroyed on the SG thread. Their destructor calls glDeleteTextures and the SG thread might still use it in rendering right after the GUI thread deleted it.
It's a bit annoying to handle ownership of those and I'm starting to think that QSGTextureMaterial should have some OwnsTexture flag like QSGNode have for materials.