Bug 151987 - [CoordinatedGraphics] layerTreeHost always exist in CoordinatedDrawingArea
Summary: [CoordinatedGraphics] layerTreeHost always exist in CoordinatedDrawingArea
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-08 02:48 PST by Ryuan Choi
Modified: 2015-12-09 01:43 PST (History)
3 users (show)

See Also:


Attachments
Patch (25.62 KB, patch)
2015-12-08 02:51 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (35.04 KB, patch)
2015-12-08 08:43 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (36.22 KB, patch)
2015-12-08 09:14 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2015-12-08 02:48:25 PST
Clean up the unnecessary check routine and dead code in CoordinatedDrawingArea.
CoordinatedGraphics using UI side compositor always forces accelerated compositing.
Comment 1 Ryuan Choi 2015-12-08 02:51:47 PST
Created attachment 266874 [details]
Patch
Comment 2 Gwang Yoon Hwang 2015-12-08 05:41:23 PST
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.
Comment 3 Ryuan Choi 2015-12-08 06:50:06 PST
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.
Comment 4 Gwang Yoon Hwang 2015-12-08 07:05:55 PST
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.
Comment 5 Ryuan Choi 2015-12-08 08:43:28 PST
Created attachment 266888 [details]
Patch
Comment 6 Ryuan Choi 2015-12-08 09:14:25 PST
Created attachment 266895 [details]
Patch
Comment 7 Gwang Yoon Hwang 2015-12-09 01:33:51 PST
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.
Comment 8 Ryuan Choi 2015-12-09 01:43:04 PST
Committed r193811: <http://trac.webkit.org/changeset/193811>
Comment 9 Ryuan Choi 2015-12-09 01:43:49 PST
Comment on attachment 266895 [details]
Patch

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