Bug 47490 - [Qt] Mock DeviceOrientation client for DRT
Summary: [Qt] Mock DeviceOrientation client for DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Diego Gonzalez
URL:
Keywords: Qt
Depends on:
Blocks: 47050
  Show dependency treegraph
 
Reported: 2010-10-11 07:53 PDT by Diego Gonzalez
Modified: 2010-12-11 11:20 PST (History)
10 users (show)

See Also:


Attachments
Patch (20.46 KB, patch)
2010-10-11 08:24 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch v2 (20.03 KB, patch)
2010-10-11 09:04 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch v3 (20.20 KB, patch)
2010-12-03 05:51 PST, Diego Gonzalez
kenneth: review+
Details | Formatted Diff | Diff
followup patch for Patch v3 (4.49 KB, patch)
2010-12-07 08:09 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 2010-10-11 07:53:34 PDT
Mock device orientation client to simulate orientation events for Qt DRT.
LayoutTests:
    fast/dom/DeviceMotion
    fast/dom/DeviceOrientation
Comment 1 Diego Gonzalez 2010-10-11 08:24:06 PDT
Created attachment 70436 [details]
Patch
Comment 2 Diego Gonzalez 2010-10-11 08:26:13 PDT
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?
Comment 3 Andreas Kling 2010-10-11 08:33:16 PDT
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.
Comment 4 Early Warning System Bot 2010-10-11 08:34:05 PDT
Attachment 70436 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4290028
Comment 5 Diego Gonzalez 2010-10-11 08:43:56 PDT
(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.
Comment 6 Diego Gonzalez 2010-10-11 09:04:56 PDT
Created attachment 70439 [details]
Patch v2

Make it build with on Qt
Comment 7 Diego Gonzalez 2010-12-03 05:51:24 PST
Created attachment 75492 [details]
Patch v3
Comment 8 Kenneth Rohde Christiansen 2010-12-06 01:31:26 PST
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.
Comment 9 Diego Gonzalez 2010-12-07 06:21:13 PST
(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?
Comment 10 Csaba Osztrogonác 2010-12-07 08:09:27 PST
Created attachment 75818 [details]
followup patch for Patch v3

I prefer making build system better to passing --device-orientation to build-webkit manually.
Comment 11 Kenneth Rohde Christiansen 2010-12-07 08:15:26 PST
Shouldn't we enable it by default?
Comment 12 Csaba Osztrogonác 2010-12-07 08:17:10 PST
(In reply to comment #11)
> Shouldn't we enable it by default?

It would break the build if QtMobility isn't installed somewhere.
Comment 13 WebKit Review Bot 2010-12-07 08:53:23 PST
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.
Comment 14 Diego Gonzalez 2010-12-07 09:13:27 PST
(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?
Comment 15 Csaba Osztrogonác 2010-12-07 09:15:11 PST
(In reply to comment #14)
> Does the bots have QtMobility installed?
Of course. ;)
Comment 16 WebKit Review Bot 2010-12-07 09:54:36 PST
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.
Comment 17 Diego Gonzalez 2010-12-07 09:58:17 PST
(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 :)
Comment 18 WebKit Review Bot 2010-12-07 10:55:43 PST
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.
Comment 19 WebKit Review Bot 2010-12-07 11:57:01 PST
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.
Comment 20 WebKit Review Bot 2010-12-07 21:36:07 PST
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.
Comment 21 Diego Gonzalez 2010-12-09 09:50:52 PST
(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? :)
Comment 22 Csaba Osztrogonác 2010-12-10 01:35:58 PST
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.
Comment 23 Diego Gonzalez 2010-12-11 11:20:14 PST
Patch landed at r73862. Closing bug.