Bug 130072

Summary: [EFL] Revise AcceleratedCompositingEfl implementation.
Product: WebKit Reporter: Hyowon Kim <hw1008.kim>
Component: WebKit EFLAssignee: Hyowon Kim <hw1008.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, ryuan.choi, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on:    
Bug Blocks: 79766    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Hyowon Kim 2014-03-11 01:41:22 PDT
I think this would be the last bug to enable accelerated compositing on WK1 efl.
Comment 1 Hyowon Kim 2014-03-11 02:07:18 PDT
Created attachment 226399 [details]
Patch
Comment 2 Ryuan Choi 2014-03-11 03:04:08 PDT
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;
Comment 3 Hyowon Kim 2014-03-11 18:51:11 PDT
Created attachment 226463 [details]
Patch
Comment 4 Hyowon Kim 2014-03-11 18:55:13 PDT
> > 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.
Comment 5 Hyowon Kim 2014-03-12 03:32:47 PDT
Created attachment 226490 [details]
Patch
Comment 6 Hyowon Kim 2014-03-12 03:35:50 PDT
(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.
Comment 7 Gyuyoung Kim 2014-03-12 03:59:29 PDT
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 :(
Comment 8 Hyowon Kim 2014-03-12 04:05:11 PDT
> > 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?
Comment 9 Gyuyoung Kim 2014-03-12 04:06:19 PDT
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.
Comment 10 Ryuan Choi 2014-03-12 04:07:19 PDT
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()?
Comment 11 Hyowon Kim 2014-03-12 04:25:17 PDT
Created attachment 226494 [details]
Patch
Comment 12 Gyuyoung Kim 2014-03-12 04:27:12 PDT
Comment on attachment 226494 [details]
Patch

LGTM. Ryuan might have a final look before landing.
Comment 13 Hyowon Kim 2014-03-12 04:28:49 PDT
(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().
Comment 14 Hyowon Kim 2014-03-12 04:31:34 PDT
(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.
Comment 15 Ryuan Choi 2014-03-12 21:47:00 PDT
Comment on attachment 226494 [details]
Patch

Clearing flags on attachment: 226494

Committed r165525: <http://trac.webkit.org/changeset/165525>
Comment 16 Ryuan Choi 2014-03-12 21:47:09 PDT
All reviewed patches have been landed.  Closing bug.