Bug 110753

Created attachment 190235 [details] WIP

Created attachment 190241 [details] patch

Comment on attachment 190241 [details] patch LGTM

Created attachment 191685 [details] patch v2 rebased

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?

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

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

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.

Created attachment 192891 [details] patch v3 Rebased. Took comments from Dongsung Huang into consideration - thanks.

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.

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

Created attachment 193135 [details] patch v4 Made EwkView size getters private as was pointed out by Dongsung Huang.

Comment on attachment 193135 [details] patch v4 LGTM

Ping owner review/sign off?

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

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