Bug 110197

Summary: [EFL][WK2] Convert from device view size to UI view size only in EwkView.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: WebKit EFLAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, dglazkov, gyuyoung.kim, jturcotte, kenneth, lucas.de.marchi, mikhail.pozdnyakov, rakuco, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110298    
Bug Blocks: 110066, 110847    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
kenneth: review+, kenneth: commit-queue-
Patch for landing
none
patch for landing none

Description Dongseong Hwang 2013-02-19 03:02:59 PST
deviceScaleFactor is the property of device window, so only EwkView should deal with converting device view size to dip(device independent) view size and device scroll position to dip scroll position. It increases readability because we can regard view size and scroll position in other classes (e.g. WebView, PageViewportController, WebPage) except for EwkView as metrics in dip unit.

Currently there is a bug. WebView::updateViewportSize() calls DrawingAreaProxy::setVisibleContentsRect() with device view size and device scroll position while PageViewportController calls setVisibleContentsRect() with the dip metrics.
Comment 1 Dongseong Hwang 2013-02-19 03:11:13 PST
Created attachment 189040 [details]
Patch
Comment 2 Dongseong Hwang 2013-02-19 03:12:19 PST
I introduced dipSize and dipScrollPosition in EwkView as following WebViewImpl::dipSize() in chromium. WDYT? :)
Comment 3 Kenneth Rohde Christiansen 2013-02-19 04:48:26 PST
Comment on attachment 189040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:486
> +IntSize EwkView::dipSize() const
> +{
> +    LayoutSize dipSize = m_size;
> +    dipSize.scale(1 / deviceScaleFactor());
> +    return roundedIntSize(dipSize);
> +}
> +
> +IntSize EwkView::size() const
> +{
> +    return roundedIntSize(m_size);
> +}

im not sure I am too fount of these names, but I guess they could work.

What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&)
Comment 4 Build Bot 2013-02-19 07:57:44 PST
Comment on attachment 189040 [details]
Patch

Attachment 189040 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16616717

New failing tests:
inspector/styles/stylesheet-tracking.html
Comment 5 Dongseong Hwang 2013-02-19 18:34:03 PST
(In reply to comment #3)
> (From update of attachment 189040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review
> im not sure I am too fount of these names, but I guess they could work.
> 
> What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&)

That's reasonable idea, but I don't want size() is public method. I mean that I want clients (e.g. WebView) to query only dip scale and position of EwkView. WDYT?
Comment 6 Dongseong Hwang 2013-02-19 20:01:42 PST
Created attachment 189225 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2013-02-22 07:05:04 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 189040 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review
> > im not sure I am too fount of these names, but I guess they could work.
> > 
> > What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&)
> 
> That's reasonable idea, but I don't want size() is public method. I mean that I want clients (e.g. WebView) to query only dip scale and position of EwkView. WDYT?

Well shouldn't size then not just return toDIP(m_size) or so, or dont many it query, but have us set the size instead.

EwkView is very EFL specific, so it makes sense to have like a nativeSize() or similar, which works in the same domain as EFL.

Then you can call ->setSize(toDIP(nativeSize())) if you need to.
Comment 8 Dongseong Hwang 2013-02-25 22:09:17 PST
Created attachment 190204 [details]
Patch
Comment 9 Dongseong Hwang 2013-02-25 22:12:34 PST
(In reply to comment #7)
> Well shouldn't size then not just return toDIP(m_size) or so, or dont many it query, but have us set the size instead.
> 
> EwkView is very EFL specific, so it makes sense to have like a nativeSize() or similar, which works in the same domain as EFL.
> 
> Then you can call ->setSize(toDIP(nativeSize())) if you need to.

WebView needs to query both native size and dip size from EwkView because recent change of WebView and EwkView.
So EwkView now has two api: size() and nativeSize(). How about it?
Comment 10 Dongseong Hwang 2013-02-25 22:47:00 PST
Created attachment 190210 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2013-02-26 06:43:03 PST
Comment on attachment 190210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190210&action=review

LGTM

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:486
> +IntSize EwkView::nativeSize() const
> +{
> +    return roundedIntSize(m_nativeSize);
> +}

m_nativeSize is already an int! at least it is set to... so rounding should do nothing
Comment 12 Dongseong Hwang 2013-03-03 17:03:25 PST
Created attachment 191154 [details]
Patch
Comment 13 Dongseong Hwang 2013-03-03 17:09:47 PST
(In reply to comment #11)
> (From update of attachment 190210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190210&action=review
> 
> LGTM

Thank you for review :)

> m_nativeSize is already an int! at least it is set to... so rounding should do nothing

Good point. now m_nativeSize is IntSize.
Comment 14 Kenneth Rohde Christiansen 2013-03-04 01:04:03 PST
Comment on attachment 191154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review

LGTM

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481
> +    // WebPage expects a size in UI units, and not raw device units.
> +    FloatSize dipSize = m_nativeSize;

Maybe cssSize is more understandable?
Comment 15 Dongseong Hwang 2013-03-04 02:48:26 PST
Comment on attachment 191154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481
>> +    FloatSize dipSize = m_nativeSize;
> 
> Maybe cssSize is more understandable?

Thank you for review!
I think cssSize is m_nativeSize / (pageScaleFactor * deviceScaleFactor), and dipSize is m_nativeSize  / pageScaleFactor. so I think that dipSize is more relevant.
How do you think? :)
Comment 16 Dongseong Hwang 2013-03-04 14:48:32 PST
Created attachment 191317 [details]
Patch
Comment 17 Dongseong Hwang 2013-03-04 14:50:40 PST
(In reply to comment #15)
> (From update of attachment 191154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481
> >> +    FloatSize dipSize = m_nativeSize;
> > 
> > Maybe cssSize is more understandable?
> 
> Thank you for review!
> I think cssSize is m_nativeSize / (pageScaleFactor * deviceScaleFactor), and dipSize is m_nativeSize  / pageScaleFactor. so I think that dipSize is more relevant.
> How do you think? :)

I changed from dipSize to uiSize because UI is more general acronym.
please refer to http://trac.webkit.org/wiki/ScalesAndZooms
Comment 18 WebKit Review Bot 2013-03-04 16:27:25 PST
Comment on attachment 191317 [details]
Patch

Attachment 191317 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16907081

New failing tests:
inspector/debugger/watch-expressions-panel-switch.html
inspector/debugger/set-breakpoint.html
Comment 19 Dongseong Hwang 2013-03-04 21:41:23 PST
(In reply to comment #18)
> (From update of attachment 191317 [details])
> Attachment 191317 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://webkit-commit-queue.appspot.com/results/16907081
> 
> New failing tests:
> inspector/debugger/watch-expressions-panel-switch.html
> inspector/debugger/set-breakpoint.html

It is flaky because this patch is not related to chromium.
Comment 20 Benjamin Poulain 2013-03-05 16:28:52 PST
Comment on attachment 191317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191317&action=review

I don't mind this. Okay for WebKit2.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:484
> +IntSize EwkView::size() const
> +{
> +    // WebPage expects a size in UI units, and not raw device units.
> +    FloatSize uiSize = m_nativeSize;
> +    uiSize.scale(1 / deviceScaleFactor());
> +    return roundedIntSize(uiSize);
> +}

I would compute this when changing the value. Then in EwkView::size() you just return drawingArea->size().

Or make the function private.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:489
> +IntSize EwkView::nativeSize() const
> +{
> +    return m_nativeSize;
> +}

This name is confusing.

A size is a dimension in a certain coordinate system. A "native" size does not mean anything.
Comment 21 Dongseong Hwang 2013-03-05 18:01:04 PST
Created attachment 191623 [details]
Patch
Comment 22 Dongseong Hwang 2013-03-05 18:13:32 PST
(In reply to comment #20)
> (From update of attachment 191317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191317&action=review
> 
> I don't mind this. Okay for WebKit2.

Thank you for review.

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:484
> > +IntSize EwkView::size() const
> > +{
> > +    // WebPage expects a size in UI units, and not raw device units.
> > +    FloatSize uiSize = m_nativeSize;
> > +    uiSize.scale(1 / deviceScaleFactor());
> > +    return roundedIntSize(uiSize);
> > +}
> 
> I would compute this when changing the value. Then in EwkView::size() you just return drawingArea->size().

That's good idea but we have problem if EwkView::size() returns drawingArea->size(). The initialize list of DrawingAreaProxy assigns m_size = m_size.

DrawingAreaProxy::DrawingAreaProxy(DrawingAreaType type, WebPageProxy* webPageProxy)
    : m_type(type)
    , m_webPageProxy(webPageProxy)
    , m_size(webPageProxy->viewSize())

> Or make the function private.

WebView needs to know both deviceSize and size.

> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:489
> > +IntSize EwkView::nativeSize() const
> > +{
> > +    return m_nativeSize;
> > +}
> 
> This name is confusing.
> A size is a dimension in a certain coordinate system. A "native" size does not mean anything.

I agree. I changed from native to device to match https://trac.webkit.org/wiki/ScalesAndZooms
Comment 23 Kenneth Rohde Christiansen 2013-03-06 00:36:53 PST
Comment on attachment 191623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191623&action=review

r=me with the change (signed off by Benjamin)

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:466
> -void EwkView::setSize(const IntSize& size)
> +void EwkView::setSize(const IntSize& deviceSize)

This should be renamed to setDeviceSize then as the getter is called deviceSize()
Comment 24 Dongseong Hwang 2013-03-06 01:01:35 PST
Created attachment 191677 [details]
Patch for landing
Comment 25 Dongseong Hwang 2013-03-06 01:03:05 PST
Created attachment 191678 [details]
patch for landing
Comment 26 Kenneth Rohde Christiansen 2013-03-06 01:05:07 PST
Comment on attachment 191677 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=191677&action=review

> Source/WebKit2/ChangeLog:34
> +
> +2013-03-03  Huang Dongsung  <luxtella@company100.net>
> +
> +        [EFL][QT][WK2] Turn on ApplyPageScaleFactorInCompositor always.
> +        https://bugs.webkit.org/show_bug.cgi?id=111056
> +

what is going on here??? multiple patches?

> LayoutTests/ChangeLog:5
> +
> +        [EFL][QT][WK2] Turn on ApplyPageScaleFactorInCompositor always.
> +        https://bugs.webkit.org/show_bug.cgi?id=111056
> +

what?
Comment 27 WebKit Review Bot 2013-03-06 01:37:24 PST
Comment on attachment 191678 [details]
patch for landing

Clearing flags on attachment: 191678

Committed r144899: <http://trac.webkit.org/changeset/144899>
Comment 28 WebKit Review Bot 2013-03-06 01:37:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Dongseong Hwang 2013-03-06 07:25:18 PST
(In reply to comment #26)
> (From update of attachment 191677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191677&action=review
> 
> what is going on here??? multiple patches?
> 
> what?

the next to last patch is mistake. I think "./webkit-patch land-safely" has a bug.
Don't worry :) r144899 is landed using the last patch.