[EFL][WK2] WebView::didRenderFrame() does not need to schedule update display.

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 191392 [details]
Patch

[Dongseong Hwang]

This isa  potential bug that the change of Bug 111405 can reveal.

[Mikhail Pozdnyakov]

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 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]

(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 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]

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.