Bug 90641

Created attachment 150996 [details] Patch

Comment on attachment 150996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:284 > +} > + > + one too many newlines > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:407 > + m_client->viewRenderedFrame(); view-rendered frame? maybe viewDidRenderFrame is more descriptive > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:57 > + virtual void viewNeedsDisplay(const QRect&) { } > + virtual void viewRequestedScroll(const QPoint&) { } > + virtual void viewProcessCrashed() { } > + virtual void viewRenderedFrame() { } > + virtual void viewProcessRelaunched() { } > + virtual void viewContentSizeChanged(const QSize&) { } Ah I see you are using it [view] as a namespace, hmm > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:128 > + QCryptographicHash hash(QCryptographicHash::Md5); > + for (unsigned row = 0; row < image.height(); ++row) > + hash.addData(reinterpret_cast<const char*>(image.constScanLine(row)), image.bytesPerLine()); Good idea, but what about fuzzyness? These have to be strictly the same?

Comment on attachment 150996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p_p.h:34 > +class QRawWebViewPrivate : public WebKit::PageClient, public virtual WebKit::LayerTreeCoordinatorProxy::Observer { Why to use virtual inheritance?

(In reply to comment #3) > (From update of attachment 150996 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > > > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p_p.h:34 > > +class QRawWebViewPrivate : public WebKit::PageClient, public virtual WebKit::LayerTreeCoordinatorProxy::Observer { > > Why to use virtual inheritance? Old habit when i use multiple inheritance... doesn't really matter.

(In reply to comment #2) > > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:128 > > + QCryptographicHash hash(QCryptographicHash::Md5); > > + for (unsigned row = 0; row < image.height(); ++row) > > + hash.addData(reinterpret_cast<const char*>(image.constScanLine(row)), image.bytesPerLine()); > > Good idea, but what about fuzzyness? These have to be strictly the same? I think maybe I'll leave this to another patch. We need a better way to test this.

Comment on attachment 150996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > Source/WebKit2/ChangeLog:11 > + Allow QRawWebViewClient to receive a notification when rendering is done (DidRenderFrame), > + and allow QRawWebView to send a message to the web process to render a new frame > + programmatically, by exposing LayerTreeCoordinator::scheduleLayerFlush via a WebKit2 > + message. You might have issues that scheduleLayerFlush will not always return you a DidRenderFrame, i.e. if any of the early returns are hit in LayerTreeCoordinator::performScheduledLayerFlush. If m_isSuspended, m_waitingForUIProcess are true, or if there was nothing dirty and m_shouldSyncFrame wasn't set to true. It feels that if notifyDone is called inside a timer where no document modification was done, you could still wait infinitely while grabbing. > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:119 > void WebView::finishLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void *clientInfo) > { `

Comment on attachment 150996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:100 > + virtual void viewRenderedFrame() > + { > if (m_frameLoaded) > emit loaded(); > } Shouldn't we actually paint here?

(In reply to comment #6) > (From update of attachment 150996 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > > > Source/WebKit2/ChangeLog:11 > > + Allow QRawWebViewClient to receive a notification when rendering is done (DidRenderFrame), > > + and allow QRawWebView to send a message to the web process to render a new frame > > + programmatically, by exposing LayerTreeCoordinator::scheduleLayerFlush via a WebKit2 > > + message. > > You might have issues that scheduleLayerFlush will not always return you a DidRenderFrame, i.e. if any of the early returns are hit in LayerTreeCoordinator::performScheduledLayerFlush. > If m_isSuspended, m_waitingForUIProcess are true, or if there was nothing dirty and m_shouldSyncFrame wasn't set to true. You're right, I need to send a NothingToRender in the case when m_shouldSyncFrame is false. m_waitingForUIProcess is not a problem because another flush would be trigger in the next RenderNextFrame call, and m_isSuspended should not be on during a grab - I think it's safe to wait to the next DidRenderFrame that would happen after the next resume. > It feels that if notifyDone is called inside a timer where no document modification was done, you could still wait infinitely while grabbing. > Right, I still need the NothingToRender event or something similar.

(In reply to comment #7) > (From update of attachment 150996 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review > > > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:100 > > + virtual void viewRenderedFrame() > > + { > > if (m_frameLoaded) > > emit loaded(); > > } > > Shouldn't we actually paint here? No need, since we already paint on viewNeedsDisplay.

Comment on attachment 150996 [details] Patch Clearing flags, working on a new patch.

Just for the record: I tried but could not get reliable pixel results with the patch. I tried it like this: https://gist.github.com/3061148. It was working for a few basic tests when I called wtr from the command line, also with old-run-webkit-tests, but not with new-run-webkit-tests. Even by programatically saving the pixel result to file the results in nrwt was different (empty blackness). I could not find anything about nrwt that could cause this, so I guess the method provided by the patch here is still not very reliable (but I'm still very puzzled about the fact that it works with one test harness and not with another).

Created attachment 151164 [details] Patch

Comment on attachment 151164 [details] Patch There's a problem with this patch, clearing flags for now.

Created attachment 151277 [details] Patch

Comment on attachment 151277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151277&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:178 > scheduleLayerFlush(); > + flushPendingLayerChanges(); Why both? > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:242 > + didSync = didSync || m_shouldSyncFrame; Is it a good idea to mix those two? syncCompositingStateIncludingSubframes can return true even if no modification was made to layers I think, and this would send DidRenderFrame messages unnecessarily. > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:136 > + bool m_isPerformingLayerFlush; Unused.

(In reply to comment #15) > (From update of attachment 151277 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151277&action=review > > > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:178 > > scheduleLayerFlush(); > > + flushPendingLayerChanges(); > > Why both? I'm not sure. This code is copied from GraphicsLayerCA. > > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:242 > > + didSync = didSync || m_shouldSyncFrame; > > Is it a good idea to mix those two? syncCompositingStateIncludingSubframes can return true even if no modification was made to layers I think, and this would send DidRenderFrame messages unnecessarily. You're right. I'll fix this.

Created attachment 151490 [details] Patch

Comment on attachment 151490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151490&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238 > + m_shouldSyncFrame = false; I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer. > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:258 > + m_waitingForUIProcess = true; m_waitingForUIProcess state changes have to be in sync with DidRenderFrame messages, it would be clearer to update it within the same scope.

(In reply to comment #18) > (From update of attachment 151490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151490&action=review > > > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238 > > + m_shouldSyncFrame = false; > > I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer. > We set m_shouldSyncFrame for every tile update. That hasn't changed in this patch, or I don't understand your comment.

(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 151490 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=151490&action=review > > > > > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238 > > > + m_shouldSyncFrame = false; > > > > I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer. > > > We set m_shouldSyncFrame for every tile update. That hasn't changed in this patch, or I don't understand your comment. Oh, I see. There's a bug there.

Created attachment 151516 [details] Patch

Comment on attachment 151516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151516&action=review r=me with one readability issue. > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:255 > + if (!didSync) > + return false; > + > + if (!m_shouldSyncFrame) > + return true; This part hardly makes sense if you just look at the variable names. I think it would read better as: if (!m_shouldSyncFrame) return didSync; And at the end, return didSync; instead of true (or remove the "didSync = didSync || m_shouldSyncFrame;" line and keep the return true). Else it's impossible to see that m_shouldSyncFrame is related to the DidRenderFrame message. You could also rename m_shouldSyncFrame to m_shouldSendDidRenderFrame or something like this.

Created attachment 151724 [details] Patch for landing

Created attachment 151726 [details] Patch for landing

Comment on attachment 151726 [details] Patch for landing Clearing flags on attachment: 151726 Committed r122340: <http://trac.webkit.org/changeset/122340>

All reviewed patches have been landed. Closing bug.