Bug 100674 - [EFL][WK2] Allow using ACCELERATED_COMPOSITING without COORDINATED_GRAPHICS
Summary: [EFL][WK2] Allow using ACCELERATED_COMPOSITING without COORDINATED_GRAPHICS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-29 07:55 PDT by Yael
Modified: 2012-11-09 10:03 PST (History)
9 users (show)

See Also:


Attachments
Patch (16.13 KB, patch)
2012-10-31 14:14 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2012-10-31 14:29 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2012-10-31 14:42 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (14.25 KB, patch)
2012-10-31 18:32 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (22.48 KB, patch)
2012-11-01 16:41 PDT, Yael
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (22.07 KB, patch)
2012-11-02 05:02 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (32.72 KB, patch)
2012-11-02 14:27 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (32.74 KB, patch)
2012-11-02 14:38 PDT, Yael
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (33.18 KB, patch)
2012-11-02 19:45 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (31.91 KB, patch)
2012-11-04 14:20 PST, Yael
no flags Details | Formatted Diff | Diff
Patch (76.03 KB, patch)
2012-11-05 05:27 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-10-29 07:55:37 PDT
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.
Comment 1 Yael 2012-10-31 14:14:24 PDT
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.
Comment 2 Kenneth Rohde Christiansen 2012-10-31 14:18:38 PDT
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.
Comment 3 Yael 2012-10-31 14:29:53 PDT
Created attachment 171721 [details]
Patch

Rebase
Comment 4 Yael 2012-10-31 14:42:00 PDT
Created attachment 171723 [details]
Patch

Third time is a charm :)
Comment 5 Yael 2012-10-31 14:50:08 PDT
(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 ?
Comment 6 Yael 2012-10-31 18:32:00 PDT
Created attachment 171751 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2012-11-01 03:25:09 PDT
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( .. ) ?
Comment 8 Mikhail Pozdnyakov 2012-11-01 03:46:09 PDT
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
Comment 9 Yael 2012-11-01 07:34:20 PDT
(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
Comment 10 Kenneth Rohde Christiansen 2012-11-01 07:55:03 PDT
Qt also uses private classes ending in *Flickable and *Legacy
Comment 11 Yael 2012-11-01 08:18:33 PDT
(In reply to comment #10)
> Qt also uses private classes ending in *Flickable and *Legacy
ok, I can rename the new class EwkViewImplLegacy .
Comment 12 Ryuan Choi 2012-11-01 08:33:28 PDT
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.
Comment 13 Yael 2012-11-01 08:53:24 PDT
(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.
Comment 14 Yael 2012-11-01 09:34:07 PDT
(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?
Comment 15 Yael 2012-11-01 16:41:54 PDT
Created attachment 171953 [details]
Patch

Created a new class for legacy mode, but tried to share as much code as possible.
Comment 16 WebKit Review Bot 2012-11-01 21:56:41 PDT
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
Comment 17 Mikhail Pozdnyakov 2012-11-02 03:20:17 PDT
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
Comment 18 Yael 2012-11-02 04:54:40 PDT
(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
Comment 19 Yael 2012-11-02 05:02:28 PDT
Created attachment 172044 [details]
Patch

Added const, as requested in comment #17.
Comment 20 Yael 2012-11-02 05:03:27 PDT
(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.
Comment 21 Kenneth Rohde Christiansen 2012-11-02 05:08:11 PDT
Well the page client (client of the page) is actually symbolising the view; that is what it was invented for.
Comment 22 Chris Dumez 2012-11-02 06:42:19 PDT
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.
Comment 23 Yael 2012-11-02 14:27:59 PDT
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.
Comment 24 WebKit Review Bot 2012-11-02 14:31:05 PDT
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.
Comment 25 Yael 2012-11-02 14:38:25 PDT
Created attachment 172142 [details]
Patch

Fix style
Comment 26 Chris Dumez 2012-11-02 14:43:56 PDT
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.
Comment 27 Chris Dumez 2012-11-02 14:51:10 PDT
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.
Comment 28 Yael 2012-11-02 14:55:15 PDT
(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.
Comment 29 Chris Dumez 2012-11-02 15:13:55 PDT
(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.
Comment 30 EFL EWS Bot 2012-11-02 17:47:17 PDT
Comment on attachment 172142 [details]
Patch

Attachment 172142 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14696855
Comment 31 Yael 2012-11-02 19:45:24 PDT
Created attachment 172196 [details]
Patch

Fix the build and address comment #26 and comment #27.
Comment 32 Yael 2012-11-02 19:48:51 PDT
(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 :)
Comment 33 WebKit Review Bot 2012-11-02 19:51:32 PDT
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.
Comment 34 Mikhail Pozdnyakov 2012-11-03 05:02:09 PDT
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
Comment 35 Kenneth Rohde Christiansen 2012-11-03 09:36:22 PDT
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.
Comment 36 Chris Dumez 2012-11-03 11:50:55 PDT
(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.
Comment 37 Yael 2012-11-03 16:05:21 PDT
(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.
Comment 38 Yael 2012-11-03 16:05:48 PDT
(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.
Comment 39 Yael 2012-11-04 14:20:43 PST
Created attachment 172247 [details]
Patch

Remove the "initialize" method.
Comment 40 Kenneth Rohde Christiansen 2012-11-04 15:58:14 PST
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.
Comment 41 Mikhail Pozdnyakov 2012-11-05 05:09:28 PST
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.
Comment 42 Yael 2012-11-05 05:27:25 PST
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.
Comment 43 WebKit Review Bot 2012-11-05 06:22:59 PST
Comment on attachment 172320 [details]
Patch

Clearing flags on attachment: 172320

Committed r133464: <http://trac.webkit.org/changeset/133464>
Comment 44 WebKit Review Bot 2012-11-05 06:23:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 Ryuan Choi 2012-11-05 18:12:07 PST
(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.
Comment 46 Yael 2012-11-05 18:30:21 PST
(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.
Comment 47 Ryuan Choi 2012-11-07 15:42:34 PST
(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.
Comment 48 Yael 2012-11-07 16:51:43 PST
(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.
Comment 49 Yael 2012-11-09 10:03:47 PST
(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?