[Webkit-unassigned] [Bug 100674] [EFL][WK2] Allow using ACCELERATED_COMPOSITING without COORDINATED_GRAPHICS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=100674





--- Comment #29 from Christophe Dumez <christophe.dumez at intel.com>  2012-11-02 15:13:55 PST ---
(In reply to comment #28)
> (In reply to comment #26)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=172138&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:133
> > > +        m_pageProxy->setUseFixedLayout(true);
> > 
> > m_pageProxy->setUseFixedLayout(mode == FixedLayoutMode);  on 1 line?
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:187
> > > +    WebCore::IntPoint scrollPosition() const { return m_scrollPosition; }
> > 
> > You could return a const reference here.
> > 
> > > Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.cpp:51
> > > +void PageClientImplFixedLayout::initialize()
> > 
> > Now that we have 3 classes, why do we still need this initialise() method? Why can't we move this code to the constructor?
> > 
> > > Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.h:49
> > > +
> > 
> > extra line here.
> > 
> > > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:89
> > > +    m_viewImpl->setScrollPosition(IntPoint(contentsPoint.x(), contentsPoint.y()));
> > 
> > We could store IntPoint(contentsPoint.x(), contentsPoint.y()) in a variable to construct it once instead of twice.
> 
> These are all small nits that can be fixed before landing.
> I added a comment in the code explaining why we need the initialize method
> 
>     // PageClientImpl is created before WebPageProxy, but PageViewportController
>     // is created after WebPageProxy, so it cannot be created in the constuctor 
>     // of PageClientImpl.

Yes, I missed the comment, sorry. Ok then. We could have an empty implementation for initialize() in the base class (instead of making it pure virtual) so that we don't need to override the method in the legacy class, right?

Besides the small nits, this new patch looks good to me. I have no problem with this new approach.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.