Summary: | [EFL] Revise AcceleratedCompositingEfl implementation. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hyowon Kim <hw1008.kim> | ||||||||||
Component: | WebKit EFL | Assignee: | 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
Hyowon Kim
2014-03-11 01:41:22 PDT
Created attachment 226399 [details]
Patch
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; Created attachment 226463 [details]
Patch
> > 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. Created attachment 226490 [details]
Patch
(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 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 :( > > 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 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 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()? Created attachment 226494 [details]
Patch
Comment on attachment 226494 [details]
Patch
LGTM. Ryuan might have a final look before landing.
(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(). (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 on attachment 226494 [details] Patch Clearing flags on attachment: 226494 Committed r165525: <http://trac.webkit.org/changeset/165525> All reviewed patches have been landed. Closing bug. |