Bug 110847 – [EFL] EwkView should keep css position instead of scroll position in device pixel.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

There are bugs that some code expect that EwkView::pagePosition() returns css
position while others expect that it returns scroll position in device pixel.
In addition, some code call EwkView::setPagePosition() with css position while
others call it with scroll position in device pixel.

This patch makes all code use setPagePosition() and pagePosition() with css
position.

[Dongseong Hwang]

Created attachment 190211 [details]
Patch

[Dongseong Hwang]

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.

[Mikhail Pozdnyakov]

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?

[Kenneth Rohde Christiansen]

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

[Dongseong Hwang]

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?

[Kenneth Rohde Christiansen]

> 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

[Dongseong Hwang]

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

[Dongseong Hwang]

Created attachment 191314 [details]
Patch

[Dongseong Hwang]

Created attachment 191316 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 191316 [details]
Patch

LGTM

[Dongseong Hwang]

(In reply to comment #10)
> (From update of attachment 191316 [details])
> LGTM

Thanks for review :)

benjamin, could you review?

[Benjamin Poulain]

Comment on attachment 191316 [details]
Patch

What about the tests?

I don't really mind this. Okay for WebKit2.

[Dongseong Hwang]

Created attachment 192150 [details]
Patch for Landing

[WebKit Review Bot]

Comment on attachment 192150 [details]
Patch for Landing

Clearing flags on attachment: 192150

Committed r145185: <http://trac.webkit.org/changeset/145185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.