[Qt] Unify the PageViewportController<->Client interface regarding positions
Created attachment 164925 [details] Patch
Comment on attachment 164925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164925&action=review > Source/WebKit2/ChangeLog:10 > + - Make sure that all positions represent the viewport relatively to the contents > + rather than the other way around If I'm not misunderstanding you here, I find this aspect of the patch a bit odd, since all the interaction with the content actually drags the content thus it seems more natural to me to look at the viewport as an inertial frame of reference wherein the contents are positioned / scaled, instead of the viewport being relatively positioned to the contents. More so because the incoming events are all relative to the viewport.
(In reply to comment #2) > If I'm not misunderstanding you here, I find this aspect of the patch a bit odd, since all the interaction with the content actually drags the content thus it seems more natural to me to look at the viewport as an inertial frame of reference wherein the contents are positioned / scaled, instead of the viewport being relatively positioned to the contents. More so because the incoming events are all relative to the viewport. I agree for Qt's case, we're dragging an additional item behind, but that is an implementation detail. The interface should be nice to the implementation, but I think that it's more important that it is concise, especially across ports, and I think that it is not possible to use "scale-applied" coordinates everywhere. So this one felt nicer to me, and makes it more human readable than negative coordinates for the content item e.g. -134,-3000.
Created attachment 164930 [details] Patch Bring back missing scale adjustments that were applied in a further patch.
(In reply to comment #3) > (In reply to comment #2) > > If I'm not misunderstanding you here, I find this aspect of the patch a bit odd, since all the interaction with the content actually drags the content thus it seems more natural to me to look at the viewport as an inertial frame of reference wherein the contents are positioned / scaled, instead of the viewport being relatively positioned to the contents. More so because the incoming events are all relative to the viewport. > > I agree for Qt's case, we're dragging an additional item behind, but that is an implementation detail. The interface should be nice to the implementation, but I think that it's more important that it is concise, especially across ports, and I think that it is not possible to use "scale-applied" coordinates everywhere. So this one felt nicer to me, and makes it more human readable than negative coordinates for the content item e.g. -134,-3000. In fact, this is a more general discrepancy between the model of touch-based interaction where the content is dragged, and the mouse-based perspective where dragging the scrollbar moves the viewport. In any case I'm not agains the change, it makes the interface consistent, I was just wondering. It also seems to depend on your other changes since it does not apply.
Comment on attachment 164930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164930&action=review I hope this is well tested :-) > Source/WebKit2/UIProcess/PageViewportController.cpp:127 > + FloatRect endPosRange = positionRangeForViewportAtScale(m_effectiveScale); hmm your other patch removed this
(In reply to comment #6) > (From update of attachment 164930 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164930&action=review > > I hope this is well tested :-) I did my best :) > > > Source/WebKit2/UIProcess/PageViewportController.cpp:127 > > + FloatRect endPosRange = positionRangeForViewportAtScale(m_effectiveScale); > > hmm your other patch removed this Yep I needed the behavior to be the same everywhere to be able to have one function to do it. Ultimately this is all needed to be able to handle position+scale directly in the interface rather than complete rects. I needed this to be able to keep a position pending valid even if the contents size or the scale is changed while the web process rendering is disabled between pages.
Comment on attachment 164930 [details] Patch Rejecting attachment 164930 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: 2/UIProcess/PageViewportControllerClient.h patching file Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp Hunk #1 FAILED at 108. 1 out of 8 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp.rej patching file Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/14031119
Created attachment 165620 [details] Patch Patch for landing
Comment on attachment 165620 [details] Patch Clearing flags on attachment: 165620 Committed r129515: <http://trac.webkit.org/changeset/129515>
All reviewed patches have been landed. Closing bug.