[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 #40 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2012-11-04 15:58:14 PST ---
(From update of attachment 172247)
View in context: https://bugs.webkit.org/attachment.cgi?id=172247&action=review

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:97
> +    enum WindowMode {
> +        LegacyMode,
> +        FixedLayoutMode
> +    };

I don't like this but :) better suggestions are hard to come by,

I think it is a kind of behavior (that is a good generic term joining rendering strategy, UI-side interaction, etc) and that it is related to the *view* not the *window*

enum ViewBehavior {
    LegacyBehavior,
    TouchOptimized
}

> Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.h:37
> +class PageClientImplFixedLayout : public PageClientImpl {

If this is the default, why not just call it PageClientImpl or PageClientDefaultImpl.

Hmm, it is indeed split.

I suggest

PageClientBase.cpp (as it is not a full implementation)
PageClientLegacyImpl.cpp
PageClientDefaultImpl.cpp

> Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.h:43
> +    static PassOwnPtr<PageClientImpl> create(EwkViewImpl* viewImpl)
> +    {
> +        return adoptPtr(new PageClientImplFixedLayout(viewImpl));
> +    }
> +    virtual ~PageClientImplFixedLayout();

I would add a newline after ctor and dtor

> Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.h:54
> +#if USE(TILED_BACKING_STORE)
> +    virtual void pageDidRequestScroll(const WebCore::IntPoint&);
> +#endif
> +    virtual void didChangeContentsSize(const WebCore::IntSize&);
> +#if USE(TILED_BACKING_STORE)

WHy not reorder? avoid all those ifs!

> Source/WebKit2/UIProcess/efl/PageClientImplLegacy.cpp:45
> +PageClientImplLegacy::~PageClientImplLegacy()
> +{
> +}

Why it this not just done in the header? We don't care about API/ABI breakage

> Source/WebKit2/UIProcess/efl/PageClientImplLegacy.cpp:72
> +void PageClientImplLegacy::didChangeContentsSize(const WebCore::IntSize& size)
> +{
> +#if USE(TILED_BACKING_STORE)
> +    m_viewImpl->page()->drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(FloatSize(size.width(), size.height()));
> +    m_viewImpl->update();
> +#else
> +    m_viewImpl->informContentsSizeChange(size);
> +#endif
> +}

Do here it is hard to see if the upper part also ends up informing the client of the content size change. A comment could clarify that, like informContentSizeChanged called as a result of setContentsSize or so

> Source/WebKit2/UIProcess/efl/PageClientImplLegacy.h:41
> +public:
> +    static PassOwnPtr<PageClientImpl> create(EwkViewImpl* viewImpl)
> +    {
> +        return adoptPtr(new PageClientImplLegacy(viewImpl));
> +    }
> +    virtual ~PageClientImplLegacy();
> +    virtual void didCommitLoad();

newline after dtor

> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:88
>  void PageViewportControllerClientEfl::setViewportPosition(const WebCore::FloatPoint& contentsPoint)
>  {
> -    setVisibleContentsRect(IntPoint(contentsPoint.x(), contentsPoint.y()), m_scaleFactor, FloatPoint());
> +    IntPoint viewportPosition(contentsPoint.x(), contentsPoint.y());

so not a contents*Point* is a viewport*Position*. Why are they not both called *Position ?

Why is contents pos equal to viewport pos? This is a bit confusing.

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