Bug 111405

Summary: [EFL][Qt][WK2] Make PageViewportController more readable.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: WebKit2Assignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abecsi, allan.jensen, cmarcelo, gyuyoung.kim, jturcotte, kenneth, mcatanzaro, menard, mikhail.pozdnyakov, noam, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111401    
Bug Blocks: 110066    
Attachments:
Description Flags
Patch jturcotte: review-, jturcotte: commit-queue-

Dongseong Hwang
Reported 2013-03-04 21:31:21 PST
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.
Attachments
Patch (18.68 KB, patch)
2013-03-04 21:38 PST, Dongseong Hwang 		jturcotte: review-
jturcotte: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2013-03-04 21:38:44 PST
Created attachment 191398 [details] Patch
Jocelyn Turcotte
Comment 2 2013-03-05 03:34:30 PST
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 3 2013-03-05 04:18:47 PST
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
Comment 4 2013-03-05 04:20:22 PST
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
Comment 5 2017-03-11 10:33:08 PST
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.
Note You need to log in before you can comment on or make changes to this bug.