Bug 90394

This has been discussed in person with several people @ QtCS, but if anybody has concerns/opinions to the contrary feel free to raise them :)

Started working on this. One thing makes me worry: is it ok to actually switch wtr to QRWV? Shouldn't we want to keep the QQWV based implementation as well?

(In reply to comment #2) > Started working on this. One thing makes me worry: is it ok to actually switch wtr to QRWV? Shouldn't we want to keep the QQWV based implementation as well? Maybe in the beginning we should keep both implementations, and switch when it is stable/makes-sense?

Created attachment 150799 [details] wip patch for discussion This is not ready. Dumping basically works, sometimes view dimensions doesn't match which can be some resizing issue. The bigger issue of mine is the pixel snapshot, which is always empty currently, and I feel like I need some help to fix this.

(In reply to comment #4) > The bigger issue of mine is the pixel snapshot, which is always empty currently, and I feel like I need some help to fix this. Probably when createSnapshot() is called you want to make the context current, paint, and then read the pixels.

(In reply to comment #5) > (In reply to comment #4) > > The bigger issue of mine is the pixel snapshot, which is always empty currently, and I feel like I need some help to fix this. > > Probably when createSnapshot() is called you want to make the context current, paint, and then read the pixels. It doesn't help. This was what I did first, but it did not work, so I tried to do it like tst_qrawwebview. tst_qrawwebview first sets up the context, loads the page and meanwhile do paints in viewNeedsDisplay, and after load finished dumps the image. This is really tricky in fact, so let's take a look what it does: it has a loadfinished callback for turning on a bool: void WebView::finishLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void *clientInfo) { WebView* obj = static_cast<WebView*>(const_cast<void*>(clientInfo)); obj->m_frameLoaded = true; } and in the _next_ viewNeedsDisplay, it emits a signal: virtual void viewNeedsDisplay(const QRect&) { m_webView->paint(QMatrix4x4(), 1, 0); if (m_frameLoaded) emit loaded(); } and it dumps the image just after this signal has emitted. So, I can grab the image if I do the same trick in createSnapshot, but I don't think this is correct behavior. The question is that why don't we have anything to paint when createSnapshot is called? For the Mac platform createSnapshot simply captures the window. I also tried to actually show the window but it did not change anything.

Ok, I looked at this more carefully. I believe that the correct state machine should be as followed: 1. ignore viewNeedsDisplay events. 2. send a new NothingToRender message from LayerTreeCoordinator when m_shouldSyncFrame is false, see LayerTreeCoordinator::performScheduledLayerFlush(). This would ensure that we only grab the pixels after all the async rendering steps are done. 3. Grabbing the pixels should be performed at the first NothingToRender message that comes up after the first "Done" event received from the injected bundle. This sequence of event should ensure that we grab the pixels when all rendering of the loaded frame is done, including the async rendering of tiles content.

(In reply to comment #7) > 2. send a new NothingToRender message from LayerTreeCoordinator when m_shouldSyncFrame is false, see LayerTreeCoordinator::performScheduledLayerFlush(). This would ensure that we only grab the pixels after all the async rendering steps are done. I think that you might have the same problem in some cases where you won't know if you should wait for a NothingToRender message or not before grabbing. For example if the done message was triggered in a timer where no DOM modification was done. So basically what you need to know is: "Should I wait to grab? Is there something on its way that might have been triggered right before that Done message?" Since we can rely on every message being received in the same order they were sent, you could send a slightly different BeginRenderFrame message each time the m_layerFlushTimer is started (meaning that a modification to the document is pending for rendering in the web process). And when the done message is received, wait until DidRenderFrame was received if we're between a BeginRenderFrame and a DidRenderFrame message, or else grab immediately. Another way could be to hold the Done message in the web process until DidRenderFrame is sent if the page needs layout when triggered, but that would be quite smelly and would violate a couple of layers. I agree that we shouldn't rely on viewNeedsDisplay, we should rely on DidRenderFrame messages instead since grabbing will/should flush the update queue anyway.

(In reply to comment #8) > (In reply to comment #7) > > 2. send a new NothingToRender message from LayerTreeCoordinator when m_shouldSyncFrame is false, see LayerTreeCoordinator::performScheduledLayerFlush(). This would ensure that we only grab the pixels after all the async rendering steps are done. > > I think that you might have the same problem in some cases where you won't know if you should wait for a NothingToRender message or not before grabbing. For example if the done message was triggered in a timer where no DOM modification was done. > > So basically what you need to know is: "Should I wait to grab? Is there something on its way that might have been triggered right before that Done message?" > > Since we can rely on every message being received in the same order they were sent, you could send a slightly different BeginRenderFrame message each time the m_layerFlushTimer is started (meaning that a modification to the document is pending for rendering in the web process). > And when the done message is received, wait until DidRenderFrame was received if we're between a BeginRenderFrame and a DidRenderFrame message, or else grab immediately. > > Another way could be to hold the Done message in the web process until DidRenderFrame is sent if the page needs layout when triggered, but that would be quite smelly and would violate a couple of layers. > > I agree that we shouldn't rely on viewNeedsDisplay, we should rely on DidRenderFrame messages instead since grabbing will/should flush the update queue anyway. This can still create a problem, if the frame loading creates a DOM change that the UI process hasn't recognized yet. I need to experiment with this state sequence a bit...

Created attachment 151128 [details] WIP patch, including supporting fixes for QRawWebView OK, I took a different route to this - using the existing ForceRepaint API. Waiting for DidRenderFrame and messing with LayerTreeCoordinator has proven unstable - also it's unnecessary. The main bug comes from the fact that LayerTreeCoordinator::forceRepaint is not implemented correctly. The patch is a combination of fixes to this and the blocking bug. It works with new-run-webkit-tests, but it still needs some work.

Can we mark this as a duplicate of bug 90641?

(In reply to comment #11) > Can we mark this as a duplicate of bug 90641? No, 90641 is just the enabling stuff in QRawWebView. We still need to modify WTR.

(In reply to comment #12) > (In reply to comment #11) > > Can we mark this as a duplicate of bug 90641? > > No, 90641 is just the enabling stuff in QRawWebView. We still need to modify WTR. Ok, than I hope you don't mind if I upload the patch ;)

Created attachment 151905 [details] Patch

Comment on attachment 151905 [details] Patch We should wait for the font rebase and double check the pixel results.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Can we mark this as a duplicate of bug 90641? > > > > No, 90641 is just the enabling stuff in QRawWebView. We still need to modify WTR. > > Ok, than I hope you don't mind if I upload the patch ;) Of course not :) This is your work, I've just been trying to help with the synchronized painting.

I have rebaselined the compositing directory to the point that now we don't have any unexpectedly faling pixel result. With this patch we have 38 fail. Some of them just needs rebaseline but the most of them is real failure. This is known to be the affect of actually testing the compositing path for real. We also have 10 passing tests that are expected to fail. It's important to note that the pixel snapshot mechanism does not work with xvfb, it needs to run with the real X server.

(In reply to comment #17) > I have rebaselined the compositing directory to the point that now we don't have any unexpectedly faling pixel result. With this patch we have 38 fail. Some of them just needs rebaseline but the most of them is real failure. This is known to be the affect of actually testing the compositing path for real. We also have 10 passing tests that are expected to fail. It's important to note that the pixel snapshot mechanism does not work with xvfb, it needs to run with the real X server. How far are we from moving forward with this? Simon/Jocelyn, would you like to review? (I can't since I'm a co-author). Once the WTR pixel test changes are in and the rebaseline is done, can we make this happen?

Comment on attachment 151905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151905&action=review A few small comments > Source/WebKit2/Shared/API/c/qt/WKImageQt.cpp:40 > +WKImageRef WKImageCreateFromQImage(QImage image) I think the QImage parameter should be passed a const reference, to avoid atomic refcount operations. > Source/WebKit2/Shared/API/c/qt/WKImageQt.cpp:49 > + QPainter* painter = graphicsContext->platformContext(); > + painter->drawImage(QPoint(0, 0), image); I hope that this isn't going to accidentally "alter" the data. Alternatively I suppose you could memcopy from the incoming QImage to the webImage->createQImage().bits()? > Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:129 > + // FIXME: should use a transformation matrix fot mirroring. fot -> for

(In reply to comment #18) > (In reply to comment #17) > > I have rebaselined the compositing directory to the point that now we don't have any unexpectedly faling pixel result. With this patch we have 38 fail. Some of them just needs rebaseline but the most of them is real failure. This is known to be the affect of actually testing the compositing path for real. We also have 10 passing tests that are expected to fail. It's important to note that the pixel snapshot mechanism does not work with xvfb, it needs to run with the real X server. > > How far are we from moving forward with this? Simon/Jocelyn, would you like to review? (I can't since I'm a co-author). > Once the WTR pixel test changes are in and the rebaseline is done, can we make this happen? I think the patch looks good. I'm a bit concerned that this removes valuable code coverage of QQuickWebView, in times when the Qt 5.0 release is nearing. At the same time it probably isn't the best time to change QQuickWebView now to use QRawWebView. It seems that in an ideal world WTR would use QRawWebView - I see benefits such as requiring less workarounds in WTR. QQuickWebView would be based on QRawWebView and we sould actually use TestWebKitAPI to test the C API.

(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #17) > > > I have rebaselined the compositing directory to the point that now we don't have any unexpectedly faling pixel result. With this patch we have 38 fail. Some of them just needs rebaseline but the most of them is real failure. This is known to be the affect of actually testing the compositing path for real. We also have 10 passing tests that are expected to fail. It's important to note that the pixel snapshot mechanism does not work with xvfb, it needs to run with the real X server. > > > > How far are we from moving forward with this? Simon/Jocelyn, would you like to review? (I can't since I'm a co-author). > > Once the WTR pixel test changes are in and the rebaseline is done, can we make this happen? > > I think the patch looks good. > > I'm a bit concerned that this removes valuable code coverage of QQuickWebView, in times when the Qt 5.0 release is nearing. At the same time it probably isn't the best time to change QQuickWebView now to use QRawWebView. Yep, this a problem, although I don't think layout tests add much coverage for QQuickWebView since it doesn't test the UI stuff. > > It seems that in an ideal world WTR would use QRawWebView - I see benefits such as requiring less workarounds in WTR. QQuickWebView would be based on QRawWebView and we sould actually use TestWebKitAPI to test the C API. QQuickWebView would be based on QRawWebView? That's totally new to me. I was thinking that QRawWebView is an internal api for non-qml use cases. Could you describe the benefit of this? I think we should have a consensus how to move forward. In any case, a patch with the rebaselines is needed, so I'm going to remove the patch from the review queue.

(In reply to comment #21) > > It seems that in an ideal world WTR would use QRawWebView - I see benefits such as requiring less workarounds in WTR. QQuickWebView would be based on QRawWebView and we sould actually use TestWebKitAPI to test the C API. > > QQuickWebView would be based on QRawWebView? That's totally new to me. I was thinking that QRawWebView is an internal api for non-qml use cases. Could you describe the benefit of this? It would remain internal for that purpose. Anyway, it is a bit too early to make a decision. But the more I think about it the more this patch makes sense anyway.

Comment on attachment 151905 [details] Patch r=me with the typo fix and the const reference fix.

(In reply to comment #23) > (From update of attachment 151905 [details]) > r=me with the typo fix and the const reference fix. Thanks for the review, but still we should do the necessary rebaseline as well. I would rather go for another round, so I'm going to remove the r+ now :)

(In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 151905 [details] [details]) > > r=me with the typo fix and the const reference fix. > > Thanks for the review, but still we should do the necessary rebaseline as well. I would rather go for another round, so I'm going to remove the r+ now :) You can simply not land for now :)

(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #18) > > > (In reply to comment #17) > > > > I have rebaselined the compositing directory to the point that now we don't have any unexpectedly faling pixel result. With this patch we have 38 fail. Some of them just needs rebaseline but the most of them is real failure. This is known to be the affect of actually testing the compositing path for real. We also have 10 passing tests that are expected to fail. It's important to note that the pixel snapshot mechanism does not work with xvfb, it needs to run with the real X server. > > > > > > How far are we from moving forward with this? Simon/Jocelyn, would you like to review? (I can't since I'm a co-author). > > > Once the WTR pixel test changes are in and the rebaseline is done, can we make this happen? > > > > I think the patch looks good. > > > > I'm a bit concerned that this removes valuable code coverage of QQuickWebView, in times when the Qt 5.0 release is nearing. At the same time it probably isn't the best time to change QQuickWebView now to use QRawWebView. > > Yep, this a problem, although I don't think layout tests add much coverage for QQuickWebView since it doesn't test the UI stuff. > > > > > It seems that in an ideal world WTR would use QRawWebView - I see benefits such as requiring less workarounds in WTR. QQuickWebView would be based on QRawWebView and we sould actually use TestWebKitAPI to test the C API. > > QQuickWebView would be based on QRawWebView? That's totally new to me. I was thinking that QRawWebView is an internal api for non-qml use cases. Could you describe the benefit of this? Separation of concerns. Right now things like QtViewportHandler and others mix cross-platform webkit functionality together with QtQuick specific code paths. It's hard to tell sometimes whether the problem is in QtQuick, in the glue layer, or in WebKit. Also, it would make QRawWebView and QQuickwebView more like a stack and less like branches. > I think we should have a consensus how to move forward. In any case, a patch with the rebaselines is needed, so I'm going to remove the patch from the review queue. If someone wants to prototype putting QQuickWebView on top of QRawWebView it would be a good step forward, probably after Qt5webkit branches. I'm OK with the status quo, once we can move forward with having some sort of GL-based pixel tests :)

I'm not sure how to proceed with this. There is a bunch of pixel failures currently, and according to my results there will be much more (~1000-1500) with the patch. It's not possible to do all the rebaseline in one patch. Also these failures can mean real bugs that we had anyway, which we could not detect so far. Also it's possible that the sync mechanism we do for grabbing the pixels is still not perfect. Maybe someone with more graphics knowledge should look at the results as well. If the problem is not the sync mechanism than I think we should go forward and land the patch, and go with the reabaselines incrementally.

Seems like I have found a real issue with the mechanism. We have trouble with repaint tests like svg/text/tspan-dynamic-positioning.svg or fast/repaint/background-generated.html. (There is a lot more of course.) The change in the runRepaintTest() function does not apply to the snapshot.

Seems like we do the repaint (in result of the dom manipulation) too late. This is strange because we call WebPage::snapshotInViewCoordinates even with the patch which is forcing a style update, so I would expect that when we do the forced repaint for the snapshot it should contain the effect of the dom change.

The repaint bug has been fixed so I think we can go forward with this. I am going to update the patch and check the tests. Doing all the rebaseline seems to be too difficult (at least for me), as there are a huge number of failures mostly because of port and wk1/2 differences but also there are some real bugs. I think it's more reasonable to land this patch first and do the rebaseline incrementally later.

Unfortunately I still have some problems with the tests so I'm trying to fix it first.

I was wrong, some repaint tests are still failing. The tests was fixed by http://trac.webkit.org/changeset/124603 but this patch made trouble and had to be rolled out. The last patch that changed on the syncronisation was http://trac.webkit.org/changeset/125034. Now the problem is that we don't get called back after WKPageForceRepaint in some tests at all. It seems like that before the second patch, we was called back but the actual repaint did not happened at the web process side.

Status: I'm still working on this. Seems like we should not use QEventLoop because it makes the event loop messed up and we lose a didrenderframe message. For that reason I modified WTR to explicitly wait for the callback, it solved the syncronisation. Unfortunately there are still tons of failures, even some text failures so I'm debugging them further.

The GL rendering path has some problems that made tests start to produce blackness flakily. For that reason I was thinking about implementing this with the software path upon QQuickWebView, as an intermediate step. Tested locally, it is quite stable. Filed bug 95992 for that work.

*** This bug has been marked as a duplicate of bug 95992 ***