Bug 76546 – [Qt][WK2] WebKitTestRunner should use 480x360 sized view for W3C SVG tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

.

[Szilard Ledan]

Created attachment 126068 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 126068 [details]
patch

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

This looks really hacked in, i dont like it

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1108
> -    d->updateViewportSize();
> +    d->updateViewportSize(d->q_ptr->boundingRect().size().toSize());

very ugly

[Szilard Ledan]

Created attachment 126078 [details]
fixed patch

Thanks for your comment. I fixed it.

[Kenneth Rohde Christiansen]

Comment on attachment 126078 [details]
fixed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:440
> +void QQuickWebViewLegacyPrivate::updateViewportSize(QSize s)

const QSize& newSize

Also this is not an update method anymore, more a setter, setViewportSize(const QSize& size)

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:91
>      m_window->resize(width, height);
> +    m_view->geometryChanged(QRectF(0, 0, width, height), QRectF(0, 0, m_view->width(), m_view->height()));

Why not do

QRectF oldGeometry(0, 0, m_view->width(), ...);
QRectF newGeometry...

geometryChanged(newGeometry, oldGeometry)

[Balazs Kelemen]

You should explain in the changelog what are you fixing and how it is broken currently. Also I would like to see how it effects minibrowser behaviour - as I believe the should fix resizing behaviour generally, at least for desktop mode.

[Balazs Kelemen]

Comment on attachment 126078 [details]
fixed patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:440
>> +void QQuickWebViewLegacyPrivate::updateViewportSize(QSize s)
> 
> const QSize& newSize
> 
> Also this is not an update method anymore, more a setter, setViewportSize(const QSize& size)

Well, I disagree with that. That's not just a setter but it's also handle resizing the view - which it does not handle currently, that is the bug :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:442
> +    QSize viewportSize = s;

let the argument's name viewportSize and remove the temporal (and useless) copy of it

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:529
> +    QSize viewportSize = s;

same as above about s and viewportSize

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:81
> -    virtual void updateViewportSize() { }
> +    virtual void updateViewportSize(QSize s) { }

Please don't add name for the argument. In the case when the role of the argument is straightforward - and in this case it is - our style say don't add a name for it in the declaration. And use const reference passing for non-primitive types.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:159
> +    virtual void updateViewportSize(QSize s);

ditto

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:175
> +    virtual void updateViewportSize(QSize s);

ditto

[Szilard Ledan]

Created attachment 126285 [details]
patch

Thanks you for your help and your patience!

[Simon Hausmann]

Comment on attachment 126285 [details]
patch

Wouldn't it be much easier to call simply call setResizeMode(QQuickView::SizeRootObjectToView); in WTR to ensure that the qquick item has the size of the window? (provided it is the root item :)

[Szilard Ledan]

Created attachment 127193 [details]
Patch

[WebKit Review Bot]

Attachment 127193 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
git.webkit.org[0: 17.254.20.231]: errno=Connection refused
fatal: unable to connect a socket (Connection refused)
Died at Tools/Scripts/update-webkit line 162.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Comment on attachment 127193 [details]
Patch

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

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:61
> +        m_view->setHeight(600);

You could use setPos and setWidth, that would avoid some extra work.

[Simon Hausmann]

Comment on attachment 127193 [details]
Patch

I agree with Alexis, but at the same time I think it's a smaller matter. r=me :)

I've tried to reproduce the issue with a stand-alone test-case, but I haven't had any luck yet. It would be good to find out why the anchors.fill doesn't work though.

[WebKit Review Bot]

Comment on attachment 127193 [details]
Patch

Clearing flags on attachment: 127193

Committed r107868: <http://trac.webkit.org/changeset/107868>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

Regarding the previous approach (with anchor.fill binding), it only works if the window is showed. Seems like the way how we try to workaround it does not sufficient, i.e. the following:

QWindowSystemInterface::handleWindowActivated(this);
m_view->page()->setFocus(true);

does not have the same effect as window->show(). I guess it's not a bug that the binding not work when the window is not showed, so we can live with that. However, if you have an idea how to properly simulate an active window, please tell it, as it would also solve the issue we worked around with the QQuickWebViewExperimental::setRenderToOffscreenBuffer private API. Maybe it's also an option to just show the window, as it is recommended to run tests in xvfb anyway, but I guess people would not like it, and also xvfb is only available on Linux.

[Balazs Kelemen]

s/Linux/non-Linux

[Balazs Kelemen]

(In reply to comment #16)
> s/Linux/non-Linux

No, the sentence was correct, please ignore this comment. Sigh....

[Simon Hausmann]

(In reply to comment #15)
> Regarding the previous approach (with anchor.fill binding), it only works if the window is showed. Seems like the way how we try to workaround it does not sufficient, i.e. the following:
> 
> QWindowSystemInterface::handleWindowActivated(this);
> m_view->page()->setFocus(true);
> 
> does not have the same effect as window->show(). I guess it's not a bug that the binding not work when the window is not showed, so we can live with that. However, if you have an idea how to properly simulate an active window, please tell it, as it would also solve the issue we worked around with the QQuickWebViewExperimental::setRenderToOffscreenBuffer private API. Maybe it's also an option to just show the window, as it is recommended to run tests in xvfb anyway, but I guess people would not like it, and also xvfb is only available on Linux.

Ahh, that explains it, well spotted!

Then the solution is to do both:


void PlatformWebView::resizeTo(unsigned width, unsigned height)
{
    m_window->resize(width, height);
    // If we do not have a platform window we will never get the necessary
    // resize event, so simulate it in that case to make sure the quickview is
    // resized to what the layout test expects.
    if (!m_window->handle())
        QWindowSystemInterface::handleGeometryChange(m_window, m_window->geometry());
}

[Balazs Kelemen]

Reopening to attach new patch.

[Balazs Kelemen]

Created attachment 128458 [details]
Patch

Reintroduce the qml binding with Simon's workaround.

[Balazs Kelemen]

Comment on attachment 128458 [details]
Patch

Clearing flags on attachment: 128458

Committed r108626: <http://trac.webkit.org/changeset/108626>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

(In reply to comment #22)
> All reviewed patches have been landed.  Closing bug.

Somehow this did not work on the bot - altough it works for me locally - so we had to revert the patch in http://trac.webkit.org/changeset/108635. I'm going to take a look in it in that env.

[Balazs Kelemen]

(In reply to comment #23)
> (In reply to comment #22)
> > All reviewed patches have been landed.  Closing bug.
> 
> Somehow this did not work on the bot - altough it works for me locally - so we had to revert the patch in http://trac.webkit.org/changeset/108635. I'm going to take a look in it in that env.

I have realized that I can reproduce the failure locally - just need to run a normal (800x600) and an svg test one after the other and the svg will render in 800x600. I have tried removing the if from
if (!m_window->handle())
    QWindowSystemInterface::handleGeometryChange(m_window, m_window->geometry());

and also replacing the call with handleSynchronousGeometryChange, but none of these helps.

[Balazs Kelemen]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > All reviewed patches have been landed.  Closing bug.
> > 
> > Somehow this did not work on the bot - altough it works for me locally - so we had to revert the patch in http://trac.webkit.org/changeset/108635. I'm going to take a look in it in that env.
> 
> I have realized that I can reproduce the failure locally - just need to run a normal (800x600) and an svg test one after the other and the svg will render in 800x600. I have tried removing the if from
> if (!m_window->handle())
>     QWindowSystemInterface::handleGeometryChange(m_window, m_window->geometry());
> 
> and also replacing the call with handleSynchronousGeometryChange, but none of these helps.

The window interface check's wether the passed in geometry and the window's geometry are differing, so we need to do it a bit differently. I'm going to upload a new patch.

[Balazs Kelemen]

Reopening to attach new patch.

[Balazs Kelemen]

Created attachment 129065 [details]
Patch

[Simon Hausmann]

Comment on attachment 129065 [details]
Patch

Thanks :)

[Balazs Kelemen]

Comment on attachment 129065 [details]
Patch

Clearing flags on attachment: 129065

Committed r109232: <http://trac.webkit.org/changeset/109232>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.