Bug 109684

Created attachment 188073 [details] Patch

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?

Created attachment 188083 [details] Patch Keep the change minimal as discussed offline with Kenneth.

Created attachment 188086 [details] Patch Rebase on master.

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?

Created attachment 188126 [details] Patch Kenneth, I had WebView subclass PageClient as you suggested. What do you think?

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

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

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

Reopening.

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

Created attachment 188344 [details] Patch Take Kenneth's feedback into consideration.

Comment on attachment 188344 [details] Patch LGTM

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 on attachment 188344 [details] Patch Clearing flags on attachment: 188344 Committed r143004: <http://trac.webkit.org/changeset/143004>

All reviewed patches have been landed. Closing bug.