Bug 109684 – [EFL][WK2] Have WebView subclass PageClient

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

To respect API layering, EFL's PageClient should be constructed and owned by WebView, not EwkView.
Our Ewk implementation should never interact directly with the PageClient.

[Chris Dumez]

Created attachment 188073 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 188073 [details]
Patch

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

> Source/WebKit2/ChangeLog:16
> +        * Shared/API/c/WKSharedAPICast.h:
> +        (WebKit::toAPI): Add toAPI() function to convert a FloatPoint into a WKPoint.
> +        There was already one for IntPoint but the FloatPoint equivalent was missing.

Maybe this should be a separate change?

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:85
> +void WKViewUpdateViewportSize(WKViewRef viewRef, WKSize size)

s/Update/Set

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:93
> +WKPoint WKViewGetPagePosition(WKViewRef viewRef)
> +{
> +    return toAPI(toImpl(viewRef)->pagePosition());
> +}

I guess it might be better to get the visible page rect instead (it contains position).

WKViewGetVisiblePageRect(WKViewRef)

It also makes a more obvious that this is in page coordinates. A page position could include scale.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:95
> +void WKViewDidCommitLoad(WKViewRef viewRef)

When would a user call this? It is not obvious

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1185
> -        view->pageClient()->updateViewportSize();
> +        WKViewUpdateViewportSize(view->wkView(), WKSizeMake(width, height));
>      }

so this actually sets the visible contents rect?

[Chris Dumez]

Created attachment 188083 [details]
Patch

Keep the change minimal as discussed offline with Kenneth.

[Chris Dumez]

Created attachment 188086 [details]
Patch

Rebase on master.

[Kenneth Rohde Christiansen]

Comment on attachment 188086 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebView.h:44
>  class WebView : public APIObject {
>  public:

maybe we should inherit from the pageclient in the future?

[Chris Dumez]

Created attachment 188126 [details]
Patch

Kenneth, I had WebView subclass PageClient as you suggested. What do you think?

[Mikhail Pozdnyakov]

Comment on attachment 188126 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:43
> +    return static_cast<WebView*>(m_pageClient)->evasObject();

we should avoid using of WebView::evasObject

> Source/WebKit2/UIProcess/efl/WebView.cpp:40
> +#include "ewk_context_private.h"

wish "ewk_context_private.h" wasn't included

> Source/WebKit2/UIProcess/efl/WebView.cpp:46
> +using namespace EwkViewCallbacks;

WebView should not know about EwkViewCallbacks

> Source/WebKit2/UIProcess/efl/WebView.cpp:55
> +    m_page->pageGroup()->preferences()->setAcceleratedCompositingEnabled(true);

why rename?

> Source/WebKit2/UIProcess/efl/WebView.cpp:132
> +        m_ewkView->scheduleUpdateDisplay();

it has to be done via view client interface

> Source/WebKit2/UIProcess/efl/WebView.cpp:181
> +    return m_ewkView->size();

I believe direct m_ewkView usage should be eliminated

> Source/WebKit2/UIProcess/efl/WebView.cpp:215
> +    m_ewkView->smartCallback<TooltipTextUnset>().call();

ahh, I think we need make mature view client first and subclass pageclient after

[Chris Dumez]

Comment on attachment 188126 [details]
Patch

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

>> Source/WebKit2/UIProcess/efl/WebView.cpp:40
>> +#include "ewk_context_private.h"
> 
> wish "ewk_context_private.h" wasn't included

Well, it is needed for now. This will be a gradual move away from Ewk.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:46
>> +using namespace EwkViewCallbacks;
> 
> WebView should not know about EwkViewCallbacks

Agreed but for now we need it (until we extend the WKViewClient). Our PageClient implementation is using EwkView callbacks and for now I am merely moving the code to WebView.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:55
>> +    m_page->pageGroup()->preferences()->setAcceleratedCompositingEnabled(true);
> 
> why rename?

m_page is much shorter and it is still clear. As a matter of fact, we usually call the getter "page()".

>> Source/WebKit2/UIProcess/efl/WebView.cpp:132
>> +        m_ewkView->scheduleUpdateDisplay();
> 
> it has to be done via view client interface

In the future yes. This patch is about merging the PageClient code into WebView, not porting the PageClient code to WKViewClient.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:181
>> +    return m_ewkView->size();
> 
> I believe direct m_ewkView usage should be eliminated

Same comments apply here.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:215
>> +    m_ewkView->smartCallback<TooltipTextUnset>().call();
> 
> ahh, I think we need make mature view client first and subclass pageclient after

Well either is possible but I think it is clearer if all the PageClient code is in WebView first. We can see more easily what are the needs for both Legacy and Default implementation.

[Chris Dumez]

Comment on attachment 188126 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebView.h:-62
> -    void setViewNeedsDisplay(const WebCore::IntRect&);

Also notice that merging PageClient and WebView allows us to remove those duplicated View client methods. For now we have only 2 but a lot more are coming. If we port all the code to the View client first, we will have a lot of duplicated methods between PageClient and WebView. This is why I think we should have WebView subclass PageClient first.

[Kenneth Rohde Christiansen]

Comment on attachment 188126 [details]
Patch

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

>>> Source/WebKit2/UIProcess/efl/WebView.cpp:181
>>> +    return m_ewkView->size();
>> 
>> I believe direct m_ewkView usage should be eliminated
> 
> Same comments apply here.

My patch does part of this by introducing the m_size

>>> Source/WebKit2/UIProcess/efl/WebView.cpp:215
>>> +    m_ewkView->smartCallback<TooltipTextUnset>().call();
>> 
>> ahh, I think we need make mature view client first and subclass pageclient after
> 
> Well either is possible but I think it is clearer if all the PageClient code is in WebView first. We can see more easily what are the needs for both Legacy and Default implementation.

Yeah, I think it would be nicer to have this in the client first instead of moving code here and later back

[Chris Dumez]

Considering how much difference the implementation order makes, and that the patch was already written, I think this is a shame but you guys do what you want.

[Kenneth Rohde Christiansen]

Comment on attachment 188126 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/efl/WebView.cpp:215
>>>> +    m_ewkView->smartCallback<TooltipTextUnset>().call();
>>> 
>>> ahh, I think we need make mature view client first and subclass pageclient after
>> 
>> Well either is possible but I think it is clearer if all the PageClient code is in WebView first. We can see more easily what are the needs for both Legacy and Default implementation.
> 
> Yeah, I think it would be nicer to have this in the client first instead of moving code here and later back

I like this patch, it cleans up a lot of code, but it is obvious that it is just one step on the way.

A suggestion would be adding the processDidCrash and a few other needed methods to the ViewClient first (as another patch).

That way we could rebase this patch on top. I don't really mind either way. and I am fine with this patch.

> Source/WebKit2/UIProcess/efl/WebView.h:90
> +private:
> +    // PageClient
> +    virtual PassOwnPtr<DrawingAreaProxy> createDrawingAreaProxy();
> +    virtual void setViewNeedsDisplay(const WebCore::IntRect&);
> +    virtual void displayView();
> +    virtual bool canScrollView() { return false; }
> +    virtual void scrollView(const WebCore::IntRect&, const WebCore::IntSize&);
> +    virtual WebCore::IntSize viewSize();
> +    virtual bool isViewWindowActive();
> +    virtual bool isViewFocused();
> +    virtual bool isViewVisible();
> +    virtual bool isViewInWindow();
> +    virtual void processDidCrash();

It is possible to "group these" a bit nicer.. like add some newlines where it makes sense?

> Source/WebKit2/UIProcess/efl/WebView.h:139
> +    OwnPtr<WebKit::PageViewportControllerClientEfl> m_pageViewportControllerClient;
> +    OwnPtr<WebKit::PageViewportController> m_pageViewportController;

We also need to get rid of these at some point.

[Chris Dumez]

(In reply to comment #12)
> (From update of attachment 188126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188126&action=review
> 
> >>>> Source/WebKit2/UIProcess/efl/WebView.cpp:215
> >>>> +    m_ewkView->smartCallback<TooltipTextUnset>().call();
> >>> 
> >>> ahh, I think we need make mature view client first and subclass pageclient after
> >> 
> >> Well either is possible but I think it is clearer if all the PageClient code is in WebView first. We can see more easily what are the needs for both Legacy and Default implementation.
> > 
> > Yeah, I think it would be nicer to have this in the client first instead of moving code here and later back
> 
> I like this patch, it cleans up a lot of code, but it is obvious that it is just one step on the way.
> 
> A suggestion would be adding the processDidCrash and a few other needed methods to the ViewClient first (as another patch).
> 
> That way we could rebase this patch on top. I don't really mind either way. and I am fine with this patch.

The rebasing is going to be a bit annoying though.

> 
> > Source/WebKit2/UIProcess/efl/WebView.h:90
> > +private:
> > +    // PageClient
> > +    virtual PassOwnPtr<DrawingAreaProxy> createDrawingAreaProxy();
> > +    virtual void setViewNeedsDisplay(const WebCore::IntRect&);
> > +    virtual void displayView();
> > +    virtual bool canScrollView() { return false; }
> > +    virtual void scrollView(const WebCore::IntRect&, const WebCore::IntSize&);
> > +    virtual WebCore::IntSize viewSize();
> > +    virtual bool isViewWindowActive();
> > +    virtual bool isViewFocused();
> > +    virtual bool isViewVisible();
> > +    virtual bool isViewInWindow();
> > +    virtual void processDidCrash();
> 
> It is possible to "group these" a bit nicer.. like add some newlines where it makes sense?

Yes, I can group these nicely.

> 
> > Source/WebKit2/UIProcess/efl/WebView.h:139
> > +    OwnPtr<WebKit::PageViewportControllerClientEfl> m_pageViewportControllerClient;
> > +    OwnPtr<WebKit::PageViewportController> m_pageViewportController;
> 
> We also need to get rid of these at some point.

So, what is your suggestion? Add a FIXME comment?

[Chris Dumez]

Reopening.

[Kenneth Rohde Christiansen]

> > That way we could rebase this patch on top. I don't really mind either way. and I am fine with this patch.
> 
> The rebasing is going to be a bit annoying though.

Lets talk to Mikhail on irc to see what path we choose.

> > We also need to get rid of these at some point.
> 
> So, what is your suggestion? Add a FIXME comment?

Just move the other FIXME a bit down.

[Chris Dumez]

Created attachment 188344 [details]
Patch

Take Kenneth's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 188344 [details]
Patch

LGTM

[Anders Carlsson]

Comment on attachment 188344 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebView.cpp:161
> +    OwnPtr<DrawingAreaProxy> drawingArea = DrawingAreaProxyImpl::create(page());
> +    return drawingArea.release();

Can just return DrawingAreaProxyImpl::create(page()) directly here.

[WebKit Review Bot]

Comment on attachment 188344 [details]
Patch

Clearing flags on attachment: 188344

Committed r143004: <http://trac.webkit.org/changeset/143004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.