Bug 106355

Summary: [Qt][EFL][WK2] Remove redundant device pixel ratio adjustment from PageViewportController
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: New BugsAssignee: Andras Becsi <abecsi>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dongseong.hwang, gyuyoung.kim, hausmann, jturcotte, kenneth, menard, ossy, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106499, 106500, 106504, 106512    
Attachments:
Description Flags
Patch none

Description Andras Becsi 2013-01-08 11:32:39 PST
[WK2] Remove redundant device pixel ratio adjustment from PageViewportController
Comment 1 Andras Becsi 2013-01-08 11:37:14 PST
Created attachment 181717 [details]
Patch
Comment 2 Andras Becsi 2013-01-08 11:41:35 PST
(In reply to comment #1)
> Created an attachment (id=181717) [details]
> Patch

A follow-up could remove the device pixel ratio adjustment logic from the ViewportAttributes also, but I'm not sure how dependent other ports are on that code.
Comment 3 Kenneth Rohde Christiansen 2013-01-08 12:37:47 PST
Comment on attachment 181717 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        As a result we can follow the same aproach as QtQuick and all the

spelling

> Source/WebKit2/ChangeLog:15
> +        coordinates in PageViewportController need to be in device independent
> +        pixels (dips) thus we do no longer need to adjust with the device pixel

AKA UI pixels

> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:66
> +    // The viewport controller expects sizes in device independent coordinates.

Maybe it is more understandable saying that

// The viewport controller expects sizes in UI units, and not raw device units.

> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:67
> +    size.scale(1 / m_controller->devicePixelRatio());

devicePixelRatio is called deviceScaleFactor most places now, maybe we should rename
Comment 4 Andras Becsi 2013-01-09 06:01:29 PST
Committed r139189: <http://trac.webkit.org/changeset/139189>
Comment 5 Andras Becsi 2013-01-09 06:10:38 PST
Comment on attachment 181717 [details]
Patch

Clearing flags from attachment.
Comment 6 Csaba Osztrogon√°c 2013-01-09 08:56:09 PST
(In reply to comment #4)
> Committed r139189: <http://trac.webkit.org/changeset/139189>

It broke many Qt-WK2 API tests:

FAIL!  : qmltests::DoubleTapToZoom::test_basic_zoomInAndBack() Compared values are not the same
   Actual   (): 2.5
   Expected (): 1.3333333333333333
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(95)]
FAIL!  : qmltests::DoubleTapToZoom::test_double_zoomInAndBack() Compared values are not the same
   Actual   (): 128x96
   Expected (): 320x240
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(106)]
FAIL!  : qmltests::DoubleTapToZoom::test_double_zoomInAndBack2() Compared values are not the same
   Actual   (): 2.5
   Expected (): 1.3333333333333333
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(146)]
FAIL!  : qmltests::DoubleTapToZoom::test_double_zoomInOutAndBack() Compared values are not the same
   Actual   (): 128x96
   Expected (): 320x240
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(166)]
FAIL!  : qmltests::DoubleTapToZoom::test_double_zoomInOutAndBack2() Compared values are not the same
   Actual   (): 2.5
   Expected (): 2
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(207)]
FAIL!  : qmltests::Resize::test_resizeAfterNeutralZoom() Compared values are not the same
   Actual   (): 2.5
   Expected (): 1.2307692307692308
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_resize.qml(120)]
FAIL!  : qmltests::Resize::test_resizeZoomedIn() Compared values are not the same
   Actual   (): 2.5
   Expected (): 1
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_resize.qml(143)]
Comment 7 Andras Becsi 2013-01-09 09:38:34 PST
(In reply to comment #6)
> (In reply to comment #4)
> > Committed r139189: <http://trac.webkit.org/changeset/139189>
> 
> It broke many Qt-WK2 API tests:

Looking into it, sorry for the breakage.
Comment 8 Andras Becsi 2013-01-09 11:53:42 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Committed r139189: <http://trac.webkit.org/changeset/139189>
> > 
> > It broke many Qt-WK2 API tests:
> 
> Looking into it, sorry for the breakage.

Fixed in http://trac.webkit.org/changeset/139215.
Comment 9 Dongseong Hwang 2013-01-09 16:55:30 PST
Great. Code became more succinct.

I fixed some nits in Bug 106499 and Bug 106500.
And I fixed blurring regression in Bug 106504.
Comment 10 Dongseong Hwang 2013-01-09 18:00:30 PST
I fixed regression that MiniBrowser renders abnormally with -r option in Bug 106512.
Comment 11 Andras Becsi 2013-01-10 10:53:19 PST
Thanks for fixing these issues.