Bug 74417

Summary: [Qt] DeviceOrientationClientMockQt should be removed in favor of DeviceOrientationClientMock
Product: WebKit Reporter: Alexander Færøy <ahf>
Component: WebKit QtAssignee: Alexander Færøy <ahf>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, kenneth, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Alexander Færøy 2011-12-13 09:34:04 PST
SSIA.

Patch coming up.
Comment 1 Alexander Færøy 2011-12-13 15:00:32 PST
Created attachment 119089 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-12-14 00:44:06 PST
Comment on attachment 119089 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        [Qt] DeviceOrientationClientMockQt should be removed in favor of DeviceOrientationClientMock
> +        https://bugs.webkit.org/show_bug.cgi?id=74417
> +
> +        Reviewed by NOBODY (OOPS!).

As this is based on my code and I did it together with you :-) You should add me and I shouldnt review it officially.

> Source/WebKit/qt/Api/qwebframe.cpp:474
>      WebCore::Frame* frame = core(q);
>  
>      switch (m_orientation.reading()->orientation()) {
> -    case QtMobility::QOrientationReading::TopUp:
> +    case QOrientationReading::TopUp:
>          orientation = 0;
>          break;
> -    case QtMobility::QOrientationReading::TopDown:
> +    case QOrientationReading::TopDown:
>          orientation = 180;
>          break;
> -    case QtMobility::QOrientationReading::LeftUp:
> +    case QOrientationReading::LeftUp:
>          orientation = -90;
>          break;
> -    case QtMobility::QOrientationReading::RightUp:
> +    case QOrientationReading::RightUp:
>          orientation = 90;
>          break;
> -    case QtMobility::QOrientationReading::FaceUp:
> -    case QtMobility::QOrientationReading::FaceDown:
> +    case QOrientationReading::FaceUp:
> +    case QOrientationReading::FaceDown:
>          // WebCore unable to handle it
>      default:
>          return;

All this code is wrong. This is for the browser window orientation on the device. It may not always follow device orientation as you could for instance have a finger on the window and block it from changing browser window orientation

> Source/WebKit/qt/Api/qwebframe_p.h:40
> +#if ENABLE(ORIENTATION_EVENTS)

ORIENTATION_EVENTS are for browser orientation! But if this is not your code (ie unrelated to the patch) it can be fixed in a follow up

> Source/WebKit/qt/Api/qwebpage.cpp:326
> +    if (QWebPagePrivate::drtRun)
> +        pageClients.deviceOrientationClient = new DeviceOrientationClientMock;
> +    else
> +        pageClients.deviceOrientationClient = new DeviceOrientationClientQt;

Maybe the below is clearer?

bool useMock = QWebPagePrivate::drtRun;

pageClients.deviceOrientationClient = useMock ? new DeviceOrientationClientMock : new DeviceOrientationClientQt;

?
Comment 3 Alexander Færøy 2011-12-14 01:29:51 PST
Created attachment 119177 [details]
Patch
Comment 4 Simon Hausmann 2011-12-14 01:38:40 PST
Comment on attachment 119177 [details]
Patch

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

r=me but watch the bot when landing :)

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:853
> +    QList<WebCore::WebPage*> pages = m_drt->getAllPages();
> +    foreach (WebCore::WebPage* page, pages)
> +        DumpRenderTreeSupportQt::setMockDeviceOrientation(page, canProvideAlpha, alpha, canProvideBeta, beta, canProvideGamma, gamma);

The loop could also be moved into setMockDeviceOrientation.
Comment 5 WebKit Review Bot 2011-12-14 01:57:42 PST
Comment on attachment 119177 [details]
Patch

Clearing flags on attachment: 119177

Committed r102755: <http://trac.webkit.org/changeset/102755>
Comment 6 WebKit Review Bot 2011-12-14 01:57:46 PST
All reviewed patches have been landed.  Closing bug.