Bug 151987 – [CoordinatedGraphics] layerTreeHost always exist in CoordinatedDrawingArea

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

Clean up the unnecessary check routine and dead code in CoordinatedDrawingArea.
CoordinatedGraphics using UI side compositor always forces accelerated compositing.

[Ryuan Choi]

Created attachment 266874 [details]
Patch

[Gwang Yoon Hwang]

Comment on attachment 266874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266874&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedDrawingAreaProxy.cpp:58
>      , m_discardBackingStoreTimer(RunLoop::current(), this, &CoordinatedDrawingAreaProxy::discardBackingStore)

Because we are not going into the non-accelerated code path anymore, you can remove all codes related with backing store states and updates.

[Ryuan Choi]

Comment on attachment 266874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266874&action=review

>> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedDrawingAreaProxy.cpp:58
>>      , m_discardBackingStoreTimer(RunLoop::current(), this, &CoordinatedDrawingAreaProxy::discardBackingStore)
> 
> Because we are not going into the non-accelerated code path anymore, you can remove all codes related with backing store states and updates.

I agree. but I am little bit worried that patch become difficult to be reviewed.
How about separating it from this patch?

If it doesn't matter, I will update the patch with more cleanup.

[Gwang Yoon Hwang]

I prefer to remove all codes related with backing store updates. 
And I think it would be easier to review :)

(In reply to comment #3)
> Comment on attachment 266874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266874&action=review
> 
> >> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedDrawingAreaProxy.cpp:58
> >>      , m_discardBackingStoreTimer(RunLoop::current(), this, &CoordinatedDrawingAreaProxy::discardBackingStore)
> > 
> > Because we are not going into the non-accelerated code path anymore, you can remove all codes related with backing store states and updates.
> 
> I agree. but I am little bit worried that patch become difficult to be
> reviewed.
> How about separating it from this patch?
> 
> If it doesn't matter, I will update the patch with more cleanup.

[Ryuan Choi]

Created attachment 266888 [details]
Patch

[Ryuan Choi]

Created attachment 266895 [details]
Patch

[Gwang Yoon Hwang]

Comment on attachment 266895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266895&action=review

It looks good to me except some nitpics. nice work.

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedDrawingAreaProxy.h:69
> +    virtual void setBackingStoreIsDiscardable(bool) { }

You can delete this line because it is already defined at DrawingAreaProxy

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:85
> +    ASSERT(m_dirtyRegion.isEmpty());

I think you can remove m_dirtyRegion as well because we are not going to update this anymore after this patch.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:94
> +    ASSERT(m_dirtyRegion.isEmpty());

ditto.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedDrawingArea.cpp:104
> +    ASSERT(m_scrollOffset.isEmpty());

We can remove m_scrollRect and m_scrollOffset as well.

[Ryuan Choi]

Committed r193811: <http://trac.webkit.org/changeset/193811>

[Ryuan Choi]

Comment on attachment 266895 [details]
Patch

Clearing flags after landed manually with following yoon's comments.