RESOLVED WONTFIX 111401
[EFL][WK2] WebView::didRenderFrame() does not need to schedule update display. 
https://bugs.webkit.org/show_bug.cgi?id=111401
Summary [EFL][WK2] WebView::didRenderFrame() does not need to schedule update display.
Dongseong Hwang
Reported 2013-03-04 20:30:52 PST
PageClient::didRenderFrame() is called to notify time to draw, so didRenderFrame() is not related to scheduling update display. QtPageClient::didRenderFrame() does not schedule update display also. So this patch makes WebView::didRenderFrame() not schedule update display. On the other hands, PageClient::setViewNeedsDisplay() requires scheduling update display to draw contents next time, so WebView::setViewNeedsDisplay() must schedule update display.
Attachments
Patch (2.36 KB, patch)
2013-03-04 20:57 PST, Dongseong Hwang 		gyuyoung.kim: review-
gyuyoung.kim: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2013-03-04 20:57:12 PST
Created attachment 191392 [details] Patch
Dongseong Hwang
Comment 2 2013-03-04 21:42:22 PST
This isa potential bug that the change of Bug 111405 can reveal.
Mikhail Pozdnyakov
Comment 3 2013-03-04 23:42:21 PST
Comment on attachment 191392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191392&action=review Not sure this change will help anyhow. > Source/WebKit2/UIProcess/efl/WebView.cpp:260 > + m_ewkView->scheduleUpdateDisplay(); this should be done inside client > Source/WebKit2/UIProcess/efl/WebView.cpp:515 > + m_ewkView->pageViewportController()->didRenderFrame(contentsSize, coveredRect); All above is also view client business. Please consider patch at https://bugs.webkit.org/show_bug.cgi?id=110741. Are you sure you don't break view legacy behaviour code path?
Dongseong Hwang
Comment 4 2013-03-05 00:05:11 PST
Comment on attachment 191392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191392&action=review >> Source/WebKit2/UIProcess/efl/WebView.cpp:515 >> + m_ewkView->pageViewportController()->didRenderFrame(contentsSize, coveredRect); > > All above is also view client business. > Please consider patch at https://bugs.webkit.org/show_bug.cgi?id=110741. > Are you sure you don't break view legacy behaviour code path? Thank you for review. My change matches to Qt PageClient behavior. And I'm sure it does not break view legacy behaviour, because didRenderFrame() is called by only one client, which already schedule update display. updateViewport() in below code eventually calls WebView::setViewNeedsDisplay(). void CoordinatedLayerTreeHostProxy::didRenderFrame(const FloatPoint& scrollPosition, const IntSize& contentsSize, const IntRect& coveredRect) { dispatchUpdate(bind(&CoordinatedGraphicsScene::flushLayerChanges, m_scene.get(), scrollPosition)); updateViewport(); #if USE(TILED_BACKING_STORE) m_drawingAreaProxy->page()->didRenderFrame(contentsSize, coveredRect); #endif } I'll update this patch after your Bug 110741 is landed :) btw, I hope to remove didRenderFrame() in all code base. didRenderFrame() makes our code complex..
Dongseong Hwang
Comment 5 2013-03-05 00:16:17 PST
(In reply to comment #4) > btw, I hope to remove didRenderFrame() in all code base. didRenderFrame() makes our code complex.. So I don't want any code to depend on didRenderFrame to enqueue update display event. In conclusion, I want 1. WebView::setViewNeedsDisplay() should enqueue update display event. 2. WebView::didRenderFrame() should not enqueue update display event.
Gyuyoung Kim
Comment 6 2013-07-16 18:41:42 PDT
Comment on attachment 191392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191392&action=review Set r- because this patch needs to be updated according to Bug 110741. >>> Source/WebKit2/UIProcess/efl/WebView.cpp:515 >>> + m_ewkView->pageViewportController()->didRenderFrame(contentsSize, coveredRect); >> >> All above is also view client business. >> Please consider patch at https://bugs.webkit.org/show_bug.cgi?id=110741. >> Are you sure you don't break view legacy behaviour code path? > > Thank you for review. > My change matches to Qt PageClient behavior. > And I'm sure it does not break view legacy behaviour, because didRenderFrame() is called by only one client, which already schedule update display. > updateViewport() in below code eventually calls WebView::setViewNeedsDisplay(). > > void CoordinatedLayerTreeHostProxy::didRenderFrame(const FloatPoint& scrollPosition, const IntSize& contentsSize, const IntRect& coveredRect) > { > dispatchUpdate(bind(&CoordinatedGraphicsScene::flushLayerChanges, m_scene.get(), scrollPosition)); > updateViewport(); > #if USE(TILED_BACKING_STORE) > m_drawingAreaProxy->page()->didRenderFrame(contentsSize, coveredRect); > #endif > } > > I'll update this patch after your Bug 110741 is landed :) > > btw, I hope to remove didRenderFrame() in all code base. didRenderFrame() makes our code complex.. > I'll update this patch after your Bug 110741 is landed :) Bug 110741 was landed. Will you update this patch ?
Michael Catanzaro
Comment 7 2017-03-11 10:37:31 PST
Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.
Note You need to log in before you can comment on or make changes to this bug.