Bug 110847

Created attachment 190211 [details] Patch

I explain more detail to help review. Currently there are two clients to call setPagePosition 1. call setPagePosition with css position void WebView::pageDidRequestScroll(const IntPoint& position) { ... m_ewkView->setPagePosition(FloatPoint(position)); m_ewkView->scheduleUpdateDisplay(); } 2. call setPagePosition with scroll position in device pixel. void PageViewportControllerClientEfl::setViewportPosition(const WebCore::FloatPoint& contentsPoint) { m_contentPosition = contentsPoint; FloatPoint pos(contentsPoint); pos.scale(m_view->pageScaleFactor(), m_view->pageScaleFactor()); pos.scale(m_view->deviceScaleFactor(), m_view->deviceScaleFactor()); m_view->setPagePosition(pos); ... } pagePosition ditto.

Comment on attachment 190211 [details] Patch The patch per se looks good and makes code cleaner. My question is: shouldn't we go further and convert view size to css units as well, otherwise it might not look consistent?

Comment on attachment 190211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190211&action=review > Source/WebKit2/ChangeLog:14 > + This patch makes all code use setPagePosition() and pagePosition() with css > + position. I am not sure that is the best solution, as when panning and going pinch etc all the values will be in user coordinates. I would prefer it the other way around. It seems more natural

Thank you for review! :) (In reply to comment #3) > (From update of attachment 190211 [details]) > The patch per se looks good and makes code cleaner. My question is: shouldn't we go further and convert view size to css units as well, otherwise it might not look consistent? Your concern is very reasonable. I agree. I decided that EwkView stores css position because of following reasons: 1. There is no code that stores DIP scroll position (= css position * page scale factor) or device scroll position (= css position * page scale factor * device scale factor). IMHO, scroll position in DIP or device pixel units is a bit weird. 2. css position is a little bit effective for clients. There are only three clients that use EwkView::pagePosition(). Two clients can be checked in this patch. and last one is WebView::updateViewportSize(). We don't need to compute float multiplication. void WebView::updateViewportSize() { ... m_page->drawingArea()->setVisibleContentsRect(FloatRect(m_ewkView->pagePosition(), m_ewkView->size()), FloatPoint()); } However, I agree that it is not consistent with size. Do you think which unit we should use for scroll position: css, DIP, or device pixel units? (In reply to comment #4) > (From update of attachment 190211 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190211&action=review > > > Source/WebKit2/ChangeLog:14 > > + This patch makes all code use setPagePosition() and pagePosition() with css > > + position. > > I am not sure that is the best solution, as when panning and going pinch etc all the values will be in user coordinates. I would prefer it the other way around. It seems more natural I don'y fully catch up what you mean. Do you think EwkView should store scroll position in user coordinates? and user coordinates may be device pixel units?

> Your concern is very reasonable. I agree. It is very simple, The user interacts with the contents (moves the finger, does a pinch gesture) using UI units. Having to convert that to CSS units and making sure things are pixel aligned etc, seems complex and easy to go wrong

(In reply to comment #6) > > Your concern is very reasonable. I agree. > > It is very simple, The user interacts with the contents (moves the finger, does a pinch gesture) using UI units. Having to convert that to CSS units and making sure things are pixel aligned etc, seems complex and easy to go wrong Yes, you and Mikhail are right! I'll change EwkView to keep scroll position in UI units. :)

Created attachment 191314 [details] Patch

Created attachment 191316 [details] Patch

Comment on attachment 191316 [details] Patch LGTM

(In reply to comment #10) > (From update of attachment 191316 [details]) > LGTM Thanks for review :) benjamin, could you review?

Comment on attachment 191316 [details] Patch What about the tests? I don't really mind this. Okay for WebKit2.

Created attachment 192150 [details] Patch for Landing

Comment on attachment 192150 [details] Patch for Landing Clearing flags on attachment: 192150 Committed r145185: <http://trac.webkit.org/changeset/145185>

All reviewed patches have been landed. Closing bug.