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

Chris Dumez
Reported 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.
Attachments
Patch (21.57 KB, patch)
2013-02-13 06:23 PST, Chris Dumez
no flags
Patch (6.15 KB, patch)
2013-02-13 08:13 PST, Chris Dumez
no flags
Patch (6.20 KB, patch)
2013-02-13 08:24 PST, Chris Dumez
no flags
Patch (55.25 KB, patch)
2013-02-13 11:23 PST, Chris Dumez
no flags
Patch (55.31 KB, patch)
2013-02-14 07:13 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2013-02-13 06:23:16 PST
Kenneth Rohde Christiansen
Comment 2 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?
Chris Dumez
Comment 3 2013-02-13 08:13:48 PST
Created attachment 188083 [details] Patch Keep the change minimal as discussed offline with Kenneth.
Chris Dumez
Comment 4 2013-02-13 08:24:59 PST
Created attachment 188086 [details] Patch Rebase on master.
Kenneth Rohde Christiansen
Comment 5 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?
Chris Dumez
Comment 6 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?
Mikhail Pozdnyakov
Comment 7 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
Chris Dumez
Comment 8 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.
Chris Dumez
Comment 9 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.
Kenneth Rohde Christiansen
Comment 10 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
Chris Dumez
Comment 11 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.
Kenneth Rohde Christiansen
Comment 12 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.
Chris Dumez
Comment 13 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?
Chris Dumez
Comment 14 2013-02-14 04:24:55 PST
Reopening.
Kenneth Rohde Christiansen
Comment 15 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.
Chris Dumez
Comment 16 2013-02-14 07:13:18 PST
Created attachment 188344 [details] Patch Take Kenneth's feedback into consideration.
Kenneth Rohde Christiansen
Comment 17 2013-02-14 09:47:35 PST
Comment on attachment 188344 [details] Patch LGTM
Anders Carlsson
Comment 18 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.
WebKit Review Bot
Comment 19 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>
WebKit Review Bot
Comment 20 2013-02-15 08:51:00 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.