541015 - Support rotating between portrait and landscape on N900

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

The N900 supports portrait and landscape modes. It also support rotation/orientation events to switch between the modes.

Unfortunately, it's not as simple as setting a flag in our application. See this post for more information:
http://wiki.maemo.org/Using_Fremantle_Widgets#Portrait_Mode

Comment 1

[Doug Turner (:dougt)]

drive by nits/comments:  


In WidgetForDOMWindow, return nsnull instead of 0


+  baseWindow->GetMainWidget(getter_AddRefs(widget));

Test for null before using widget again.


Drop the printf


+      GtkWidget* widget = WidgetForDOMWindow(window);
+      GdkWindow *gdk = GTK_WIDGET(widget)->window;


Test for widget before using


Need to call dbus_bus_remove_match somewhere.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Several people tested the build here:
http://people.mozilla.com/~mfinkle/fennec/fennec-1.1a1pre.en-US.linux-gnueabi-are.rotation.tar

and found that dynamic rotation was somehow tied to microb. If they enabled portrait mode in microb (shift+ctrl+o) then it worked in fennec too. It failed otherwise.

Comment 3

[LonelyBob]

Not sure why dynamic rotation only works when dynamic rotation is enabled in microb browser. But if that is the case, we may want to look into how GPodder is implementing auto-rotation, Gpodder has had auto-rotation on N900 for a while now without having anything to do with microb's setting. 
Some useful links for the project: http://gpodder.org or https://garage.maemo.org/projects/gpodder/

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch adds the required property to the Gdk window during creation. It also enables accelerometers, so we can get updates on the orientation. When we get an orientation update, we issue the Hildon property changes to cause the window to rotate.

We also disable/enable the accelerometers on screen dim, to save battery power.

Karl for widget changes

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 435679 [details] [diff] [review]
patch 2

Doug for AppSupportUnix changes

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Also, note this patch is for mozilla-192, not trunk. I tested it on device and it works correctly, without any microb tweaks.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 435679 [details] [diff] [review]
patch 2


> #include <dbus/dbus.h>
> #include <dbus/dbus-protocol.h>
> #include <libosso.h>

  add a space here

>+#define MCE_SERVICE "com.nokia.mce"
>+#define MCE_REQUEST_IF "com.nokia.mce.request"
>+#define MCE_REQUEST_PATH "/com/nokia/mce/request"
>+#define MCE_SIGNAL_IF "com.nokia.mce.signal"
>+#define MCE_DEVICE_ORIENTATION_SIG "sig_device_orientation_ind"
>+#define MCE_MATCH_RULE "type='signal',interface='" MCE_SIGNAL_IF "',member='" MCE_DEVICE_ORIENTATION_SIG "'"

It be nice to reference where you found all of these values.


> 
>+  // Enable the accelerometer for orientation support
>+  OssoRequestAccelerometer(PR_TRUE_);
>+

Is PR_TRUE_ a real type?



>+  /* Get the system DBus connection */
>+  DBusConnection *connnection = (DBusConnection*)osso_get_sys_dbus_connection(m_osso_context);
>+  dbus_bus_add_match(connnection, MCE_MATCH_RULE, nsnull);
>+  dbus_connection_add_filter(connnection, dbus_handle_mce_message, nsnull, nsnull);


Do we have to remove the filter when we close this connection?


Otherwise looks great

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

with dougt's comments addressed

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 435695 [details] [diff] [review]
patch 3



>+// These come from <mce/dbus-names.h> (maemo sdk 5+)
>+#define MCE_SERVICE "com.nokia.mce"
>+#define MCE_REQUEST_IF "com.nokia.mce.request"
>+#define MCE_REQUEST_PATH "/com/nokia/mce/request"
>+#define MCE_SIGNAL_IF "com.nokia.mce.signal"
>+#define MCE_DEVICE_ORIENTATION_SIG "sig_device_orientation_ind"
>+#define MCE_MATCH_RULE "type='signal',interface='" MCE_SIGNAL_IF "',member='" MCE_DEVICE_ORIENTATION_SIG "'"

are they defined in 5+?  do we need to #ifndef #define #endif?


>+static void OssoRequestAccelerometer(PRBool aEnabled)
>+{
>+  if (aEnabled) {
>+    osso_rpc_run_system(m_osso_context, MCE_SERVICE, MCE_REQUEST_PATH, MCE_REQUEST_IF,
>+                        "req_accelerometer_enable", NULL, DBUS_TYPE_INVALID);
>+  } else {
>+    osso_rpc_run_system(m_osso_context, MCE_SERVICE, MCE_REQUEST_PATH, MCE_REQUEST_IF,
>+                        "req_accelerometer_disable", NULL, DBUS_TYPE_INVALID);
>+  }
>+}


osso_rpc_run_system(m_osso_context, 
                    MCE_SERVICE,
                    MCE_REQUEST_PATH, MCE_REQUEST_IF,
                    aEnabled ? "req_accelerometer_enable" : "req_accelerometer_disable",
                    NULL,
                    DBUS_TYPE_INVALID);

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 4

With a DougT suggested change to OssoRequestAccelerometer

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> (From update of attachment 435695 [details] [diff] [review])
> 
> 
> >+// These come from <mce/dbus-names.h> (maemo sdk 5+)
> >+#define MCE_SERVICE "com.nokia.mce"
> >+#define MCE_REQUEST_IF "com.nokia.mce.request"
> >+#define MCE_REQUEST_PATH "/com/nokia/mce/request"
> >+#define MCE_SIGNAL_IF "com.nokia.mce.signal"
> >+#define MCE_DEVICE_ORIENTATION_SIG "sig_device_orientation_ind"
> >+#define MCE_MATCH_RULE "type='signal',interface='" MCE_SIGNAL_IF "',member='" MCE_DEVICE_ORIENTATION_SIG "'"
> 
> are they defined in 5+?  do we need to #ifndef #define #endif?

Only if you include the right header file, which we currently don't

> >+static void OssoRequestAccelerometer(PRBool aEnabled)
> 
> osso_rpc_run_system(m_osso_context, 
>                     MCE_SERVICE,
>                     MCE_REQUEST_PATH, MCE_REQUEST_IF,
>                     aEnabled ? "req_accelerometer_enable" :
> "req_accelerometer_disable",
>                     NULL,
>                     DBUS_TYPE_INVALID);

Changed

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Karl, Doug wanted you to take a peek at the AppSupportUnix changes too

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 435728 [details] [diff] [review]
patch 4

>+            GdkWindow *gdkwin = GTK_WIDGET (mShell)->window;

mShell is already a GtkWidget, so no need for the GTK_WIDGET() cast.

>+            // Tell the Hildon desktop that we support being rotated
>+            guint32 portrait_set = 1;
>+            GdkAtom support = gdk_atom_intern("_HILDON_PORTRAIT_MODE_SUPPORT", FALSE);
>+            gdk_property_change(gdkwin, support, gdk_x11_xatom_to_atom(XA_CARDINAL),
>+                                32, GDK_PROP_MODE_REPLACE, (const guchar *) &portrait_set, 1);

The man page for XChangeProperty says

"If the specified format is 32, the property data must be a long array."

Similarly gdk docs say:

"Note that on the client side, properties of format 32 will be stored with one
unit per long, even if a long integer has more than 32 bits on the
platform. (This decision was apparently made for Xlib to maintain
compatibility with programs that assumed longs were 32 bits, at the expense of
programs that knew better.)"

So use gulong for the data in gdk_property_change (here and in dbus_handle_mce_message).

>+      GtkWidget* widget = WidgetForDOMWindow(window);
>+      GdkWindow *gdk = GTK_WIDGET(widget)->window;

I didn't review all the osso/dbus code thoroughly, but I didn't see how any initial portrait orientation before creation of the first window would be handled.

Looks like portrait mode would end if the window with _HILDON_PORTRAIT_MODE_REQUEST became non-"visible" even if there is now a more recent non-transient Gecko window visible.  Can the app have a second toplevel window?

Comment 14

[Karl Tomlinson (:karlt)]

>+      GtkWidget* widget = WidgetForDOMWindow(window);
>+      GdkWindow *gdk = GTK_WIDGET(widget)->window;

Forgot to insert comments on this.

No need for the GTK_WIDGET(GtkWidget*) cast to GtkWidget.

A null check on widget looks necessary.
A null widget->window is unlikely AFAIK, but safer to add a check anyway.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 435728 [details] [diff] [review]
patch 4

>@@ -390,19 +473,27 @@ nsNativeAppSupportUnix::Start(PRBool *aR
>                                    PR_TRUE,
>                                    nsnull);
> 
>   /* Check that initilialization was ok */
>   if (m_osso_context == nsnull) {
>       return NS_ERROR_FAILURE;
>   }
> 
>+  // Enable the accelerometer for orientation support
>+  OssoRequestAccelerometer(PR_TRUE);

Is it possible that nsNativeAppSupportUnix::Start() may be called while the screen is dimmed?  e.g. does faststart do restarts occasionally?

>     if (appService)
>       appService->Quit(nsIAppStartup::eForceQuit);
>-
>     return OSSO_OK;

I quite liked this blank line here, given the lack of braces.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Patch coming up that addresses all of Karl's comments:
* We check for screen-dimmed before enabling the accelerometer in nsNativeAppSupportUnix::Start()
* Added blank line back
* Remove unneeded GTK_WIDGET casts
* Adds a null check for the widget and widget->window
* Initial toplevel window orientation is set in nsNativeAppSupportUnix
* Subsequent toplevel window orientation is set in the nsWindow::Create method
* gulong for the data in gdk_property_change (nsWindow and
nsNativeAppSupportUnix)

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 5

With review comments addressed

Comment 18

[Doug Turner (:dougt)]

drive-by goodness:

[9:01pm] dougt: not to be an ass, but you probably can get rid of OssoRequestAccelerometer.
[9:01pm] dougt: or make it inline (at least)
[9:01pm] dougt: also, screen-orientation might be better?
[9:03pm] mfinkle: add that, "screen-orientation", as a drive-by please?

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 436116 [details] [diff] [review]
patch 5

>-  osso_hw_state_t m_hw_state;
>+  osso_hw_state_t m_hw_state;  

Accidental extra whitespace.

> static void OssoDisplayCallback(osso_display_state_t state, gpointer data)
> {
>   nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
>   if (!os)
>       return;
>- 
>-  if (state == OSSO_DISPLAY_ON)
>+
>+  osso_context_t* context = (osso_context_t*) data;
>+
>+  if (state == OSSO_DISPLAY_ON) {
>       os->NotifyObservers(nsnull, "system-display-on", nsnull);
>-  else
>+      OssoRequestAccelerometer(context, PR_TRUE);
>+  } else {

Here we have the possible situation where the orientation changed while the
display was off/dimmed, so the orientation should be checked here I assume.

Note that the current state is available from MCE_ACCELEROMETER_ENABLE_REQ (which is when you want to know it):

"If this request message indicates reply requested (dbus_no_reply is set
FALSE), then reply message is sent with current orientation. This eliminates
the need for separate MCE_DEVICE_ORIENTATION_GET when starting listening
orientation."

http://maemo.org/api_refs/5.0/5.0-final/mce-dev/dbus-names_8h.html#dde12c93c5957d39e127047cc430e034

So checking the orientation in OssoRequestAccelerometer() is possible, so
OssoIsPortrait() and nsNativeAppSupportUnix::Enable() are probably not
necessary.

>+  // Enable the accelerometer for orientation support
>+  if (OssoIsScreenOn(m_osso_context))
>+      OssoRequestAccelerometer(m_osso_context, PR_TRUE);
>+
>   osso_hw_set_event_cb(m_osso_context, nsnull, OssoHardwareCallback, &m_hw_state);
>-  osso_hw_set_display_event_cb(m_osso_context, OssoDisplayCallback, nsnull);
>+  osso_hw_set_display_event_cb(m_osso_context, OssoDisplayCallback, m_osso_context);
>   osso_rpc_set_default_cb_f(m_osso_context, OssoDbusCallback, nsnull);
>+
>+  // Setup an MCE callback to monitor orientation
>+  DBusConnection *connnection = (DBusConnection*)osso_get_sys_dbus_connection(m_osso_context);
>+  dbus_bus_add_match(connnection, MCE_MATCH_RULE, nsnull);
>+  dbus_connection_add_filter(connnection, dbus_handle_mce_message, nsnull, nsnull);

Documentation may be suggesting that the match rule should be added before
MCE_ACCELEROMETER_ENABLE_REQ, though I'm not clear on what constitutes a
"listener".

"If no listeners exist, then accelerometer is disabled and no messages are
sent."

>+dbus_handle_mce_message(DBusConnection *con, DBusMessage *msg, gpointer data)
>+{
>+  if (dbus_message_is_signal(msg, MCE_SIGNAL_IF, MCE_DEVICE_ORIENTATION_SIG)) {
>+    DBusMessageIter iter;
>+    if (dbus_message_iter_init(msg, &iter)) {
>+      const gchar *mode = NULL;
>+      dbus_message_iter_get_basic(&iter, &mode);
>+
>+      nsCOMPtr<nsIDOMWindowInternal> window;
>+      GetMostRecentWindow(NS_LITERAL_STRING("").get(), getter_AddRefs(window));
>+      OssoSetWindowOrientation(WidgetForDOMWindow(window), strcmp(mode, "portrait") == 0);

I'm not confident in the use of GetMostRecentWindow here when there are
multiple windows, particularly when ending portrait mode.
OssoSetWindowOrientation only changes the property on one window, but
_HILDON_PORTRAIT_MODE_REQUEST is set on every window, and when the property
remains on any "visible" window the device stays in portrait mode.

Also I wonder whether the most recent may be a transient window and thus its
property may be ignored.

Is it possible to iterate over all toplevel windows?
Or can the windows listen for changes in the "orientation" from system-info?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #19)
> (From update of attachment 436116 [details] [diff] [review])
> >-  osso_hw_state_t m_hw_state;
> >+  osso_hw_state_t m_hw_state;  
> 
> Accidental extra whitespace.

Fixed

> > static void OssoDisplayCallback(osso_display_state_t state, gpointer data)
> > {
> >   nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
> >   if (!os)
> >       return;
> >- 
> >-  if (state == OSSO_DISPLAY_ON)
> >+
> >+  osso_context_t* context = (osso_context_t*) data;
> >+
> >+  if (state == OSSO_DISPLAY_ON) {
> >       os->NotifyObservers(nsnull, "system-display-on", nsnull);
> >-  else
> >+      OssoRequestAccelerometer(context, PR_TRUE);
> >+  } else {
> 
> Here we have the possible situation where the orientation changed while the
> display was off/dimmed, so the orientation should be checked here I assume.
> 
> Note that the current state is available from MCE_ACCELEROMETER_ENABLE_REQ
> (which is when you want to know it):
> 
> "If this request message indicates reply requested (dbus_no_reply is set
> FALSE), then reply message is sent with current orientation. This eliminates
> the need for separate MCE_DEVICE_ORIENTATION_GET when starting listening
> orientation."
> 
> http://maemo.org/api_refs/5.0/5.0-final/mce-dev/dbus-names_8h.html#dde12c93c5957d39e127047cc430e034

OK. I am adding code to get the orientation in OssoRequestAccelerometer, and updating the window. This will fix the issue of changing orientation while the screen is dimmed.

> So checking the orientation in OssoRequestAccelerometer() is possible, so
> OssoIsPortrait() and nsNativeAppSupportUnix::Enable() are probably not
> necessary.

I think I still need nsNativeAppSupportUnix::Enable() so I can update the orientation of the initial window. It is created after nsNativeAppSupportUnix::Start() and I can't update the system-info in nsNativeAppSupportUnix::Start() because XPCOM is not ready yet.

> >+  // Enable the accelerometer for orientation support
> >+  if (OssoIsScreenOn(m_osso_context))
> >+      OssoRequestAccelerometer(m_osso_context, PR_TRUE);
> >+
> >   osso_hw_set_event_cb(m_osso_context, nsnull, OssoHardwareCallback, &m_hw_state);
> >-  osso_hw_set_display_event_cb(m_osso_context, OssoDisplayCallback, nsnull);
> >+  osso_hw_set_display_event_cb(m_osso_context, OssoDisplayCallback, m_osso_context);
> >   osso_rpc_set_default_cb_f(m_osso_context, OssoDbusCallback, nsnull);
> >+
> >+  // Setup an MCE callback to monitor orientation
> >+  DBusConnection *connnection = (DBusConnection*)osso_get_sys_dbus_connection(m_osso_context);
> >+  dbus_bus_add_match(connnection, MCE_MATCH_RULE, nsnull);
> >+  dbus_connection_add_filter(connnection, dbus_handle_mce_message, nsnull, nsnull);
> 
> Documentation may be suggesting that the match rule should be added before
> MCE_ACCELEROMETER_ENABLE_REQ, though I'm not clear on what constitutes a
> "listener".

I moved the code around

> >+dbus_handle_mce_message(DBusConnection *con, DBusMessage *msg, gpointer data)
> >+{
> >+  if (dbus_message_is_signal(msg, MCE_SIGNAL_IF, MCE_DEVICE_ORIENTATION_SIG)) {
> >+    DBusMessageIter iter;
> >+    if (dbus_message_iter_init(msg, &iter)) {
> >+      const gchar *mode = NULL;
> >+      dbus_message_iter_get_basic(&iter, &mode);
> >+
> >+      nsCOMPtr<nsIDOMWindowInternal> window;
> >+      GetMostRecentWindow(NS_LITERAL_STRING("").get(), getter_AddRefs(window));
> >+      OssoSetWindowOrientation(WidgetForDOMWindow(window), strcmp(mode, "portrait") == 0);
> 
> I'm not confident in the use of GetMostRecentWindow here when there are
> multiple windows, particularly when ending portrait mode.
> OssoSetWindowOrientation only changes the property on one window, but
> _HILDON_PORTRAIT_MODE_REQUEST is set on every window, and when the property
> remains on any "visible" window the device stays in portrait mode.
> 
> Also I wonder whether the most recent may be a transient window and thus its
> property may be ignored.
> 
> Is it possible to iterate over all toplevel windows?
> Or can the windows listen for changes in the "orientation" from system-info?

We might be able to handle multiple windows somehow, but for Fennec there is only ever one window. Can I file a followup bug for multiple windows? I'd like to land this patch before code freeze on April 5th.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to comment #20)
> > So checking the orientation in OssoRequestAccelerometer() is possible, so
> > OssoIsPortrait() and nsNativeAppSupportUnix::Enable() are probably not
> > necessary.
> 
> I think I still need nsNativeAppSupportUnix::Enable() so I can update the
> orientation of the initial window. It is created after
> nsNativeAppSupportUnix::Start() and I can't update the system-info in
> nsNativeAppSupportUnix::Start() because XPCOM is not ready yet.

Ah.  Thanks for explaining.  I hadn't grasped that we didn't have XPCOM and
system-info yet.

How about moving the first OssoRequestAccelerometer() from Start() to
Enable()?  It seems clearer to have the first accelerometer-enable and
initial state check in the same place.

> We might be able to handle multiple windows somehow, but for Fennec there is
> only ever one window. Can I file a followup bug for multiple windows? I'd like
> to land this patch before code freeze on April 5th.

OK.  Can you add a comment please to indicate that the use of
GetMostRecentWindow is designed for apps with only one window? 
(Provided there is a comment, I don't mind whether we have a follow-up bug
report or not.)

>+static void
>+OssoSetWindowOrientation(GtkWidget* widget, PRBool aPortrait)
>+{
>+  // Tell Hildon desktop to force our window to be either portrait or landscape,
>+  // depending on the current rotation
>+  if (widget && widget->window) {
>+    GdkWindow *gdk = widget->window;
>+    GdkAtom request = gdk_atom_intern("_HILDON_PORTRAIT_MODE_REQUEST", FALSE);
>+
>+    nsCOMPtr<nsIWritablePropertyBag2> info = do_GetService("@mozilla.org/system-info;1");
>+
>+    if (aPortrait) {
>+      gulong portrait_set = 1;
>+      gdk_property_change(gdk, request, gdk_x11_xatom_to_atom(XA_CARDINAL),
>+                          32, GDK_PROP_MODE_REPLACE, (const guchar *) &portrait_set, 1);
>+      if (info)
>+        info->SetPropertyAsAString(NS_LITERAL_STRING("orientation"), NS_LITERAL_STRING("portrait"));
>+    }
>+    else {
>+      gdk_property_delete(gdk, request);
>+      if (info)
>+        info->SetPropertyAsAString(NS_LITERAL_STRING("orientation"), NS_LITERAL_STRING("landscape"));
>+    }
>+  }

Can you separate the system-info code from the |if (widget && widget->window)|
test please?  That way this'll still work if the window is created after
enable.

The property setting can then be

info->SetPropertyAsAString(NS_LITERAL_STRING("orientation"),
                           aPortrait ? NS_LITERAL_STRING("portrait")
                             : NS_LITERAL_STRING("landscape"));

(or similar for your favorite ?: style).

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)
> (In reply to comment #20)

> > I think I still need nsNativeAppSupportUnix::Enable() so I can update the
> > orientation of the initial window. It is created after
> > nsNativeAppSupportUnix::Start() and I can't update the system-info in
> > nsNativeAppSupportUnix::Start() because XPCOM is not ready yet.
> 
> Ah.  Thanks for explaining.  I hadn't grasped that we didn't have XPCOM and
> system-info yet.
> 
> How about moving the first OssoRequestAccelerometer() from Start() to
> Enable()?  It seems clearer to have the first accelerometer-enable and
> initial state check in the same place.

I moved it and was able to remove some redundant code too.

> > We might be able to handle multiple windows somehow, but for Fennec there is
> > only ever one window. Can I file a followup bug for multiple windows? I'd like
> > to land this patch before code freeze on April 5th.
> 
> OK.  Can you add a comment please to indicate that the use of
> GetMostRecentWindow is designed for apps with only one window? 
> (Provided there is a comment, I don't mind whether we have a follow-up bug
> report or not.)

All window update code has been localized to OssoSetWindowOrientation so it's easier to make changes later. I added the comment there too.

> >+static void
> >+OssoSetWindowOrientation(GtkWidget* widget, PRBool aPortrait)

> >+  }
> 
> Can you separate the system-info code from the |if (widget && widget->window)|
> test please?  That way this'll still work if the window is created after
> enable.

Done

The latest patch properly handles initial top-level window and handles updating the orientation when the screen is dimmed.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 6

patch addresses review comments

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Karl - I just realized I can remove OssoIsPortrait now. It is unused. If your review goes well, I can remove it before checkin.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 436417 [details] [diff] [review]
patch 6

>-  osso_context_t *m_osso_context;    
>+  osso_context_t *m_osso_context;
>   /* A note about why we need to have m_hw_state:
>      the osso hardware callback does not tell us what changed, just
>      that something has changed.  We need to keep track of state so
>      that we can determine what has changed.
>   */  
>-  osso_hw_state_t m_hw_state;
>+  osso_hw_state_t m_hw_state;  

Looks like you removed a different accidental whitespace.

>+GtkWidget*
>+WidgetForDOMWindow(nsISupports *aWindow)
>+{
>+  nsCOMPtr<nsPIDOMWindow> domWindow(do_QueryInterface(aWindow));
>+  if (!domWindow)
>+    return 0;
>+
>+  nsCOMPtr<nsIBaseWindow> baseWindow = do_QueryInterface(domWindow->GetDocShell());
>+  if (!baseWindow)
>+    return 0;
>+
>+  nsCOMPtr<nsIWidget> widget;
>+  baseWindow->GetMainWidget(getter_AddRefs(widget));
>+  if (!widget)
>+    return 0;

Usually we use NULL for GObject pointers such as these.

>+  // NOTE: We only update the most recent top-level window

Add something like: "This is only suitable for apps with only one window."
(Otherwise someone might assume that updating one window is always good
enough.)

>+    info->SetPropertyAsAString(NS_LITERAL_STRING("orientation"),

Don't forget dougt's "screen-orientation" drive-by.

>+OssoSetWindowOrientation(PRBool aPortrait)

This could take a string argument.  Boolean parameters are hard to interpret
from the caller, and it can save duplicating strcmp(something, "portrait"),
but whatever you decide.

>+static PRBool OssoIsScreenOn(osso_context_t* ctx)
>+{
>+  osso_rpc_t ret;
>+  PRBool result = PR_FALSE;
>+
>+  if (osso_rpc_run_system(ctx, MCE_SERVICE, MCE_REQUEST_PATH, MCE_REQUEST_IF, "get_display_status", &ret, DBUS_TYPE_INVALID) == OSSO_OK) {

I like 80 columns.  There are already plenty of lines over 80 in this file,
but this is a particularly long one that could be easily shortened.

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 436417 [details] [diff] [review]
patch 6

>+GtkWidget*
>+WidgetForDOMWindow(nsISupports *aWindow)

static

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for checkin

Updated for review comments

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/12f125711253

pushed to m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8688a65bc7ce

Comment 29

[Aakash Desai [:aakashd]]

verified FIXED On builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100402 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100402 Namoroka/3.7a4pre Fennec/1.1a2pre

litmus testcase: https://litmus.mozilla.org/show_test.cgi?id=7531 regression tests this bug


follow-up bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=556822