[webkit-reviews] review denied: [Bug 111401] [EFL][WK2] WebView::didRenderFrame() does not need to schedule update display. : [Attachment 191392] Patch

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied Dongseong Hwang
<dongseong.hwang at intel.com>'s request for review:
Bug 111401: [EFL][WK2] WebView::didRenderFrame() does not need to schedule
update display.
https://bugs.webkit.org/show_bug.cgi?id=111401

Attachment 191392: Patch
https://bugs.webkit.org/attachment.cgi?id=191392&action=review

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
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 ?