Bug 110197 – [EFL][WK2] Convert from device view size to UI view size only in EwkView.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 189040 [details]
Patch

[Dongseong Hwang]

I introduced dipSize and dipScrollPosition in EwkView as following WebViewImpl::dipSize() in chromium. WDYT? :)

[Kenneth Rohde Christiansen]

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&)

[Build Bot]

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

[Dongseong Hwang]

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

[Dongseong Hwang]

Created attachment 189225 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Dongseong Hwang]

Created attachment 190204 [details]
Patch

[Dongseong Hwang]

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

[Dongseong Hwang]

Created attachment 190210 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Dongseong Hwang]

Created attachment 191154 [details]
Patch

[Dongseong Hwang]

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

[Kenneth Rohde Christiansen]

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?

[Dongseong Hwang]

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? :)

[Dongseong Hwang]

Created attachment 191317 [details]
Patch

[Dongseong Hwang]

(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

[WebKit Review Bot]

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

[Dongseong Hwang]

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

[Benjamin Poulain]

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.

[Dongseong Hwang]

Created attachment 191623 [details]
Patch

[Dongseong Hwang]

(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

[Kenneth Rohde Christiansen]

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

[Dongseong Hwang]

Created attachment 191677 [details]
Patch for landing

[Dongseong Hwang]

Created attachment 191678 [details]
patch for landing

[Kenneth Rohde Christiansen]

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
> +
> [phone]  Huang Dongsung  <[e-mail]>
> +
> +        [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?

[WebKit Review Bot]

Comment on attachment 191678 [details]
patch for landing

Clearing flags on attachment: 191678

Committed r144899: <http://trac.webkit.org/changeset/144899>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dongseong Hwang]

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