Bug 100674 – [EFL][WK2] Allow using ACCELERATED_COMPOSITING without COORDINATED_GRAPHICS

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Yael]

This mode is needed for running the layout tests.
In Qt port, there is a "--desktop" mode that is used for running layout tests. EFL port needs a similar mode.

[Yael]

Created attachment 171718 [details]
Patch

Make a distinction between calling ewk_view_base_add and ewk_view_smart_add.
Calling ewk_view_base_add creates a desktop style view, that does not support
fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
As a result, WebKitTestRunner and the inspector window do not support fixed layout size, while MiniBrowser does. This change allows many DumpAsText layout tests to pass without modification, when AC is enabled.

I chose to make this distinction, to avoid adding a new public API.

[Kenneth Rohde Christiansen]

Comment on attachment 171718 [details]
Patch

I would like to wait with landing anything like this. I am trying to do some changes so that our code is more correct. I am looking at it with Noam's help.

[Yael]

Created attachment 171721 [details]
Patch

Rebase

[Yael]

Created attachment 171723 [details]
Patch

Third time is a charm :)

[Yael]

(In reply to comment #2)
> (From update of attachment 171718 [details])
> I would like to wait with landing anything like this. I am trying to do some changes so that our code is more correct. I am looking at it with Noam's help.

Do you have a bug ID ?

[Yael]

Created attachment 171751 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 171718 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:549
> -static inline Evas_Object* createEwkView(Evas* canvas, Evas_Smart* smart, PassRefPtr<Ewk_Context> context, WKPageGroupRef pageGroupRef = 0)
> +static inline Evas_Object* createEwkView(Evas* canvas, Evas_Smart* smart, PassRefPtr<Ewk_Context> context, WKPageGroupRef pageGroupRef = 0, bool desktopMode = false)

I hate us calling this desktop mode. The desktop is changing...

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:577
> +    return createEwkView(canvas, createEwkViewSmartClass(), Ewk_Context::create(contextRef), pageGroupRef, true);

an enum would be a lot nicer

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:130
> +    if (!desktopMode)
> +        m_pageViewportController = adoptPtr(new PageViewportController(m_pageProxy.get(), m_pageViewportControllerClient.get()));
>      m_pageProxy->pageGroup()->preferences()->setAcceleratedCompositingEnabled(true);
>      m_pageProxy->pageGroup()->preferences()->setForceCompositingMode(true);
> +    if (m_pageViewportController)
> +        m_pageProxy->setUseFixedLayout(true);

why not do the useFixedLayout further up? do avoid an additional if

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:321
>  void EwkViewImpl::informLoadCommitted()
>  {
> -    m_pageViewportController->didCommitLoad();
> +    if (m_pageViewportController)
> +        m_pageViewportController->didCommitLoad();
> +    else
> +        m_pageViewportControllerClient->didChangeVisibleContents();
>  }

This is weird and confusing. I am not saying it is wrong, but this code just looks really confusing

> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:105
> +    if (!m_pageViewportController) {
> +        FloatRect mapRectToWebContent = FloatRect(m_scrollPosition, m_viewportSize);
> +        mapRectToWebContent.scale(1 / m_scaleFactor);
> +        drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(size.width(), size.height()));
> +    }

can't you uuse the new toWebContents.mapRect( .. ) ?

[Mikhail Pozdnyakov]

Comment on attachment 171751 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:96
> +EwkViewImpl::EwkViewImpl(Evas_Object* view, PassRefPtr<Ewk_Context> context, PassRefPtr<WebPageGroup> pageGroup, bool desktopMode)

don't like custom flags, we need to think of it in OOP way and either have another instance of view impl class or encapsulate the behavioural difference in some strategy class.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:130
> +        m_pageProxy->setUseFixedLayout(true);

maybe put it under the same if (!desktopMode) ?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:327
> +    if (m_pageViewportController)

this should be avoided, need some design decision like I said in first comment

[Yael]

(In reply to comment #7)
> (From update of attachment 171718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171718&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:549
> > -static inline Evas_Object* createEwkView(Evas* canvas, Evas_Smart* smart, PassRefPtr<Ewk_Context> context, WKPageGroupRef pageGroupRef = 0)
> > +static inline Evas_Object* createEwkView(Evas* canvas, Evas_Smart* smart, PassRefPtr<Ewk_Context> context, WKPageGroupRef pageGroupRef = 0, bool desktopMode = false)
> 
> I hate us calling this desktop mode. The desktop is changing...
> 
That is what Qt port is using. Do you have a suggestion for another name?

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:577
> > +    return createEwkView(canvas, createEwkViewSmartClass(), Ewk_Context::create(contextRef), pageGroupRef, true);
> 
> an enum would be a lot nicer
> 
ok
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:130
> > +    if (!desktopMode)
> > +        m_pageViewportController = adoptPtr(new PageViewportController(m_pageProxy.get(), m_pageViewportControllerClient.get()));
> >      m_pageProxy->pageGroup()->preferences()->setAcceleratedCompositingEnabled(true);
> >      m_pageProxy->pageGroup()->preferences()->setForceCompositingMode(true);
> > +    if (m_pageViewportController)
> > +        m_pageProxy->setUseFixedLayout(true);
> 
> why not do the useFixedLayout further up? do avoid an additional if
> 
ok
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:321
> >  void EwkViewImpl::informLoadCommitted()
> >  {
> > -    m_pageViewportController->didCommitLoad();
> > +    if (m_pageViewportController)
> > +        m_pageViewportController->didCommitLoad();
> > +    else
> > +        m_pageViewportControllerClient->didChangeVisibleContents();
> >  }
> 
> This is weird and confusing. I am not saying it is wrong, but this code just looks really confusing
> 
I agree. I did not want to add a new API or another class before I hear other opinions, but now I know that people prefer a second class.

> > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:105
> > +    if (!m_pageViewportController) {
> > +        FloatRect mapRectToWebContent = FloatRect(m_scrollPosition, m_viewportSize);
> > +        mapRectToWebContent.scale(1 / m_scaleFactor);
> > +        drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(size.width(), size.height()));
> > +    }
> 
> can't you uuse the new toWebContents.mapRect( .. ) ?
ok

[Kenneth Rohde Christiansen]

Qt also uses private classes ending in *Flickable and *Legacy

[Yael]

(In reply to comment #10)
> Qt also uses private classes ending in *Flickable and *Legacy
ok, I can rename the new class EwkViewImplLegacy .

[Ryuan Choi]

Comment on attachment 171751 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        Calling ewk_view_base_add creates a desktop style view, that does not support
> +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.

If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?

ewk_view_smart_add is an API that inherit ewk_view on application side.

[Yael]

(In reply to comment #12)
> (From update of attachment 171751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> 
> > Source/WebKit2/ChangeLog:10
> > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> 
> If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> 
> ewk_view_smart_add is an API that inherit ewk_view on application side.

ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.

[Yael]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 171751 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > 
> > > Source/WebKit2/ChangeLog:10
> > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > 
> > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > 
> > ewk_view_smart_add is an API that inherit ewk_view on application side.
> 
> ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.
Or we can turn this into a setting instead of the separation I proposed in my patch. Do you have a preference?

[Yael]

Created attachment 171953 [details]
Patch

Created a new class for legacy mode, but tried to share as much code as possible.

[WebKit Review Bot]

Comment on attachment 171953 [details]
Patch

Attachment 171953 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14692678

New failing tests:
fast/repaint/fixed-table-overflow-zindex.html
fast/repaint/scroll-fixed-reflected-layer.html
fast/repaint/fixed-scale.html
fast/repaint/fixed-tranformed.html

[Mikhail Pozdnyakov]

Comment on attachment 171953 [details]
Patch

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

Think it's worth creating PageClientImplLegacy rather than EwkViewImplLegacy

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:334
> +{

Does the view really need to have all these methods, seems the place of this functionality is inside page client

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:187
> +    virtual float scaleFactor() { return m_pageViewportControllerClient->scaleFactor(); }

should be const

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:188
> +    virtual WebCore::IntPoint scrollPosition() { return m_pageViewportControllerClient->scrollPosition(); }

same here

[Yael]

(In reply to comment #17)
> (From update of attachment 171953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171953&action=review
> 
> Think it's worth creating PageClientImplLegacy rather than EwkViewImplLegacy
> 
I don't agree on that one :) At a high level, being a legacy mode or fixed layout mode is an attribute of the view, not of the page client.

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:334
> > +{
> 
> Does the view really need to have all these methods, seems the place of this functionality is inside page client
> 
It Qt port, these methods go all the way up to QQuickWebView asnd QQuickWebPage. In my first implementation, I took a shortcut.

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:187
> > +    virtual float scaleFactor() { return m_pageViewportControllerClient->scaleFactor(); }
> 
> should be const
> 
ok

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:188
> > +    virtual WebCore::IntPoint scrollPosition() { return m_pageViewportControllerClient->scrollPosition(); }
> 
> same here
ok

[Yael]

Created attachment 172044 [details]
Patch

Added const, as requested in comment #17.

[Yael]

(In reply to comment #16)
> (From update of attachment 171953 [details])
> Attachment 171953 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14692678
> 
> New failing tests:
> fast/repaint/fixed-table-overflow-zindex.html
> fast/repaint/scroll-fixed-reflected-layer.html
> fast/repaint/fixed-scale.html
> fast/repaint/fixed-tranformed.html

I don't see how this is related to my patch. I only modified EFL code in WK2.

[Kenneth Rohde Christiansen]

Well the page client (client of the page) is actually symbolising the view; that is what it was invented for.

[Chris Dumez]

Comment on attachment 172044 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:161
> +void EwkViewImpl::initialize()

I don't really like this initialize() method. We were trying to get rid of this initialization in several steps in separate functions.
I guess this could be solved by having a base class shared by EwkViewImplLegacy and EwkViewImpl. This code would then move to the EwkViewImpl constructor.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:250
> +    IntPoint scrollPosition = this->scrollPosition();

I would remove the "this->", we don't usually use it explicitely

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:251
> +    float scaleFactor = this->scaleFactor();

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:187
> +    virtual const float scaleFactor() { return m_pageViewportControllerClient->scaleFactor(); }

should probably be a const method, not const return value.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:188
> +    virtual const WebCore::IntPoint scrollPosition() { return m_pageViewportControllerClient->scrollPosition(); }

Ditto.

[Yael]

Created attachment 172138 [details]
Patch

Address comments in this bug and on IRC.
Instead of 2 EwkView classes, created 3 PageClient classes: base class, legacy class and fixed layout class.

[WebKit Review Bot]

Attachment 172138 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:93:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:94:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:95:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:96:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 172142 [details]
Patch

Fix style

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 172142 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:93
> +    typedef enum {

I don't believe you need a typedef in C++, enum WindowMode { would suffice.

[Yael]

(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.

[Chris Dumez]

(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.

[EFL EWS Bot]

Comment on attachment 172142 [details]
Patch

Attachment 172142 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14696855

[Yael]

Created attachment 172196 [details]
Patch

Fix the build and address comment #26 and comment #27.

[Yael]

(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?
> 
That will trigger calling  setUseFixedLayout(false) needlessly.

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:187
> > +    WebCore::IntPoint scrollPosition() const { return m_scrollPosition; }
> 
> You could return a const reference here.
> 
ok
> > 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?
> 
ok
> > Source/WebKit2/UIProcess/efl/PageClientImplFixedLayout.h:49
> > +
> 
> extra line here.
> 
ok
> > 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.
I was told before not to create this kind of temporary variables. I wonder if there is something in the coding style about that. (Did it anyways :)

[WebKit Review Bot]

Attachment 172196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/efl/PageClientImpl.h:42:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

Comment on attachment 172196 [details]
Patch

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

looks much nicer for me, thank you!

>> Source/WebKit2/UIProcess/efl/PageClientImpl.h:42
>> +    virtual void initialize() {};
> 
> Missing space inside { }.  [whitespace/braces] [5]

We could do without initialize() function if m_pageViewportController is initialized lazily.

> Source/WebKit2/UIProcess/efl/PageClientImplLegacy.h:45
> +

extra line

[Kenneth Rohde Christiansen]

Comment on attachment 172196 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:95
> +        FixedLayoutMode

I really wish we could find a better name. You can easily use fixed layout without delegating scrolling, handing interaction on the ui side etc.

It all boils down to having touch as a first citizen, which requires non-blocking interaction and handing interaction and painting out of process.

[Chris Dumez]

(In reply to comment #32)
> (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?
> > 
> That will trigger calling  setUseFixedLayout(false) needlessly.

True but I thought it would be more robust not to rely on what the default value is. Without this, the code gets broken easily if someone makes fixed layout the default. Anyway, this is minor.

[Yael]

(In reply to comment #35)
> (From update of attachment 172196 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172196&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:95
> > +        FixedLayoutMode
> 
> I really wish we could find a better name. You can easily use fixed layout without delegating scrolling, handing interaction on the ui side etc.
> 
> It all boils down to having touch as a first citizen, which requires non-blocking interaction and handing interaction and painting out of process.

Do you have a suggestion for a name? If not, we can rename it in a separate patch. This is not a public API, after all :)
The functionality this patch is adding is really about fixed layout vs. non fixed layout and it is not about touch.

[Yael]

(In reply to comment #36)
> (In reply to comment #32)
> > (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?
> > > 
> > That will trigger calling  setUseFixedLayout(false) needlessly.
> 
> True but I thought it would be more robust not to rely on what the default value is. Without this, the code gets broken easily if someone makes fixed layout the default. Anyway, this is minor.
We don't normally do that.

[Yael]

Created attachment 172247 [details]
Patch

Remove the "initialize" method.

[Kenneth Rohde Christiansen]

Comment on attachment 172247 [details]
Patch

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.

[Mikhail Pozdnyakov]

Comment on attachment 172247 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:97
>> +    };
> 
> 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
> }

As we intensively use Legacy/Default terminology, think it's worth naming the enum as 
ViewBehavior { Legacy,  Default } 

and follow this terminology everywhere else.

[Yael]

Created attachment 172320 [details]
Patch

Address comment #40 and comment #41.
Please note that "svn mv" put 2 copies of the files PageClientBase.cpp and PageClientBase.h into the patch. One is exact copy of the PageClientImpl.* and the second copy is my modifications.

[WebKit Review Bot]

Comment on attachment 172320 [details]
Patch

Clearing flags on attachment: 172320

Committed r133464: <http://trac.webkit.org/changeset/133464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryuan Choi]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 171751 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > 
> > > Source/WebKit2/ChangeLog:10
> > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > 
> > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > 
> > ewk_view_smart_add is an API that inherit ewk_view on application side.
> 
> ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.

Sorry for the late answer.
I am so busy because of internal works.

But so sad, ewk_view_base_add is not a public API.
And applications can not access WKContextRef and WKPageGroupRef.

When I get enough time to contribute, I will try to check it more.

[Yael]

(In reply to comment #45)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 171751 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > > 
> > > > Source/WebKit2/ChangeLog:10
> > > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > > 
> > > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > > 
> > > ewk_view_smart_add is an API that inherit ewk_view on application side.
> > 
> > ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.
> 
> Sorry for the late answer.
> I am so busy because of internal works.
> 
> But so sad, ewk_view_base_add is not a public API.
> And applications can not access WKContextRef and WKPageGroupRef.
> 
> When I get enough time to contribute, I will try to check it more.
My apologies. ewk_view_base_add is not a public API, however it is called by WKViewCreate, which is a public API. If that is not a viable solution, then we should create a new public API, how about ewk_view_legacy_add ?
Is it required to be able to switch behaviors at runtime? currently, we need to specify the behavior before the view is created.

[Ryuan Choi]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (From update of attachment 171751 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > > > 
> > > > > Source/WebKit2/ChangeLog:10
> > > > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > > > 
> > > > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > > > 
> > > > ewk_view_smart_add is an API that inherit ewk_view on application side.
> > > 
> > > ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.
> > 
> > Sorry for the late answer.
> > I am so busy because of internal works.
> > 
> > But so sad, ewk_view_base_add is not a public API.
> > And applications can not access WKContextRef and WKPageGroupRef.
> > 
> > When I get enough time to contribute, I will try to check it more.
> My apologies. ewk_view_base_add is not a public API, however it is called by WKViewCreate, which is a public API. If that is not a viable solution, then we should create a new public API, how about ewk_view_legacy_add ?
> Is it required to be able to switch behaviors at runtime? currently, we need to specify the behavior before the view is created.

ewk_view_legacy_add looks not good idea.
If we really need new API, I hope that we add a flag and APIs in ewk_context such as ewk_context_behavior_set.
What do you think about it?

Anyway, I think that I need to learn why we should separate PageClient first.

[Yael]

(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > (In reply to comment #13)
> > > > (In reply to comment #12)
> > > > > (From update of attachment 171751 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > > > > 
> > > > > > Source/WebKit2/ChangeLog:10
> > > > > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > > > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > > > > 
> > > > > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > > > > 
> > > > > ewk_view_smart_add is an API that inherit ewk_view on application side.
> > > > 
> > > > ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.
> > > 
> > > Sorry for the late answer.
> > > I am so busy because of internal works.
> > > 
> > > But so sad, ewk_view_base_add is not a public API.
> > > And applications can not access WKContextRef and WKPageGroupRef.
> > > 
> > > When I get enough time to contribute, I will try to check it more.
> > My apologies. ewk_view_base_add is not a public API, however it is called by WKViewCreate, which is a public API. If that is not a viable solution, then we should create a new public API, how about ewk_view_legacy_add ?
> > Is it required to be able to switch behaviors at runtime? currently, we need to specify the behavior before the view is created.
> 
> ewk_view_legacy_add looks not good idea.
> If we really need new API, I hope that we add a flag and APIs in ewk_context such as ewk_context_behavior_set.
> What do you think about it?
Sure, that will work too.
> 
> Anyway, I think that I need to learn why we should separate PageClient first.

[Yael]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > (In reply to comment #45)
> > > > (In reply to comment #13)
> > > > > (In reply to comment #12)
> > > > > > (From update of attachment 171751 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171751&action=review
> > > > > > 
> > > > > > > Source/WebKit2/ChangeLog:10
> > > > > > > +        Calling ewk_view_base_add creates a desktop style view, that does not support
> > > > > > > +        fixed layout size, while calling ewk_view_smart_add does support fixed layout size.
> > > > > > 
> > > > > > If then, can I know how we can disable fixed layout size when using ewk_view_smart_add ?
> > > > > > 
> > > > > > ewk_view_smart_add is an API that inherit ewk_view on application side.
> > > > > 
> > > > > ewk_view_base_add is a public API. Applications that need to disable fixed layout size can use that.
> > > > 
> > > > Sorry for the late answer.
> > > > I am so busy because of internal works.
> > > > 
> > > > But so sad, ewk_view_base_add is not a public API.
> > > > And applications can not access WKContextRef and WKPageGroupRef.
> > > > 
> > > > When I get enough time to contribute, I will try to check it more.
> > > My apologies. ewk_view_base_add is not a public API, however it is called by WKViewCreate, which is a public API. If that is not a viable solution, then we should create a new public API, how about ewk_view_legacy_add ?
> > > Is it required to be able to switch behaviors at runtime? currently, we need to specify the behavior before the view is created.
> > 
> > ewk_view_legacy_add looks not good idea.
> > If we really need new API, I hope that we add a flag and APIs in ewk_context such as ewk_context_behavior_set.
> > What do you think about it?
> Sure, that will work too.
> > 
> > Anyway, I think that I need to learn why we should separate PageClient first.

Perhaps the new setting in http://trac.webkit.org/changeset/133761 is what you need?