Closed Bug 562181 Opened 10 years ago Closed 10 years ago

Missing support for MozOrientation on Qt platform

Categories

(Core Graveyard :: Widget: Qt, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: MikeK, Assigned: MikeK)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 21 obsolete files)

51.40 KB, patch
Details | Diff | Splinter Review
Actually the code is there (in widget/src/gtk2/nsAccelerometerUnix.cpp) - but currently it is only being used when we build for gtk2.
No X support in this string of patches
Assignee: nobody → mkristoffersen
Status: NEW → ASSIGNED
Now without white space changes - All patches fits c3af7e83e26f on mozilla-central
Attachment #447609 - Attachment is obsolete: true
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?
Attachment #447606 - Flags: feedback?(ted.mielczarek)
Attached patch Part 1/5 - Move interface file (obsolete) — Splinter Review
Attachment #447606 - Attachment is obsolete: true
Attachment #447606 - Flags: feedback?(ted.mielczarek)
Attachment #447607 - Attachment is obsolete: true
Attachment #447608 - Attachment is obsolete: true
Attachment #447620 - Attachment is obsolete: true
Attached patch Part 1/5 Move interface files (obsolete) — Splinter Review
Cosmetic changes in the series
Attachment #448011 - Attachment is obsolete: true
Cosmetic changes in the series
Attachment #448012 - Attachment is obsolete: true
Cosmetic changes in the series
Attachment #448014 - Attachment is obsolete: true
Cosmetic changes in the series
Cosmetic changes in the series
Attachment #448015 - Attachment is obsolete: true
Attachment #448016 - Attachment is obsolete: true
Attachment #448815 - Flags: review?(ted.mielczarek)
Attachment #448816 - Flags: review?(ted.mielczarek)
Attachment #448817 - Flags: review?(ted.mielczarek)
Attachment #448818 - Flags: review?(ted.mielczarek)
Attachment #448819 - Flags: review?(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.
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.
Attachment #448815 - Flags: review?(ted.mielczarek)
Attachment #448816 - Flags: review?(ted.mielczarek)
Attachment #448817 - Flags: review?(ted.mielczarek)
Attachment #448818 - Flags: review?(ted.mielczarek)
Attachment #448819 - Flags: review?(ted.mielczarek)
jst and I both were okay with moving platformish code into dom/system
How about one of you does the review then? :)
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.
Will do - as soon as I get my machine to build mobile again with trunk source.
These hg commands must be executed before the patch can apply
Attachment #448815 - Attachment is obsolete: true
Attachment #448816 - Attachment is obsolete: true
Attachment #448817 - Attachment is obsolete: true
Attachment #448818 - Attachment is obsolete: true
Attachment #448819 - Attachment is obsolete: true
Attachment #450098 - Flags: review?(doug.turner)
Attachment #450099 - Flags: review?(doug.turner)
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).
Attachment #450098 - Flags: review?(doug.turner) → review-
Attachment #450099 - Flags: review?(doug.turner) → review-
also, dbaron showed be a --git diff has these commands... so that would be best.
Fits:

changeset:   43717:431eab8cf6ab
tag:         tip
user:        Alexander Surkov <surkov.alexander@gmail.com>
date:        Fri Jun 11 18:54:18 2010 +0900
summary:     Bug 541618 - fix solaris bustage
Attachment #450098 - Attachment is obsolete: true
Attachment #450099 - Attachment is obsolete: true
(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 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 <mikek@mikek.dk>
>+#
>+# 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?
Attached patch Updated with review comments (obsolete) — Splinter Review
Attachment #450748 - Attachment is obsolete: true
(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.
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.
(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 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.
Attachment #451030 - Flags: review?(doug.turner)
Attachment #451030 - Flags: review?(doug.turner)
Comment on attachment 451030 [details] [diff] [review]
Updated with review comments

This is the wrong patch...
Think I got myself too confused yesterday - the patch has been updated to trunk as a bonus.
Attachment #451030 - Attachment is obsolete: true
Attachment #452207 - Flags: review?(doug.turner)
Blocks: 569680
Attached patch Updated to trunk (obsolete) — Splinter Review
Attachment #452207 - Attachment is obsolete: true
Attachment #453047 - Flags: review?(doug.turner)
Attachment #452207 - Flags: review?(doug.turner)
Attachment #453047 - Flags: review?(doug.turner) → review+
Keywords: checkin-needed
Attached patch Updated to trunkSplinter Review
Attachment #453047 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/1d4e2804796a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Depends on: 574869
Is
  defined(machintosh)
with -ch- really right?
Blocks: 575206
(In reply to comment #40)
> Is
>   defined(machintosh)
> with -ch- really right?

Probably not - the fix for this is handled in Bug 575206
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.