111401 2013-03-04 20:30:52 -0800 [EFL][WK2] WebView::didRenderFrame() does not need to schedule update display. 2017-03-11 10:37:31 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 110741 110066 111405 1 dongseong.hwang dongseong.hwang gyuyoung.kim kenneth lucas.de.marchi mcatanzaro mikhail.pozdnyakov rakuco sergio webkit.review.bot oldest_to_newest 847223 0 dongseong.hwang 2013-03-04 20:30:52 -0800 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. 847232 1 191392 dongseong.hwang 2013-03-04 20:57:12 -0800 Created attachment 191392 Patch 847255 2 dongseong.hwang 2013-03-04 21:42:22 -0800 This isa potential bug that the change of Bug 111405 can reveal. 847336 3 191392 mikhail.pozdnyakov 2013-03-04 23:42:21 -0800 Comment on attachment 191392 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? 847358 4 191392 dongseong.hwang 2013-03-05 00:05:11 -0800 Comment on attachment 191392 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.. 847365 5 dongseong.hwang 2013-03-05 00:16:17 -0800 (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. 909101 6 191392 gyuyoung.kim 2013-07-16 18:41:42 -0700 Comment on attachment 191392 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 ? 1286342 7 mcatanzaro 2017-03-11 10:37:31 -0800 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. 191392 2013-03-04 20:57:12 -0800 2013-07-16 18:41:42 -0700 Patch bug-111401-20130305135246.patch text/plain 2417 dongseong.hwang U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ0NzEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMmJlMTY5MjMzMzc0ZDc4 NzI5OTcxYWQ1ZDdmNzg2ODlhYmNjODlhMy4uMWE2ZmNkNGFmN2Y1MjkzM2Y3NjE3OTM3ZDZlOTY2 MWQ2MGI0MGQ4NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEzLTAzLTA0ICBIdWFu ZyBEb25nc3VuZyAgPGx1eHRlbGxhQGNvbXBhbnkxMDAubmV0PgorCisgICAgICAgIFtFRkxdW1dL Ml0gV2ViVmlldzo6ZGlkUmVuZGVyRnJhbWUoKSBkb2VzIG5vdCBuZWVkIHRvIHNjaGVkdWxlIHVw ZGF0ZSBkaXNwbGF5LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTExNDAxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgUGFnZUNsaWVudDo6ZGlkUmVuZGVyRnJhbWUoKSBpcyBjYWxsZWQgdG8gbm90aWZ5IHRp bWUgdG8gZHJhdywgc28gZGlkUmVuZGVyRnJhbWUoKQorICAgICAgICBpcyBub3QgcmVsYXRlZCB0 byBzY2hlZHVsaW5nIHVwZGF0ZSBkaXNwbGF5LiBRdFBhZ2VDbGllbnQ6OmRpZFJlbmRlckZyYW1l KCkgZG9lcworICAgICAgICBub3Qgc2NoZWR1bGUgdXBkYXRlIGRpc3BsYXkgYWxzby4gU28gdGhp cyBwYXRjaCBtYWtlcyBXZWJWaWV3OjpkaWRSZW5kZXJGcmFtZSgpCisgICAgICAgIG5vdCBzY2hl ZHVsZSB1cGRhdGUgZGlzcGxheS4KKworICAgICAgICBPbiB0aGUgb3RoZXIgaGFuZHMsIFBhZ2VD bGllbnQ6OnNldFZpZXdOZWVkc0Rpc3BsYXkoKSByZXF1aXJlcyBzY2hlZHVsaW5nIHVwZGF0ZQor ICAgICAgICBkaXNwbGF5IHRvIGRyYXcgY29udGVudHMgbmV4dCB0aW1lLCBzbyBXZWJWaWV3Ojpz ZXRWaWV3TmVlZHNEaXNwbGF5KCkgbXVzdAorICAgICAgICBzY2hlZHVsZSB1cGRhdGUgZGlzcGxh eS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9lZmwvV2ViVmlldy5jcHA6CisgICAgICAgIChXZWJL aXQ6OldlYlZpZXc6OnNldFZpZXdOZWVkc0Rpc3BsYXkpOgorICAgICAgICAoV2ViS2l0OjpXZWJW aWV3OjpkaWRSZW5kZXJGcmFtZSk6CisKIDIwMTMtMDItMjYgIEh1YW5nIERvbmdzdW5nICA8bHV4 dGVsbGFAY29tcGFueTEwMC5uZXQ+CiAKICAgICAgICAgW0VGTF1bUXRdIENvb3JkaW5hdGVkIEdy YXBoaWNzOiBzY3JvbGwgYW5pbWF0aW9uIG1ha2VzIGZpeGVkIGVsZW1lbnQgc2hha2UuCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZWZsL1dlYlZpZXcuY3BwIGIvU291cmNl L1dlYktpdDIvVUlQcm9jZXNzL2VmbC9XZWJWaWV3LmNwcAppbmRleCBiNWQ5MTJhMDdhZDUwOTJj OGM3NjVhNmVmODFhNWM4M2Q0OGZhYmY4Li45ZTE5MDNkNmUxMDQxMjFkN2FjZmVjNzE3NGMwNjA3 ZjJmODJhYmRlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZWZsL1dlYlZp ZXcuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9lZmwvV2ViVmlldy5jcHAKQEAg LTI1Nyw2ICsyNTcsNyBAQCBQYXNzT3duUHRyPERyYXdpbmdBcmVhUHJveHk+IFdlYlZpZXc6OmNy ZWF0ZURyYXdpbmdBcmVhUHJveHkoKQogdm9pZCBXZWJWaWV3OjpzZXRWaWV3TmVlZHNEaXNwbGF5 KGNvbnN0IFdlYkNvcmU6OkludFJlY3QmIGFyZWEpCiB7CiAgICAgbV9jbGllbnQudmlld05lZWRz RGlzcGxheSh0aGlzLCBhcmVhKTsKKyAgICBtX2V3a1ZpZXctPnNjaGVkdWxlVXBkYXRlRGlzcGxh eSgpOwogfQogCiB2b2lkIFdlYlZpZXc6OmRpc3BsYXlWaWV3KCkKQEAgLTUwOSwxMSArNTEwLDkg QEAgdm9pZCBXZWJWaWV3OjpwYWdlRGlkUmVxdWVzdFNjcm9sbChjb25zdCBJbnRQb2ludCYgcG9z aXRpb24pCiAKIHZvaWQgV2ViVmlldzo6ZGlkUmVuZGVyRnJhbWUoY29uc3QgV2ViQ29yZTo6SW50 U2l6ZSYgY29udGVudHNTaXplLCBjb25zdCBXZWJDb3JlOjpJbnRSZWN0JiBjb3ZlcmVkUmVjdCkK IHsKLSAgICBpZiAobV9wYWdlLT51c2VGaXhlZExheW91dCgpKSB7Ci0gICAgICAgIG1fZXdrVmll dy0+cGFnZVZpZXdwb3J0Q29udHJvbGxlcigpLT5kaWRSZW5kZXJGcmFtZShjb250ZW50c1NpemUs IGNvdmVyZWRSZWN0KTsKKyAgICBpZiAoIW1fcGFnZS0+dXNlRml4ZWRMYXlvdXQoKSkKICAgICAg ICAgcmV0dXJuOwotICAgIH0KLSAgICBtX2V3a1ZpZXctPnNjaGVkdWxlVXBkYXRlRGlzcGxheSgp OworICAgIG1fZXdrVmlldy0+cGFnZVZpZXdwb3J0Q29udHJvbGxlcigpLT5kaWRSZW5kZXJGcmFt ZShjb250ZW50c1NpemUsIGNvdmVyZWRSZWN0KTsKIH0KIAogdm9pZCBXZWJWaWV3OjpwYWdlVHJh bnNpdGlvblZpZXdwb3J0UmVhZHkoKQo=