Bug 111405 – [EFL][Qt][WK2] Make PageViewportController more readable.

• Component: WebKit2
• Status: RESOLVED WONTFIX

[Dongseong Hwang]

Refactor PageViewportController to reveal subtle intention handling scaling and
scrolling more explicitly.

Note we only use pending mechanism when PageViewportController is the source of
scaling or scrolling, and didRenderFrame() plays a role to notify time to flush
pending variables to the client.

1. Scaling
There are two kind sources of scaling events: PageViewportController itself and
the client (a.k.a the viewport).

If the client is the source, we must change the scale of PageViewportController
and WebCore immediately.

If PageViewportController is the source, we pend to sychronize the scale with
the client until receiving didRenderFrame() call, while we notify WebCore
immediately. It is because it is a special case in which we concern about
performance: setPageScaleFactorWithPendingChange() and
updateMinimumScaleToFit().

One big change is that didRenderFrame() does not call WebPageProxy::scalePage()
because didRenderFrame() is time to sychronize the scale of
PageViewportController with the client. If we call WebPageProxy::scalePage() in
didRenderFrame(), we will receive redundant didRenderFrame() call one more time.

2. Scrolling
There are three kind sources of scrolling events: PageViewportController itself,
the client and WebCore.

If the client is the source, we must change the scroll position of
PageViewportController and WebCore immediately.

If PageViewportController is the source, we pend to sychronize the scroll
position with the client until receiving didRenderFrame() call, while we notify
WebCore immediately. It is because it is a special case in which we concern
about performance: didCommitLoad() and the special case of pageDidRequestScroll().

If WebCore is the source, PageViewportController::pageDidRequestScroll() is
called. We must change the scroll position of PageViewportController and the
client  immediately.

There is one big changes: PageViewportControllerClient::setViewportPosition()
does not call PageViewportController::didChangeContentsVisibility().
didChangeContentsVisibility() is called when the client is the source of scaling
or scrolling events and setViewportPosition() is called when
PageViewportController want to syncronize the scroll position with the client,
so setViewportPosition() must not call didChangeContentsVisibility().
I introduced PageViewportControllerClientQt::m_isViewportPositionSychronizingWithPageViewportController
to achieve above requirement.

In addition, I introduced m_changingViewportAttributes to not change the scale
and scroll position during changingViewportAttributes. Currently,
PageViewportController::didChangeViewportAttributes() overuses
m_pendingPositionChange and m_pendingScaleChange for the purpose of
m_changingViewportAttributes.

[Dongseong Hwang]

Created attachment 191398 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 191398 [details]
Patch

Is there any bug caused by the current code?
If so, please have separate bugs for each of them and only fix those.

The code is fine, it doesn't need refactoring.

[Jocelyn Turcotte]

Comment on attachment 191398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191398&action=review

Let me address my concerns individually as well, I'm not being fair.

> Source/WebKit2/UIProcess/PageViewportController.cpp:361
> +bool PageViewportController::setContentsPosition(const WebCore::FloatPoint& pos)

The introduction of this method makes the code harder to read.
- From their use points, it is unclear if it has any effect on the client or the web process
- Given the above ambiguity, the use of a bool feels like "it failed to be applied", which isn't the case, it means the value is already there.

> Source/WebKit2/UIProcess/PageViewportController.h:94
> +    void setPageScaleFactorWithPendingChange(float);
> +    void setContentsPositionWithPendingChange(const WebCore::FloatPoint&);

I don't see why you need to rename those methods. The "WithPendingChange" and the end especially, "AfterRenderingContents" feeled clearer to me.

> Source/WebKit2/UIProcess/PageViewportController.h:120
> +    bool m_changingViewportAttributes;

This flag should be named according to its effect rather than the reason it happened to avoid it being abused, like you said.

> Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:329
> +    // See the comment in setViewportPosition().

This comment costs more than it gives, please remove it. Anybody's reflex should be to search for references of the variable anyway.

> Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:511
> +    // See the comment in setViewportPosition().

ditto

> Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h:142
> +    bool m_isViewportPositionSychronizingWithPageViewportController;

This should also name the effects rather than the source of the event.

[Andras Becsi]

View in context: https://bugs.webkit.org/attachment.cgi?id=191398&action=review

I'm not convinced either that this would be an improvement.
If you try to fix bugs, please fix them one-by-one.

> Source/WebKit2/UIProcess/PageViewportController.h:98
> -    void applyScaleAfterRenderingContents(float scale);
> -    void applyPositionAfterRenderingContents(const WebCore::FloatPoint& pos);
> +
> +    // Following two method must be called when PageViewportController is the source of scaling or scrolling event.
> +    // We pend to synchronize scale and scroll values with clients because it takes time until WebCore receives the event and responses didRenderFrame().
> +    void setPageScaleFactorWithPendingChange(float);
> +    void setContentsPositionWithPendingChange(const WebCore::FloatPoint&);
> +
>      bool updateMinimumScaleToFit(bool userInitiatedUpdate);
> +    bool setPageScaleFactor(float);
> +    bool setContentsPosition(const WebCore::FloatPoint&);

I'm not sure, that this would make it more readable.
My feeling is that it becomes more complex than before and I have concerns that the subtle behavioral changes might reintroduce issues that have already been fixed.

> Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:323
>  void PageViewportControllerClientQt::setViewportPosition(const FloatPoint& contentsPoint)
>  {
>      QPointF newPosition((m_pageItem->position() + QPointF(contentsPoint)) * m_pageItem->contentsScale());
> -    // The contentX and contentY property changes trigger a visible rect update.
> +    // Prevent a visible rect update due to the contentX and contentY property changes, because this method is called when PageViewportController synchronizes the viewport position to client.
> +    TemporaryChange<bool> protector(m_isViewportPositionSychronizingWithPageViewportController, true);

I suspect that this would reintroduce https://bugs.webkit.org/show_bug.cgi?id=108337.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.