Bug 91257 – [WK2] Make [Qt]ViewportHandler cross platform

• Component: WebKit2
• Status: RESOLVED FIXED

[Noam Rosenthal]

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.

[Noam Rosenthal]

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

[Andras Becsi]

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

[Noam Rosenthal]

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

[Kenneth Rohde Christiansen]

I very much support this change! but I am also on vacation :-)

[Andras Becsi]

Created attachment 161723 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Andras Becsi]

Created attachment 161902 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 161902 [details]
proposed patch

Attachment 161902 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13741146

[Kenneth Rohde Christiansen]

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.

[Andras Becsi]

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

[Andras Becsi]

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

[Andras Becsi]

Created attachment 161907 [details]
proposed patch

Applied naming suggestions where appropriate.

[Build Bot]

Comment on attachment 161907 [details]
proposed patch

Attachment 161907 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13744117

[Andras Becsi]

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

[Andras Becsi]

Created attachment 161917 [details]
Patch

[Andras Becsi]

(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 :)

[Kenneth Rohde Christiansen]

> 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

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

> 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

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

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

[Andras Becsi]

Created attachment 162044 [details]
updated patch

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Jocelyn Turcotte]

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

[Andras Becsi]

Created attachment 162259 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 162259 [details]
Patch

Maybe you want someone else to look at it

[Andras Becsi]

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

[Simon Hausmann]

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?

[Andras Becsi]

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

[Andras Becsi]

Committed r127720: <http://trac.webkit.org/changeset/127720>

[Simon Hausmann]

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

[Andras Becsi]

Comment on attachment 162259 [details]
Patch

Clearing flags from attachment.