Bug 110847 - [EFL] EwkView should keep css position instead of scroll position in device pixel.
Summary: [EFL] EwkView should keep css position instead of scroll position in device p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on: 110197 110298
Blocks: 109033 110066
  Show dependency treegraph
 
Reported: 2013-02-25 22:34 PST by Dongseong Hwang
Modified: 2013-03-07 23:32 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2013-02-25 22:49 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2013-03-04 14:40 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2013-03-04 14:44 PST, Dongseong Hwang
kenneth: review+
Details | Formatted Diff | Diff
Patch for Landing (4.67 KB, patch)
2013-03-07 22:56 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2013-02-25 22:34:29 PST
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.
Comment 1 Dongseong Hwang 2013-02-25 22:49:08 PST
Created attachment 190211 [details]
Patch
Comment 2 Dongseong Hwang 2013-02-25 23:30:55 PST
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 3 Mikhail Pozdnyakov 2013-02-26 05:02:33 PST
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 4 Kenneth Rohde Christiansen 2013-02-26 05:24:50 PST
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
Comment 5 Dongseong Hwang 2013-03-03 17:30:23 PST
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?
Comment 6 Kenneth Rohde Christiansen 2013-03-04 00:59:18 PST
> 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
Comment 7 Dongseong Hwang 2013-03-04 02:55:01 PST
(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. :)
Comment 8 Dongseong Hwang 2013-03-04 14:40:40 PST
Created attachment 191314 [details]
Patch
Comment 9 Dongseong Hwang 2013-03-04 14:44:45 PST
Created attachment 191316 [details]
Patch
Comment 10 Kenneth Rohde Christiansen 2013-03-05 01:10:23 PST
Comment on attachment 191316 [details]
Patch

LGTM
Comment 11 Dongseong Hwang 2013-03-05 01:45:51 PST
(In reply to comment #10)
> (From update of attachment 191316 [details])
> LGTM

Thanks for review :)

benjamin, could you review?
Comment 12 Benjamin Poulain 2013-03-05 16:30:39 PST
Comment on attachment 191316 [details]
Patch

What about the tests?

I don't really mind this. Okay for WebKit2.
Comment 13 Dongseong Hwang 2013-03-07 22:56:12 PST
Created attachment 192150 [details]
Patch for Landing
Comment 14 WebKit Review Bot 2013-03-07 23:32:10 PST
Comment on attachment 192150 [details]
Patch for Landing

Clearing flags on attachment: 192150

Committed r145185: <http://trac.webkit.org/changeset/145185>
Comment 15 WebKit Review Bot 2013-03-07 23:32:15 PST
All reviewed patches have been landed.  Closing bug.