Bug 47490 – [Qt] Mock DeviceOrientation client for DRT

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

[Diego Gonzalez]

Mock device orientation client to simulate orientation events for Qt DRT.
LayoutTests:
    fast/dom/DeviceMotion
    fast/dom/DeviceOrientation

[Diego Gonzalez]

Created attachment 70436 [details]
Patch

[Diego Gonzalez]

For it's works with DRT is necessary to build if --device-orientation and assure if Qt Mobility with sensors lib is installed in the bots. Ossy, could you check if this patch build in the bots?

[Andreas Kling]

Comment on attachment 70436 [details]
Patch

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

> WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:-25
> -

Touching an unrelated file just to remove a newline seems a little unnecessary. ;-)

> WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:662
> +    delete DeviceOrientationClientMockQt::client();

This doesn't look right- after this, client() will still return the old (now deleted) pointer.

[Early Warning System Bot]

Attachment 70436 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4290028

[Diego Gonzalez]

(In reply to comment #3)
> (From update of attachment 70436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70436&action=review
> 
> > WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:-25
> > -
> 
> Touching an unrelated file just to remove a newline seems a little unnecessary. ;-)
> 
> > WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:662
> > +    delete DeviceOrientationClientMockQt::client();
> 
> This doesn't look right- after this, client() will still return the old (now deleted) pointer.


It's only used by DRT, for this I called to remove it at DRT destructor.

[Diego Gonzalez]

Created attachment 70439 [details]
Patch v2

Make it build with on Qt

[Diego Gonzalez]

Created attachment 75492 [details]
Patch v3

[Kenneth Rohde Christiansen]

Comment on attachment 75492 [details]
Patch v3

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

> WebKit/qt/WebCoreSupport/DeviceOrientationClientMockQt.cpp:48
> +    delete m_clientMock;

cant this be an OwnPtr? If not this is ok

> WebKit/qt/WebCoreSupport/DeviceOrientationClientMockQt.h:46
> +    static bool drtActive;

static bool mockIsActive would be more descriptive.

[Diego Gonzalez]

(In reply to comment #8)
> (From update of attachment 75492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75492&action=review
> 
> > WebKit/qt/WebCoreSupport/DeviceOrientationClientMockQt.cpp:48
> > +    delete m_clientMock;
> 
> cant this be an OwnPtr? If not this is ok
> 
> > WebKit/qt/WebCoreSupport/DeviceOrientationClientMockQt.h:46
> > +    static bool drtActive;
> 
> static bool mockIsActive would be more descriptive.

Thanks Kenneth.

To build the orientation stuff we should user --device-orientation flags.
Ossy: is possible to get the bots building with this flag?

[Csaba Osztrogonác]

Created attachment 75818 [details]
followup patch for Patch v3

I prefer making build system better to passing --device-orientation to build-webkit manually.

[Kenneth Rohde Christiansen]

Shouldn't we enable it by default?

[Csaba Osztrogonác]

(In reply to comment #11)
> Shouldn't we enable it by default?

It would break the build if QtMobility isn't installed somewhere.

[WebKit Review Bot]

Attachment 75818 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Diego Gonzalez]

(In reply to comment #12)
> (In reply to comment #11)
> > Shouldn't we enable it by default?
> 
> It would break the build if QtMobility isn't installed somewhere.

Does the bots have QtMobility installed?

[Csaba Osztrogonác]

(In reply to comment #14)
> Does the bots have QtMobility installed?
Of course. ;)

[WebKit Review Bot]

Attachment 75818 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Diego Gonzalez]

(In reply to comment #15)
> (In reply to comment #14)
> > Does the bots have QtMobility installed?
> Of course. ;)

Cool, in this case I think your patch is a good way :)

[WebKit Review Bot]

Attachment 75818 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75818 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75818 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Diego Gonzalez]

(In reply to comment #10)
> Created an attachment (id=75818) [details]
> followup patch for https://bugs.webkit.org/attachment.cgi?id=75492
> 
> I prefer making build system better to passing --device-orientation to build-webkit manually.

Could anyone review this Ossy's patch? I cannot commit my patch without this patch? :)

[Csaba Osztrogonác]

Comment on attachment 75818 [details]
followup patch for Patch v3

It is obsolote because of https://bugs.webkit.org/show_bug.cgi?id=50781

I'm going to update it.

[Diego Gonzalez]

Patch landed at r73862. Closing bug.