Bug 109684

Summary: [EFL][WK2] Have WebView subclass PageClient
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, benjamin, gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2013-02-13 05:26:10 PST
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.
Comment 1 Chris Dumez 2013-02-13 06:23:16 PST
Created attachment 188073 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-13 06:49:02 PST
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?
Comment 3 Chris Dumez 2013-02-13 08:13:48 PST
Created attachment 188083 [details]
Patch

Keep the change minimal as discussed offline with Kenneth.
Comment 4 Chris Dumez 2013-02-13 08:24:59 PST
Created attachment 188086 [details]
Patch

Rebase on master.
Comment 5 Kenneth Rohde Christiansen 2013-02-13 08:44:41 PST
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?
Comment 6 Chris Dumez 2013-02-13 11:23:54 PST
Created attachment 188126 [details]
Patch

Kenneth, I had WebView subclass PageClient as you suggested. What do you think?
Comment 7 Mikhail Pozdnyakov 2013-02-13 23:36:48 PST
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
Comment 8 Chris Dumez 2013-02-13 23:45:32 PST
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.
Comment 9 Chris Dumez 2013-02-13 23:47:59 PST
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.
Comment 10 Kenneth Rohde Christiansen 2013-02-14 03:48:42 PST
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
Comment 11 Chris Dumez 2013-02-14 03:57:56 PST
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.
Comment 12 Kenneth Rohde Christiansen 2013-02-14 04:15:00 PST
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.
Comment 13 Chris Dumez 2013-02-14 04:24:27 PST
(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?
Comment 14 Chris Dumez 2013-02-14 04:24:55 PST
Reopening.
Comment 15 Kenneth Rohde Christiansen 2013-02-14 04:33:55 PST
> > 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.
Comment 16 Chris Dumez 2013-02-14 07:13:18 PST
Created attachment 188344 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 17 Kenneth Rohde Christiansen 2013-02-14 09:47:35 PST
Comment on attachment 188344 [details]
Patch

LGTM
Comment 18 Anders Carlsson 2013-02-15 08:31:26 PST
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.
Comment 19 WebKit Review Bot 2013-02-15 08:50:55 PST
Comment on attachment 188344 [details]
Patch

Clearing flags on attachment: 188344

Committed r143004: <http://trac.webkit.org/changeset/143004>
Comment 20 WebKit Review Bot 2013-02-15 08:51:00 PST
All reviewed patches have been landed.  Closing bug.