562181 - Missing support for MozOrientation on Qt platform

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Mike Kristoffersen (:MikeK)]

Actually the code is there (in widget/src/gtk2/nsAccelerometerUnix.cpp) - but currently it is only being used when we build for gtk2.

Comment 1

[Mike Kristoffersen (:MikeK)]

Attachment: Movement of the interface file (patch 1/4)

No X support in this string of patches

Comment 2

[Mike Kristoffersen (:MikeK)]

Attachment: Movement of base accelerometer class (patch 2/4)

Comment 3

[Mike Kristoffersen (:MikeK)]

Attachment: Movement of Linux files (patch 3/4)

Comment 4

[Mike Kristoffersen (:MikeK)]

Attachment: Movement of Windows files (patch 4/4)

Comment 5

[Mike Kristoffersen (:MikeK)]

Attachment: Movement of Windows files (patch 4/4)

Now without white space changes - All patches fits c3af7e83e26f on mozilla-central

Comment 6

[Mike Kristoffersen (:MikeK)]

Comment on attachment 447606 [details] [diff] [review]
Movement of the interface file (patch 1/4)

Hi Ted, can I get your initial feedback on the stuff done so far?

Comment 7

[Mike Kristoffersen (:MikeK)]

Attachment: Part 1/5 - Move interface file

Comment 8

[Mike Kristoffersen (:MikeK)]

Attachment: Part 2/5 - Move common implementation

Comment 9

[Mike Kristoffersen (:MikeK)]

Attachment: Part 3/5 - Move Linux specific files

Comment 10

[Mike Kristoffersen (:MikeK)]

Attachment: Part 4/5 - Move Windows specific files

Comment 11

[Mike Kristoffersen (:MikeK)]

Attachment: Part 5/5 - Move Mac specific files

Comment 12

[Mike Kristoffersen (:MikeK)]

Attachment: Part 1/5 Move interface files

Cosmetic changes in the series

Comment 13

[Mike Kristoffersen (:MikeK)]

Attachment: Part 2/5 - Move common implementation

Cosmetic changes in the series

Comment 14

[Mike Kristoffersen (:MikeK)]

Attachment: Part 3/5 - Move Linux specific files

Cosmetic changes in the series

Comment 15

[Mike Kristoffersen (:MikeK)]

Attachment: Part 4/5 - Move Windows specific files

Cosmetic changes in the series

Comment 16

[Mike Kristoffersen (:MikeK)]

Attachment: Part 5/5 - Move Mac specific files

Cosmetic changes in the series

Comment 17

[(not currently active) Ted Mielczarek]

Can you explain the reasoning behind all this file-shuffling?

Also, since you're just moving files, I think you'd be better off with review from a widget peer.

Comment 18

[Mike Kristoffersen (:MikeK)]

The reason for the move is that the code was previously placed in a gtk2 directory even it wasn't gtk2 specific - a discussion with a number of people resulted in a decision to create dom/system and have system specific code under it.

I'll try to find someone more appropriate to do the review.

Comment 19

[Doug Turner (:dougt)]

jst and I both were okay with moving platformish code into dom/system

Comment 20

[(not currently active) Ted Mielczarek]

How about one of you does the review then? :)

Comment 21

[Doug Turner (:dougt)]

sure, i can review; jst can sr.

Like I mentioned in another bug, lets just see the hg commands (not the patches).  And then have one patch for any new files or diffs you need to keep things building/working.

Comment 22

[Mike Kristoffersen (:MikeK)]

Will do - as soon as I get my machine to build mobile again with trunk source.

Comment 23

[Mike Kristoffersen (:MikeK)]

Attachment: Hg commands that moves the files around

These hg commands must be executed before the patch can apply

Comment 24

[Mike Kristoffersen (:MikeK)]

Attachment: Fixes the files (MUST be applied AFTER files are moved)

Comment 25

[Doug Turner (:dougt)]

Comment on attachment 450099 [details] [diff] [review]
Fixes the files (MUST be applied AFTER files are moved)

can you get the classes to be named the same to avoid:

+#ifdef unix
+       nsAccelerometerUnixConstructor
+#elif defined(_WINDOWS)
+       nsAccelerometerWinConstructor
+#elif defined(macintosh)
+       nsAccelerometerXConstructor
+#endif

And instead, remove:

#ifdef NS_ACCELEROMETER_IMPLEMENTED


And just use the platform check.  (don't use #ifdef unix, instead use XP_UNIX).

Comment 26

[Doug Turner (:dougt)]

also, dbaron showed be a --git diff has these commands... so that would be best.

Comment 27

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with off-line review comments

Fits:

changeset:   43717:431eab8cf6ab
tag:         tip
user:        Alexander Surkov <[e-mail]>
date:        Fri Jun 11 18:54:18 2010 +0900
summary:     Bug 541618 - fix solaris bustage

Comment 28

[Mike Kristoffersen (:MikeK)]

(In reply to comment #25)
> (From update of attachment 450099 [details] [diff] [review])
> can you get the classes to be named the same to avoid:
> 

I have renamed them to nsAccelerometerSystem, the only _minor_ problem I see with this solution is that it makes it harder to see which file you are currently working on when you are editing the file, and it moved the complexity to the makefile instead of the source file.

The patch has also been updated to include the android accelerometer files that landed somewhere after the original creation of the patch.

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 450748 [details] [diff] [review]
Updated with off-line review comments


>-#include "nsIAccelerometer.h"
>-#include "nsWidgetsCID.h"
>+#include "nsAccelerometer.h"
> #include "nsIContent.h"

What happened to nsWidgetsCID.h?

>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2009

2010

>+
>+# LOCAL_INCLUDES	= \
>+# 		-I$(srcdir) \
>+# 		-I$(srcdir)/../../../base \
>+# 		-I$(srcdir)/../../../../content/base/src \
>+# 		-I$(srcdir)/../../../../content/events/src \
>+# 		-I$(srcdir)/../../../generic \
>+# 		-I$(srcdir)/../../../style \
>+# 		$(NULL)

what is this mess?
>+# The Original Code is mozilla.org build system.
>+#
>+# The Initial Developer of the Original Code is Mozilla Foundation
>+# Portions created by the Initial Developer are Copyright (C) 2008
>+# the Initial Developer. All Rights Reserved.

2010

>+#
>+# Contributor(s):
>+#  Mike Kristoffersen <[e-mail]>
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+DEPTH       = ../../..
>+topsrcdir   = @top_srcdir@
>+srcdir      = @srcdir@
>+VPATH       = @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+MODULE      = dom
>+LIBRARY_NAME    = domsystemandroid_s
>+
>+# we don't want the shared lib, but we want to force the creation of a static lib.
>+LIBXUL_LIBRARY   = 1
>+FORCE_STATIC_LIB = 1
>+EXPORT_LIBRARY = 1
>+
>+include $(topsrcdir)/config/config.mk
>+
>+CPPSRCS     = \
>+        nsAccelerometerSystem.cpp \
>+        $(NULL)
>+
>+include $(topsrcdir)/config/rules.mk
>


>+# Portions created by the Initial Developer are Copyright (C) 2008
>+# the Initial Developer. All Rights Reserved.

2010


Otherwise looks fine.  Did you run this through mozilla try?

Comment 30

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with review comments

Comment 31

[Mike Kristoffersen (:MikeK)]

(In reply to comment #29)
> (From update of attachment 450748 [details] [diff] [review])
> 
> >-#include "nsIAccelerometer.h"
> >-#include "nsWidgetsCID.h"
> >+#include "nsAccelerometer.h"
> > #include "nsIContent.h"
> 
> What happened to nsWidgetsCID.h?

It's not needed anymore...

> >+
> >+# LOCAL_INCLUDES	= \
> >+# 		-I$(srcdir) \
> >+# 		-I$(srcdir)/../../../base \
> >+# 		-I$(srcdir)/../../../../content/base/src \
> >+# 		-I$(srcdir)/../../../../content/events/src \
> >+# 		-I$(srcdir)/../../../generic \
> >+# 		-I$(srcdir)/../../../style \
> >+# 		$(NULL)
> 
> what is this mess?

No idea - copy paste error maybee... it has been nuked!

> Otherwise looks fine.  Did you run this through mozilla try?

Yes originally I did - trying to run it again with the updated patch failed with a "transaction abort" and:

remote: abort: Permission denied: /repo/hg/mozilla/try_vcview/.hg/store/data/editor/libeditor/html/tests/test__root__element__replacement.html.i
remote: warning: changegroup.z_push_to_vcview hook exited with status 255

I wouldn't let me retry...  no idea what that error is about, will comment in this bug when I get a clean run.

Comment 32

[Johnny Stenback (:jst)]

That was due to a problem with the try server, see bug 571860.

If you either re-qpush your local patches or commit a dummy change on top of what you have so that there's a new commit id that the try server hasn't seen yet, the try server should accept it now.

Comment 33

[Mike Kristoffersen (:MikeK)]

(In reply to comment #32)
> That was due to a problem with the try server, see bug 571860.
> 
> If you either re-qpush your local patches or commit a dummy change on top of
> what you have so that there's a new commit id that the try server hasn't seen
> yet, the try server should accept it now.

Hey, thank you :)  I thought I had made a mistake somewhere... the try-server now accepted the change - I'll updated this bug when the results are in.

Comment 34

[Mike Kristoffersen (:MikeK)]

Comment on attachment 451030 [details] [diff] [review]
Updated with review comments

After studying the try server results, I don't think any of the warnings are related to this patch.

Comment 35

[Mike Kristoffersen (:MikeK)]

Comment on attachment 451030 [details] [diff] [review]
Updated with review comments

This is the wrong patch...

Comment 36

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk (Changeset: 87e2e65a7323)

Think I got myself too confused yesterday - the patch has been updated to trunk as a bonus.

Comment 37

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk

Comment 38

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk

Comment 39

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1d4e2804796a

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Is
  defined(machintosh)
with -ch- really right?

Comment 41

[Mike Kristoffersen (:MikeK)]

(In reply to comment #40)
> Is
>   defined(machintosh)
> with -ch- really right?

Probably not - the fix for this is handled in Bug 575206