Bug 130072 – [EFL] Revise AcceleratedCompositingEfl implementation.

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

[Hyowon Kim]

I think this would be the last bug to enable accelerated compositing on WK1 efl.

[Hyowon Kim]

Created attachment 226399 [details]
Patch

[Ryuan Choi]

Comment on attachment 226399 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81
> +    ewk_view_mark_for_sync(m_view);

It's quite complex.

What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ?

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:107
> +    return m_rootGraphicsLayer && m_textureMapper;

Does we have situation which one of them is valid ?

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:132
> +        m_syncTimer.startOneShot(1.0 / 60.0);

We'd better to mentione constant value like below
const double frameRate = 60;

[Hyowon Kim]

Created attachment 226463 [details]
Patch

[Hyowon Kim]

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81
> > +    ewk_view_mark_for_sync(m_view);
> 
> It's quite complex.
> 
> What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ?

All sync requests will call ewk_view_mark_for_sync().
And compositingObject will be removed in next patches.

> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:107
> > +    return m_rootGraphicsLayer && m_textureMapper;
> 
> Does we have situation which one of them is valid ?

Yes, m_rootGraphicsLayer could be 0 while m_textureMapper is vaild.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:132
> > +        m_syncTimer.startOneShot(1.0 / 60.0);
> 
> We'd better to mentione constant value like below
> const double frameRate = 60;

Done.

[Hyowon Kim]

Created attachment 226490 [details]
Patch

[Hyowon Kim]

(In reply to comment #4)
> > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81
> > > +    ewk_view_mark_for_sync(m_view);
> > 
> > It's quite complex.
> > 
> > What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ?
> 
> All sync requests will call ewk_view_mark_for_sync().
> And compositingObject will be removed in next patches.
> 

ewkView and compositingObject will be passed to AcceleratedCompositing.
Thank you for good comment.

[Gyuyoung Kim]

Comment on attachment 226490 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
> +    bool enabled();

Public function name is too simple :(

[Hyowon Kim]

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
> > +    bool enabled();
> 
> Public function name is too simple :(

If it's moved to private, its name is ok?

[Gyuyoung Kim]

Comment on attachment 226490 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:37
> +    OwnPtr<AcceleratedCompositingContext> context = adoptPtr(new AcceleratedCompositingContext(ewkView, compositingObject));

Can't we use c++11 std::unique_ptr ?

>> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
>> +    bool enabled();
> 
> Public function name is too simple :(

Ah. this name comes from super class. I see.

[Ryuan Choi]

Comment on attachment 226490 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:31
> +const double compositingFrameRate = 1.0 / 60.0;

FrameRate is 60 :)

>> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
>> +    bool enabled();
> 
> Public function name is too simple :(

I agree. how about needComposite() or canComposite()?

[Hyowon Kim]

Created attachment 226494 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 226494 [details]
Patch

LGTM. Ryuan might have a final look before landing.

[Hyowon Kim]

(In reply to comment #9)
> (From update of attachment 226490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:37
> > +    OwnPtr<AcceleratedCompositingContext> context = adoptPtr(new AcceleratedCompositingContext(ewkView, compositingObject));
> 
> Can't we use c++11 std::unique_ptr ?

Good. Done.
 
> >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
> >> +    bool enabled();
> > 
> > Public function name is too simple :(
> 
> Ah. this name comes from super class. I see.

enabled() has been replaced with canComposite().

[Hyowon Kim]

(In reply to comment #10)
> (From update of attachment 226490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:31
> > +const double compositingFrameRate = 1.0 / 60.0;
> 
> FrameRate is 60 :)

Right. Done.
 
> >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51
> >> +    bool enabled();
> > 
> > Public function name is too simple :(
> 
> I agree. how about needComposite() or canComposite()?

We chose canCompsite(). :) Done.

[Ryuan Choi]

Comment on attachment 226494 [details]
Patch

Clearing flags on attachment: 226494

Committed r165525: <http://trac.webkit.org/changeset/165525>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.