Bug 97220 – [Qt] Unify the PageViewportController<->Client interface regarding positions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt] Unify the PageViewportController<->Client interface regarding positions

[Jocelyn Turcotte]

Created attachment 164925 [details]
Patch

[Andras Becsi]

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.

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

Created attachment 164930 [details]
Patch

Bring back missing scale adjustments that were applied in a further patch.

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

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

[Jocelyn Turcotte]

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

[WebKit Review Bot]

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

[Jocelyn Turcotte]

Created attachment 165620 [details]
Patch

Patch for landing

[WebKit Review Bot]

Comment on attachment 165620 [details]
Patch

Clearing flags on attachment: 165620

Committed r129515: <http://trac.webkit.org/changeset/129515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.