Bug 131108 – [EFL] Should flush and render layers after the root layer is changed.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

When the root layer is changed, we should repaint the webview by calling syncLayers().

[Hyowon Kim]

Created attachment 228399 [details]
Patch

[Ryuan Choi]

Comment on attachment 228399 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:7
> +

Maybe we need some description why?

We discussed about regression. Is this patch for that?

> Source/WebKit/efl/ChangeLog:11
> +        (WebCore::AcceleratedCompositingContext::setRootGraphicsLayer): Deleted.

This comment is ambiguous.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:187
> +    m_syncTimer.startOneShot(0);

Don't we need to check wehther m_syncTimer is active?

[Hyowon Kim]

(In reply to comment #2)
> (From update of attachment 228399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228399&action=review
> 
> > Source/WebKit/efl/ChangeLog:7
> > +
> 
> Maybe we need some description why?
> 
> We discussed about regression. Is this patch for that?

Yes, I'll add some more description.

> > Source/WebKit/efl/ChangeLog:11
> > +        (WebCore::AcceleratedCompositingContext::setRootGraphicsLayer): Deleted.
> 
> This comment is ambiguous.

It's automatically generated comment.
I'll replace it with "Move implementation from header file to source file".

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:187
> > +    m_syncTimer.startOneShot(0);
> 
> Don't we need to check wehther m_syncTimer is active?

We will set the syncTimer with 0 interval and do flushAndRenderLayers() as soon as possible even though syncTimer is active by css animations.

[Ryuan Choi]

Comment on attachment 228399 [details]
Patch

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

>>> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:187
>>> +    m_syncTimer.startOneShot(0);
>> 
>> Don't we need to check wehther m_syncTimer is active?
> 
> We will set the syncTimer with 0 interval and do flushAndRenderLayers() as soon as possible even though syncTimer is active by css animations.

Well, if some javascript changes the condition of it to enable/disable sequentially?
I am not sure whether it is impossible.

[Hyowon Kim]

(In reply to comment #4)
> (From update of attachment 228399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228399&action=review
> 
> >>> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:187
> >>> +    m_syncTimer.startOneShot(0);
> >> 
> >> Don't we need to check wehther m_syncTimer is active?
> > 
> > We will set the syncTimer with 0 interval and do flushAndRenderLayers() as soon as possible even though syncTimer is active by css animations.
> 
> Well, if some javascript changes the condition of it to enable/disable sequentially?
> I am not sure whether it is impossible.

I understand your point. startOneShot(0) may be repeatedly called by setRootGraphicsLayer() calls.
I'll add isActive() check.

[Hyowon Kim]

Created attachment 228468 [details]
Patch

[Ryuan Choi]

Comment on attachment 228468 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:10
> +        Empty or non updated webview is shown when there is no update event after the root layer is changed.
> +        Because the syncTimer is not being triggered by root layer change.
> +        We should flush and render layers by starting syncTimer when the root layer is changed.

Patch itself is fine to me.

Nit, comment is bit ambiguous to me. (but I am also bad with english :) )

[Gyuyoung Kim]

Comment on attachment 228468 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:8
> +        Empty or non updated webview is shown when there is no update event after the root layer is changed.

I modify this description a slightly.

"Even when there isn't any update event, empty(or, non-updated webview) webview can be shown after root layer is changed."

[Hyowon Kim]

Created attachment 228476 [details]
Patch

[Ryuan Choi]

(In reply to comment #9)
> Created an attachment (id=228476) [details]
> Patch

Good to me.

[Gyuyoung Kim]

Comment on attachment 228476 [details]
Patch

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

r=me. Please land it after applying my comment.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:185
> +{

Please add ASSERT(rootLayer); It looks rootLayer can be null when user set it as null by using ewk_view_root_graphics_layer_set().

[WebKit Commit Bot]

Comment on attachment 228476 [details]
Patch

Clearing flags on attachment: 228476

Committed r166768: <http://trac.webkit.org/changeset/166768>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.