.
Created attachment 126068 [details] patch
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
Created attachment 126078 [details] fixed patch Thanks for your comment. I fixed it.
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)
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.
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
Created attachment 126285 [details] patch Thanks you for your help and your patience!
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 :)
Created attachment 127193 [details] Patch
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.
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.
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.
Comment on attachment 127193 [details] Patch Clearing flags on attachment: 127193 Committed r107868: <http://trac.webkit.org/changeset/107868>
All reviewed patches have been landed. Closing bug.
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.
s/Linux/non-Linux
(In reply to comment #16) > s/Linux/non-Linux No, the sentence was correct, please ignore this comment. Sigh....
(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()); }
Reopening to attach new patch.
Created attachment 128458 [details] Patch Reintroduce the qml binding with Simon's workaround.
Comment on attachment 128458 [details] Patch Clearing flags on attachment: 128458 Committed r108626: <http://trac.webkit.org/changeset/108626>
(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.
(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.
(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.
Created attachment 129065 [details] Patch
Comment on attachment 129065 [details] Patch Thanks :)
Comment on attachment 129065 [details] Patch Clearing flags on attachment: 129065 Committed r109232: <http://trac.webkit.org/changeset/109232>