Bug 110753 – [WK2][EFL] WebView should own view size

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

[Mikhail Pozdnyakov]

At the moment the view size is contained within EwkView class and WebView has to call EwkView method to know its size which is wrong.

[Mikhail Pozdnyakov]

Created attachment 190235 [details]
WIP

[Mikhail Pozdnyakov]

Created attachment 190241 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 190241 [details]
patch

LGTM

[Mikhail Pozdnyakov]

Created attachment 191685 [details]
patch v2

rebased

[Kenneth Rohde Christiansen]

Comment on attachment 191685 [details]
patch v2

So, I would say that these should take values in UI units (ie not physical ones) so it should be able to test this by setting setDeviceScaleFactor somehow.

Can we have a unit test?

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 191685 [details])
> So, I would say that these should take values in UI units (ie not physical ones) so it should be able to test this by setting setDeviceScaleFactor somehow.
> 
> Can we have a unit test?

I'd say layout tests would be more helpful there rather than unit tests,   thing to be tested is how page is looking if device scale factor is changed.

[Dongseong Hwang]

Comment on attachment 191685 [details]
patch v2

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

> Source/WebKit2/UIProcess/efl/WebView.cpp:282
> +    return size();

LGTM without one point.

It should return the size in ui units, because WebView now deal with m_size in device units.
FloatSize dipSize(m_size);
dipSize.scale(1 / page()->deviceScaleFactor());

[Dongseong Hwang]

Comment on attachment 191685 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/efl/EwkView.h:134
> +    WebCore::IntSize size() const;

Could you rebase to upstream?
Currently, there is two methods: size() and deviceSize(). ui units and device units respectively.

Your patch moves role dealing with device size to WebView. and you need to reserve carefully the behavior on each sites using either two methods.

[Mikhail Pozdnyakov]

Created attachment 192891 [details]
patch v3

Rebased. Took comments from Dongsung Huang into consideration - thanks.

[Dongseong Hwang]

Comment on attachment 192891 [details]
patch v3

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

LGTM. I add some comments.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:471
>  IntSize EwkView::size() const

I think size() and deviceSize() can be private now.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1104
> +        if (WKPageUseFixedLayout(self->wkPage()))

It is not needed because WebView::setSize already call WebView::updateViewportSize().

> Source/WebKit2/UIProcess/efl/WebView.cpp:-199
> -    if (m_page->useFixedLayout()) {

Could you explain why this code is moved to EwkView::handleEvasObjectCalculate?
Previously if there is pageViewportController, we skipped notifying to drawing area, but now the behavior is changed.

> Source/WebKit2/UIProcess/efl/WebView.cpp:527
> +inline WebCore::FloatSize WebView::dipSize() const

How about uiSize because we already used uiSize as a local variable in EwkView.

[Mikhail Pozdnyakov]

Comment on attachment 192891 [details]
patch v3

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1104
>> +        if (WKPageUseFixedLayout(self->wkPage()))
> 
> It is not needed because WebView::setSize already call WebView::updateViewportSize().

WebView is not aware of pageViewportController(), so it is needed.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:-199
>> -    if (m_page->useFixedLayout()) {
> 
> Could you explain why this code is moved to EwkView::handleEvasObjectCalculate?
> Previously if there is pageViewportController, we skipped notifying to drawing area, but now the behavior is changed.

Well actually we notified drawing area, as viewportcontroller invokes drawingArea()->setSize(roundedIntSize(newSize), IntSize()); plus it updates its internal state.

The reason for moving is fixing of API layering: WebView should not access viewport controller directly.

>> Source/WebKit2/UIProcess/efl/WebView.cpp:527
>> +inline WebCore::FloatSize WebView::dipSize() const
> 
> How about uiSize because we already used uiSize as a local variable in EwkView.

mmm.. I'd say dip size is clearer (ui size might mean that page scale factor is also taken for calculation).

[Mikhail Pozdnyakov]

Created attachment 193135 [details]
patch v4

Made EwkView size getters private as was pointed out by Dongsung Huang.

[Kenneth Rohde Christiansen]

Comment on attachment 193135 [details]
patch v4

LGTM

[Kenneth Rohde Christiansen]

Ping owner review/sign off?

[WebKit Review Bot]

Comment on attachment 193135 [details]
patch v4

Rejecting attachment 193135 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 193135, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ffset 12 lines).
Hunk #5 succeeded at 544 (offset 20 lines).
patching file Source/WebKit2/UIProcess/efl/WebView.h
Hunk #1 succeeded at 57 (offset 2 lines).
Hunk #2 succeeded at 92 (offset 2 lines).
Hunk #3 FAILED at 98.
Hunk #4 FAILED at 175.
2 out of 4 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/efl/WebView.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Andreas Kling']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17418080

[Mikhail Pozdnyakov]

Committed r147617: <http://trac.webkit.org/changeset/147617>