Currently ViewportHandlerQt includes a lot of behavior that's not platform specific, together with hooks to the Qt API layer which are platform specific. It would make things cleaner if we could have a non-Qt ViewportHandler class, with a client (ViewportHandlerClient?) that's implemented specifically for every platform. We can put the QQuick-releated code and things like the scale QAnimation in the client class (QtViewportHandlerClient?) This would allow other ports to use ViewportHandler, and would also enable usage of the viewport functionality from QRawWebView.
I'm messing around with this, and it seems pretty doable. The main tricky part is that some functions use viewport coordinates and some use contents coordinates, and it's not always apparent. If someone else wants to pick this challenge, please assign it to yourself :)
(In reply to comment #1) > I'm messing around with this, and it seems pretty doable. The main tricky part is that some functions use viewport coordinates and some use contents coordinates, and it's not always apparent. > > If someone else wants to pick this challenge, please assign it to yourself :) Since I wrote major parts of this code I can work on it as soon as I'm back from vacation in two weeks. I already have some needed patches that also touch the viewport handler and it would be good to align them with this effort.
> Since I wrote major parts of this code I can work on it as soon as I'm back from vacation in two weeks. I was hoping for that :) Enjoy your vacation.
I very much support this change! but I am also on vacation :-)
Created attachment 161723 [details] Patch
Comment on attachment 161723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161723&action=review Good first step. Some random comments all over... trying to figure how to make this better. It would be really good to see this actually used by another port such as EFL or GTK+ > Source/WebKit2/UIProcess/ViewportHandler.cpp:104 > +float ViewportHandler::viewportCoordFromCSS(float value) const float convertToViewport(float value) const > Source/WebKit2/UIProcess/ViewportHandler.cpp:203 > +void ViewportHandler::updateWebProcessViewportState(const FloatPoint& trajectoryVector) > +{ > + DrawingAreaProxy* const drawingArea = m_webPageProxy->drawingArea(); > + if (!drawingArea || m_viewportSize.isEmpty() || m_contentsSize.isEmpty() || m_visibleContentsRect.isEmpty()) > + return; > + > + drawingArea->setVisibleContentsRect(m_visibleContentsRect, m_viewportScale, trajectoryVector); > + > + m_client->visibleContentsChanged(); > +} syncVisibleContent()? inform... ? > Source/WebKit2/UIProcess/ViewportHandler.h:65 > +class ViewportUpdateDeferrer { Is this only used in this cpp file? so can be be internal? or could it be methods of the ViewportHandler... > Source/WebKit2/UIProcess/ViewportHandler.h:74 > +class ViewportHandler { ContentsViewport ? The class deals with the view to the contents and with the position, state (suspended, etc) of the content. Maybe ContentViewport is a better name? > Source/WebKit2/UIProcess/ViewportHandler.h:79 > + FloatRect positionRangeForContentAtScale(float viewportScale) const; Isn't this name very Qt specific? > Source/WebKit2/UIProcess/ViewportHandler.h:87 > + FloatRect viewportRectFromCSS(const FloatRect&) const; > + float cssScaleFromViewport(float) const; > + float viewportScaleFromCSS(float) const; > + float viewportCoordFromCSS(float) const; > + > + float innerBoundedCSSScale(float) const; > + float outerBoundedCSSScale(float) const; We could make this more compatible with FrameView and get rid of the css confusing. FloatRect convertToViewport(const FloatRect&) const IntPoint convertToViewport(const IntPoint&) const float convertToViewport(float scale) const float convertFromViewport(float scale) const float innerBoundedContentScale(float scale) const; > Source/WebKit2/UIProcess/ViewportHandler.h:93 > + FloatSize layoutSize() const { return m_rawAttributes.layoutSize; } contentsLayoutSize() > Source/WebKit2/UIProcess/ViewportHandler.h:95 > + float minimumScale() const { return m_minimumScale; } minimumContentsScale? > Source/WebKit2/UIProcess/ViewportHandler.h:103 > + // Notifications to the WebProcess. > + void updateViewportSize(const FloatSize& newSize); > + void updateVisibleContentsRect(const FloatRect& visibleContentsRect, float viewportScale, const FloatPoint& trajectoryVector = FloatPoint::zero()); > + Isn't similar methods in WebKit2 called someting like notify or send? > Source/WebKit2/UIProcess/ViewportHandler.h:110 > + void suspendPageContent(); suspendContent? > Source/WebKit2/UIProcess/ViewportHandler.h:137 > +bool fuzzyCompare(float, float, float epsilon); > +FloatPoint boundPosition(const FloatPoint minPosition, const FloatPoint& position, const FloatPoint& maxPosition); I guess these could be int the cpp file? > Source/WebKit2/UIProcess/ViewportHandlerClient.h:37 > + virtual void setContentPosition(const WebCore::FloatPoint& positionInViewCoordinates) = 0; > + virtual void setContentsScale(float scale, bool isInitialScale) = 0; So the former is in viewport coords? The second is in CSS scales? This needs to be clearer > Source/WebKit2/UIProcess/ViewportHandlerClient.h:44 > + virtual void contentsSizeChanged() = 0; > + virtual void visibleContentsChanged() = 0; > + virtual void viewportAttributesChanged() = 0; Very Qt'ish names, not very WebKit like. What is used elsewhere? didChangeContentSize? or contentSizeDidChange? > Source/WebKit2/UIProcess/ViewportHandlerClient.h:47 > + virtual void setViewportHandler(ViewportHandler*) = 0; > +}; The "client" is normally what the port implements so why should it implement how to set the viewporthandler > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.cpp:188 > +void ViewportHandlerClientQt::focusEditableArea(const QRectF& caretArea, const QRectF& targetArea) > +{ > + // This can only happen as a result of a user interaction. > + ASSERT(m_viewportHandler->hadUserInteraction()); > + > + QRectF endArea = m_viewportHandler->viewportRectFromCSS(targetArea); > + Without doing the actual animation, it would be really nice to share methods to calculate these target areas. IntRect bestVisibleRectForEditableElement(const IntRect& elementRect, const IntRect& caretRect, float preferredEditingScale) > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.cpp:219 > +void ViewportHandlerClientQt::zoomToAreaGestureEnded(const QPointF& touchPoint, const QRectF& targetArea) To follow the above, part of this code could be split out into a generic IntRect bestVisibleRectForNonEditableElement(const IntRect& elementRect, const IntPoint& interestPoint) > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.h:44 > +class ViewportHandlerClientQt : public QObject, public ViewportHandlerClient { > + Q_OBJECT Maybe it makes sense moving the gesture recognition here? > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.h:52 > + virtual void setContentPosition(const FloatPoint& positionInViewCoordinates); > + virtual void setContentsScale(float scale, bool isInitialScale); one with s one without?
Created attachment 161902 [details] proposed patch
Comment on attachment 161902 [details] proposed patch Attachment 161902 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13741146
Comment on attachment 161902 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=161902&action=review > Source/WebKit2/UIProcess/ViewportHandler.h:105 > + void updateViewportSize(const FloatSize& newSize); > + void updateVisibleContentsRect(const FloatRect& visibleContentsRect, float viewportScale, const FloatPoint& trajectoryVector = FloatPoint::zero()); FrameView seems to use adjust* naming.
(In reply to comment #9) > (From update of attachment 161902 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161902&action=review > > > Source/WebKit2/UIProcess/ViewportHandler.h:105 > > + void updateViewportSize(const FloatSize& newSize); > > + void updateVisibleContentsRect(const FloatRect& visibleContentsRect, float viewportScale, const FloatPoint& trajectoryVector = FloatPoint::zero()); > > FrameView seems to use adjust* naming. adjust* sound ok.
(In reply to comment #8) > (From update of attachment 161902 [details]) > Attachment 161902 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13741146 Seems bogus.
Created attachment 161907 [details] proposed patch Applied naming suggestions where appropriate.
Comment on attachment 161907 [details] proposed patch Attachment 161907 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13744117
(In reply to comment #6) > (From update of attachment 161723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161723&action=review > > Good first step. Some random comments all over... trying to figure how to make this better. It would be really good to see this actually used by another port such as EFL or GTK+ > > > Source/WebKit2/UIProcess/ViewportHandler.cpp:104 > > +float ViewportHandler::viewportCoordFromCSS(float value) const > > float convertToViewport(float value) const [...] > syncVisibleContent()? inform... ? > Renamed. > > Source/WebKit2/UIProcess/ViewportHandler.h:65 > > +class ViewportUpdateDeferrer { > > Is this only used in this cpp file? so can be be internal? or could it be methods of the ViewportHandler... Although the guard is currently only an implementation detail of the Qt client and it's usage is not imposed, I think the functionality of a scoped update guard could be useful for other ports as well. > > > Source/WebKit2/UIProcess/ViewportHandler.h:74 > > +class ViewportHandler { > > ContentsViewport ? The class deals with the view to the contents and with the position, state (suspended, etc) of the content. Maybe ContentViewport is a better name? I'm not sure putting content in the class name would improve the name. The term "content" is already used as a synonym for the CSS coordinate system. > > > Source/WebKit2/UIProcess/ViewportHandler.h:79 > > + FloatRect positionRangeForContentAtScale(float viewportScale) const; > > Isn't this name very Qt specific? Which part do you feel is Qt-specific? Do you have any suggestion for a better name? > [snip] Renames done. > > > Source/WebKit2/UIProcess/ViewportHandler.h:137 > > +bool fuzzyCompare(float, float, float epsilon); > > +FloatPoint boundPosition(const FloatPoint minPosition, const FloatPoint& position, const FloatPoint& maxPosition); > > I guess these could be int the cpp file? No, they are also used in the client. > > > Source/WebKit2/UIProcess/ViewportHandlerClient.h:37 > > + virtual void setContentPosition(const WebCore::FloatPoint& positionInViewCoordinates) = 0; > > + virtual void setContentsScale(float scale, bool isInitialScale) = 0; > > So the former is in viewport coords? > The second is in CSS scales? > > This needs to be clearer Both are in viewport coords, made it more explicit in the parameter name. > > > Source/WebKit2/UIProcess/ViewportHandlerClient.h:44 > > + virtual void contentsSizeChanged() = 0; > > + virtual void visibleContentsChanged() = 0; > > + virtual void viewportAttributesChanged() = 0; > > Very Qt'ish names, not very WebKit like. What is used elsewhere? didChangeContentSize? or contentSizeDidChange? Looks like the did* pattern is more common in WebKit2. > > > Source/WebKit2/UIProcess/ViewportHandlerClient.h:47 > > + virtual void setViewportHandler(ViewportHandler*) = 0; > > +}; > > The "client" is normally what the port implements so why should it implement how to set the viewporthandler Since the ViewportHandler is used by the client by setting it in the constructor we assure that it is always set. > > > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.cpp:188 > > +void ViewportHandlerClientQt::focusEditableArea(const QRectF& caretArea, const QRectF& targetArea) > > +{ > > + // This can only happen as a result of a user interaction. > > + ASSERT(m_viewportHandler->hadUserInteraction()); > > + > > + QRectF endArea = m_viewportHandler->viewportRectFromCSS(targetArea); > > + > > Without doing the actual animation, it would be really nice to share methods to calculate these target areas. > > IntRect bestVisibleRectForEditableElement(const IntRect& elementRect, const IntRect& caretRect, float preferredEditingScale) > > > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.cpp:219 > > +void ViewportHandlerClientQt::zoomToAreaGestureEnded(const QPointF& touchPoint, const QRectF& targetArea) > > To follow the above, part of this code could be split out into a generic > > IntRect bestVisibleRectForNonEditableElement(const IntRect& elementRect, const IntPoint& interestPoint) I want to clean up the QQuick related animation and scaling logic in a separate patch, this would be a candidate for that later change. I wanted to keep the client interface to a bare minimum to have a good layout as starting point. > > > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.h:44 > > +class ViewportHandlerClientQt : public QObject, public ViewportHandlerClient { > > + Q_OBJECT > > Maybe it makes sense moving the gesture recognition here? I do not have a strong opinion about that, but I think this should not be in this patch. > > > Source/WebKit2/UIProcess/qt/ViewportHandlerClientQt.h:52 > > + virtual void setContentPosition(const FloatPoint& positionInViewCoordinates); > > + virtual void setContentsScale(float scale, bool isInitialScale); > > one with s one without? Fixed.
Created attachment 161917 [details] Patch
(In reply to comment #13) > (From update of attachment 161907 [details]) > Attachment 161907 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13744117 Seems bogus as well. Looks like some EWS's are sick today :)
> I'm not sure putting content in the class name would improve the name. The term "content" is already used as a synonym for the CSS coordinate system. PageViewport might be a better name. > > > Source/WebKit2/UIProcess/ViewportHandler.h:79 > > > + FloatRect positionRangeForContentAtScale(float viewportScale) const; > > > > Isn't this name very Qt specific? > > Which part do you feel is Qt-specific? Do you have any suggestion for a better name? It has that name because of the QML (Flickable?) API. Maybe it is OK... maybe something with valid. > > I want to clean up the QQuick related animation and scaling logic in a separate patch, this would be a candidate for that later change. > I wanted to keep the client interface to a bare minimum to have a good layout as starting point. I see
(In reply to comment #17) > > I'm not sure putting content in the class name would improve the name. The term "content" is already used as a synonym for the CSS coordinate system. > > PageViewport might be a better name. > Is there something else in the UI process that could have a viewport besides the page? To put it another way: what does the "page" prefix give us when the term viewport is already defined as being a visual interface (window) to the page. In my opinion the name ViewportHandler describes the responsibilities pretty well. > > > > > Source/WebKit2/UIProcess/ViewportHandler.h:79 > > > > + FloatRect positionRangeForContentAtScale(float viewportScale) const; > > > > > > Isn't this name very Qt specific? > > > > Which part do you feel is Qt-specific? Do you have any suggestion for a better name? > > It has that name because of the QML (Flickable?) API. Maybe it is OK... maybe something with valid. Hmm, I'm not sure what you mean. But maybe validPositionRangeForContentAtScale?
> Is there something else in the UI process that could have a viewport besides the page? > > To put it another way: what does the "page" prefix give us when the term viewport is already defined as being a visual interface (window) to the page. > > In my opinion the name ViewportHandler describes the responsibilities pretty well. We do have FrameView so I think PageViewport fits well as you can only have the viewport to a page and not to any frame. I am also not sure how much info Handler adds, or wehther it is best to leave it out. Viewport seems too generic, it could be per frame or relate to OpenGL etc
(In reply to comment #19) > > Is there something else in the UI process that could have a viewport besides the page? > > > > To put it another way: what does the "page" prefix give us when the term viewport is already defined as being a visual interface (window) to the page. > > > > In my opinion the name ViewportHandler describes the responsibilities pretty well. > > We do have FrameView so I think PageViewport fits well as you can only have the viewport to a page and not to any frame. I am also not sure how much info Handler adds, or wehther it is best to leave it out. Viewport seems too generic, it could be per frame or relate to OpenGL etc I disagree. The problem I see with PageViewport is that the "ViewportHandler" is not actually the page viewport itself but it is rather an abstraction between the web process and the encapsulation of the platform specific view and page (client). Although handler might not be the best name it describes that this abstraction is rather a controller or a proxy than the viewport itself. To finish this dispute about how to name that freakin' thing, what about PageViewportController or PageViewportProxy?
(In reply to comment #20) > (In reply to comment #19) > > > Is there something else in the UI process that could have a viewport besides the page? > > > > > > To put it another way: what does the "page" prefix give us when the term viewport is already defined as being a visual interface (window) to the page. > > > > > > In my opinion the name ViewportHandler describes the responsibilities pretty well. > > > > We do have FrameView so I think PageViewport fits well as you can only have the viewport to a page and not to any frame. I am also not sure how much info Handler adds, or wehther it is best to leave it out. Viewport seems too generic, it could be per frame or relate to OpenGL etc > > I disagree. The problem I see with PageViewport is that the "ViewportHandler" is not actually the page viewport itself but it is rather an abstraction between the web process and the encapsulation of the platform specific view and page (client). > > Although handler might not be the best name it describes that this abstraction is rather a controller or a proxy than the viewport itself. > > To finish this dispute about how to name that freakin' thing, what about PageViewportController or PageViewportProxy? It is also not really a proxy as it is not just proxying one object from the other side. PageViewportController might be fine.
Created attachment 162044 [details] updated patch
Comment on attachment 162044 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=162044&action=review Getting there > Source/WebKit2/UIProcess/PageViewportController.h:45 > +// UPDATE DEFERRING (SUSPEND/RESUME) > +// ================================= > +// I would just remove those three lines... also please make sure the comment still makes sense > Source/WebKit2/UIProcess/PageViewportController.h:75 > +class PageViewportController { WTF_MAKE_NONCOPYABLE(PageViewportController); ? > Source/WebKit2/UIProcess/PageViewportController.h:132 > + float m_viewportScale; // Should always be cssScale * devicePixelRatio. m_effectiveScale ? > Source/WebKit2/UIProcess/PageViewportControllerClient.h:40 > + virtual void requestViewportUpdate() = 0; Looking at the header it is not so clear what the purpose of this method is > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:184 > +void PageViewportControllerClientQt::focusEditableArea(const QRectF& caretArea, const QRectF& targetArea) > +{ > + // This can only happen as a result of a user interaction. I still wish we can refactor out this code as I suggested (like in a separate patch) > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:322 > +void PageViewportControllerClientQt::setContentsPosition(const FloatPoint& positionInViewCoordinates) I think setContentsPosition(const FloatPoint& localPoint) would be ok > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:329 > +void PageViewportControllerClientQt::setContentsScale(float viewportScale, bool isInitialScale) setContentsScale(float localScale, bool treatAsInitialValue) > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h:47 > + why this newline? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h:57 > + virtual void setContentsRectToNearestValidBounds(); > + > + virtual void requestViewportUpdate(); > + > + virtual void didChangeContentsSize(); I think setContentsRectToNearestValidBounds is a bit too implementation specific. The client version of didChangeContentsSize should take care of this > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h:61 > + virtual void setPageViewportController(PageViewportController* handler) { m_viewportController = handler; } I think other clients (Geolocaton etc) are just using setController(...) { m_controller = } I even find that a bit cleaner http://opensource.apple.com/source/WebCore/WebCore-1298/dom/DeviceOrientationClient.h
Comment on attachment 162044 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=162044&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:84 > + m_rawAttributes.devicePixelRatio = m_devicePixelRatio; devicePixelRatio should not be gotten from the attributes anymore... It should actually be removed from them. It would be good to fix this soonish. It is supposed to be a platform value, not a calculated one. Maybe the funtion here can be removed all together
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 161902 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161902&action=review > > > > > Source/WebKit2/UIProcess/ViewportHandler.h:105 > > > + void updateViewportSize(const FloatSize& newSize); > > > + void updateVisibleContentsRect(const FloatRect& visibleContentsRect, float viewportScale, const FloatPoint& trajectoryVector = FloatPoint::zero()); > > > > FrameView seems to use adjust* naming. > adjust* sound ok. adjust is usually for stuff pulling the data from somewhere else. I would just call them setViewportSize and setVisibleContentsRect.
Created attachment 162259 [details] Patch
Comment on attachment 162259 [details] Patch Maybe you want someone else to look at it
(In reply to comment #27) > (From update of attachment 162259 [details]) > Maybe you want someone else to look at it Yes, Simon said he's going to look at it as well.
Comment on attachment 162259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162259&action=review I think this looks good. A few minor suggestions that can be easily done in follow-up changes if you decide to do them. I think I get an idea of the difficulty of finding the cut-off point, i.e. determining where to split the code between controller and the Qt specific client. The big bulk of the code remains in the Qt client though (570 LOC vs. ~250 LOC in the non-Qt controller), so I wonder how much this split is really going to buy us. I don't see the code becoming simpler. Unless of course it turns out that over time we can move more code to the controller. Anyway, I'm not objecting to the change. It's worth a try and it must've been difficult to do this fine split - hats off :) > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:860 > - m_viewportHandler->disconnect(); > + m_pageViewportControllerClient->disconnect(); I think that you can remove this code in the future. The scoped pointer will delete the object right afterwards, and the signal slot connections are automatically disconnected when the sender or receiver is destroyed. Also: Wouldn't this code crash if the webview is destructed before onComponentCmplete() is called? > Source/WebKit2/UIProcess/PageViewportController.cpp:38 > +bool fuzzyCompare(float a, float b, float epsilon) static missing here? > Source/WebKit2/UIProcess/PageViewportController.cpp:43 > +FloatPoint boundPosition(const FloatPoint minPosition, const FloatPoint& position, const FloatPoint& maxPosition) Ditto. > Source/WebKit2/UIProcess/PageViewportController.cpp:101 > +float PageViewportController::convertFromViewport(float value) const > +{ > + return value / m_devicePixelRatio; > +} > + > +float PageViewportController::convertToViewport(float value) const > +{ > + return value * m_devicePixelRatio; > +} Should these two trivial functions perhaps be inline in the class declaration? > Source/WebKit2/UIProcess/PageViewportController.cpp:236 > +void PageViewportController::suspendContent() > +{ > + if (m_hasSuspendedContent) > + return; > + > + m_hasSuspendedContent = true; > + m_webPageProxy->suspendActiveDOMObjectsAndAnimations(); > +} > + > +void PageViewportController::resumeContent() > +{ > + if (!m_rawAttributes.layoutSize.isEmpty() && m_rawAttributes.initialScale > 0) { > + m_hadUserInteraction = false; > + m_client->setContentsScale(convertToViewport(innerBoundedContentsScale(m_rawAttributes.initialScale)), /* isInitialScale */ true); > + m_rawAttributes.initialScale = -1; // Mark used. > + } > + > + m_client->didResumeContent(); > + > + if (!m_hasSuspendedContent) > + return; > + > + m_hasSuspendedContent = false; > + m_webPageProxy->resumeActiveDOMObjectsAndAnimations(); > +} It's a bit ugly that suspendContent() and resumeContent() are not really symmetric, i.e. resumeContent() does more than just undoing what suspendContent() did. On the caller side it looks strange, like in the UpdateDeferrer, where we call suspendContent() only if the constructor flag suggests to do so, but we unconditionally call resumeContent(). Makes me wonder if the suspendActiveDOMObjectAndAnimations() code should just move from here to the callers?
(In reply to comment #29) [snip] > The big bulk of the code remains in the Qt client though (570 LOC vs. ~250 LOC in the non-Qt controller), so I wonder how much this split is really going to buy us. I don't see the code becoming simpler. Unless of course it turns out that over time we can move more code to the controller. > Yes, as a first step I tried to establish the basic layout for the split. I think it should be possible to merge and move considerable pars of the focusEditableArea and the zoomToArea... functionality as well in follow-up patches and also change how qwebkitest accesses the viewport state data to be able to remove the proxying from the client. > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:860 > > - m_viewportHandler->disconnect(); > > + m_pageViewportControllerClient->disconnect(); > > I think that you can remove this code in the future. The scoped pointer will delete the object right afterwards, and the signal slot connections are automatically disconnected when the sender or receiver is destroyed. > > Also: Wouldn't this code crash if the webview is destructed before onComponentCmplete() is called? Fixed. > > > Source/WebKit2/UIProcess/PageViewportController.cpp:38 > > +bool fuzzyCompare(float a, float b, float epsilon) > > static missing here? > > > Source/WebKit2/UIProcess/PageViewportController.cpp:43 > > +FloatPoint boundPosition(const FloatPoint minPosition, const FloatPoint& position, const FloatPoint& maxPosition) > > Ditto. These helper functions are also needed in the translation unit of the client so they are declared in the header and defined here, they need external linkage. > > > Source/WebKit2/UIProcess/PageViewportController.cpp:101 > > +float PageViewportController::convertFromViewport(float value) const > > +{ > > + return value / m_devicePixelRatio; > > +} > > + > > +float PageViewportController::convertToViewport(float value) const > > +{ > > + return value * m_devicePixelRatio; > > +} > > Should these two trivial functions perhaps be inline in the class declaration? Right. > [snip] > It's a bit ugly that suspendContent() and resumeContent() are not really symmetric, i.e. resumeContent() does more than just undoing what suspendContent() did. On the caller side it looks strange, like in the UpdateDeferrer, where we call suspendContent() only if the constructor flag suggests to do so, but we > unconditionally call resumeContent(). Makes me wonder if the suspendActiveDOMObjectAndAnimations() code should just move from here to the callers? I think Jocelyn's work on fixing scaling artifacts during page transition should remove the need to rely on setting the initial scale when resuming.
Committed r127720: <http://trac.webkit.org/changeset/127720>
(In reply to comment #30) > (In reply to comment #29) > [snip] > > The big bulk of the code remains in the Qt client though (570 LOC vs. ~250 LOC in the non-Qt controller), so I wonder how much this split is really going to buy us. I don't see the code becoming simpler. Unless of course it turns out that over time we can move more code to the controller. > > > Yes, as a first step I tried to establish the basic layout for the split. I think it should be possible to merge and move considerable pars of the focusEditableArea and the zoomToArea... functionality as well in follow-up patches and also change how qwebkitest accesses the viewport state data to be able to remove the proxying from the client. Excellent, if you think that there's more that can be migrated over time, then it sounds like it's really worth it.
Comment on attachment 162259 [details] Patch Clearing flags from attachment.