Closed Bug 745145 Opened 12 years ago Closed 12 years ago

Crash getting screen dimensions from gonk content process

Categories

(Core :: Widget, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(9 files, 3 obsolete files)

2.24 KB, patch
mwu
: review+
Details | Diff | Splinter Review
1.09 KB, patch
philikon
: review+
Details | Diff | Splinter Review
2.99 KB, patch
mwu
: review+
Details | Diff | Splinter Review
30.43 KB, patch
mounir
: review+
Details | Diff | Splinter Review
10.07 KB, patch
mounir
: review+
Details | Diff | Splinter Review
10.32 KB, patch
mwu
: review+
Details | Diff | Splinter Review
4.34 KB, patch
mounir
: review+
Details | Diff | Splinter Review
56.12 KB, patch
Details | Diff | Splinter Review
43.13 KB, patch
Details | Diff | Splinter Review
See bug 714861 comment 50

(gdb) bt
#0  nsScreenGonk::GetPixelDepth (this=<value optimized out>, aPixelDepth=0xbec229b4) at /home/cjones/mozilla/b2g/glue/gonk-ics/frameworks/base/include/ui/FramebufferNativeWindow.h:55
#1  0x407606e6 in nsPrincipal::SubsumesIgnoringDomain (this=0x0, aOther=0xbec229b4, aResult=0x410dd1f0) at /home/cjones/mozilla/mozilla-central/caps/src/nsPrincipal.cpp:379
#2  0x40b36af6 in gfxAndroidPlatform::gfxAndroidPlatform (this=0x41c361c0) at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxAndroidPlatform.cpp:67
#3  0x40b2f8a8 in gfxPlatform::Init () at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxPlatform.cpp:299
#4  0x40b2fac8 in gfxPlatform::GetPlatform () at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxPlatform.cpp:255
#5  0x409e1238 in mozilla::widget::PuppetWidget::Create (this=0x41ceb440, aParent=0x0, aNativeParent=<value optimized out>, aRect=<value optimized out>, aHandleEventFunction=0, aContext=0x0, aInitData=0x0) at /home/cjones/mozilla/mozilla-central/widget/xpwidgets/PuppetWidget.cpp:127

This code was a hack we added for android, and now we have to pay the piper for gonk.
mwu, want to grab this after the |repo| stuff is finished?

It'll be like old times in multi-process android!!! yay!!

We can hack around this, but I think the best fix is to make add PuppetScreen analogue to PuppetWidget in PuppetWidget.cpp, and fetch the screen configuration through PHal.  We already have to send updates there for the screen-lock API.  The logic in PuppetScreen can be exactly the same as the gonk and android screens, except stripped of all platform-specific hackery.  Then we modify the XPCOM constructor to create the right nsIScreen depending on process type.

(For bonus points, we could refactor the shared logic in android/gonk/puppet into a BasicScreen or something.)
Attached patch WIP (obsolete) — Splinter Review
This makes the gaia browser app work, with jlebar's patches applied.  Needs cleanup.
Assignee: nobody → jones.chris.g
Attached patch WIP 2 (obsolete) — Splinter Review
Attachment #621260 - Attachment is obsolete: true
Attachment #621374 - Flags: review?(philipp) → review+
Attachment #621373 - Flags: review?(mwu) → review+
Attachment #621375 - Flags: review?(mwu) → review+
Attachment #621379 - Flags: superreview?(roc) → superreview+
Attachment #621379 - Flags: review?(mwu) → review+
Comment on attachment 621376 [details] [diff] [review]
part 1: Generalize ScreenOrientation into ScreenConfiguration, and add screen rect and color+pixel depth to it

Review of attachment 621376 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments applied

::: dom/base/nsScreen.cpp
@@ +47,5 @@
>  #include "nsDOMEvent.h"
>  
>  using namespace mozilla;
>  using namespace mozilla::dom;
> +using namespace mozilla::hal;

nit: I would prefer to keep |hal::| explicit. And I think you are only saving two |hal::| with this line.

::: dom/base/nsScreen.h
@@ -46,2 @@
>  #include "nsDOMEventTargetHelper.h"
> -#include "mozilla/Observer.h"

Why don't you need that anymore?

::: hal/android/AndroidHal.cpp
@@ +193,5 @@
> +    NS_ERROR("Can't find nsIScreenManager!");
> +    return;
> +  }
> +
> +  nsIntRect r;

s/r/rect/

@@ +195,5 @@
> +  }
> +
> +  nsIntRect r;
> +  PRInt32 colorDepth, pixelDepth;
> +  ScreenOrientation o;

s/o/orientation/

@@ +202,5 @@
> +  screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> +  screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> +  screen->GetColorDepth(&colorDepth);
> +  screen->GetPixelDepth(&pixelDepth);
> +  o = static_cast<ScreenOrientation>(bridge->GetScreenOrientation());

Don't static_cast here, that method should return the enum.

::: hal/fallback/FallbackScreenOrientation.cpp
@@ +29,5 @@
>      NS_ERROR("Can't find nsIScreenManager!");
>      return;
>    }
>  
> +  nsIntRect r;

s/r/rect/

@@ +31,5 @@
>    }
>  
> +  nsIntRect r;
> +  PRInt32 colorDepth, pixelDepth;
> +  dom::ScreenOrientation o;

s/o/orientation/, please :)

@@ +40,5 @@
> +  screen->GetColorDepth(&colorDepth);
> +  screen->GetPixelDepth(&pixelDepth);
> +  o = r.width >= r.height
> +      ? dom::eScreenOrientation_LandscapePrimary
> +      : dom::eScreenOrientation_PortraitPrimary;

nit: I think having something like that would follow the coding style better:
orientation = rect.width >= rect.height
                ? dom::...Landscape...
                : dom::...Portrait...

::: widget/android/AndroidBridge.cpp
@@ +2158,3 @@
>  {
>      ALOG_BRIDGE("AndroidBridge::GetScreenOrientation");
> +    return mJNIEnv->CallStaticShortMethod(mGeckoAppShellClass, jGetScreenOrientation);

As long as you are here, you should do:

  JNIEnv *env = AndroidBridge::GetJNIEnv();
  if (!env)
    return;

and use |env| instead of |mJNIEnv|.

@@ +2179,3 @@
>  {
>    ALOG_BRIDGE("AndroidBridge::LockScreenOrientation");
> +  mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jLockScreenOrientation, aOrientation);

ditto

::: widget/android/AndroidBridge.h
@@ +443,5 @@
>      // This method doesn't take a ScreenOrientation because it's an enum and
>      // that would require including the header which requires include IPC
>      // headers which requires including basictypes.h which requires a lot of
>      // changes...
> +    uint32_t GetScreenOrientation();

I would prefer if that was returning a |mozilla::dom::ScreenOrientation|.

@@ +448,3 @@
>      void EnableScreenOrientationNotifications();
>      void DisableScreenOrientationNotifications();
> +    void LockScreenOrientation(uint32_t aOrientation);

I would prefer if that was taking a |mozilla::dom::ScreenOrientation|.

::: widget/android/nsAppShell.cpp
@@ +574,5 @@
> +            NS_ERROR("Can't find nsIScreenManager!");
> +            break;
> +        }
> +
> +        nsIntRect r;

s/r/rect/

@@ +576,5 @@
> +        }
> +
> +        nsIntRect r;
> +        PRInt32 colorDepth, pixelDepth;
> +        dom::ScreenOrientation o;

s/o/orientation/

@@ +583,5 @@
> +        screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> +        screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> +        screen->GetColorDepth(&colorDepth);
> +        screen->GetPixelDepth(&pixelDepth);
> +        o = static_cast<dom::ScreenOrientation>(curEvent->ScreenOrientation());

It's the third occurrence of that code in this patch.
Could you write a helper that would prefer re-writing over and over that code?
Attachment #621376 - Flags: review?(mounir) → review+
Comment on attachment 621378 [details] [diff] [review]
part 2: Implement the ScreenConfiguration hal for gonk

Review of attachment 621378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments taken into account.

::: hal/Makefile.in
@@ -139,5 @@
>  # Fallbacks for backends implemented on Android only.
>  ifneq (android,$(MOZ_WIDGET_TOOLKIT))
> -CPPSRCS += \
> -  FallbackNetwork.cpp \
> -  FallbackScreenOrientation.cpp \

Could you actually rename that file to FallbackScreenConfiguration.cpp? Might be better to do that in the other patch.

@@ +142,5 @@
> +endif
> +
> +# Fallbacks for backends implemented on Gonk and Android only.
> +ifeq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> +CPPSRCS += FallbackScreenOrientation.cpp

I would prefer to add the file in the big ifeq/else/endif block above instead of adding a special "android and gonk only" block.

::: hal/gonk/GonkHal.cpp
@@ +615,5 @@
> +
> +bool
> +LockScreenOrientation(const dom::ScreenOrientation& aOrientation)
> +{
> +  // FIXME/bug XXXXXX: implement

Please, put the bug number here, it's 743638.

@@ +622,5 @@
> +
> +void
> +UnlockScreenOrientation()
> +{
> +  // FIXME/bug XXXXXX: implement

bug 743638

::: widget/gonk/nsScreenManagerGonk.h
@@ +46,5 @@
>  #include "nsIScreenManager.h"
>  
>  class nsScreenGonk : public nsBaseScreen
>  {
> +    typedef mozilla::hal::ScreenConfiguration ScreenConfiguration;

nit: IMO, |using mozilla;| and prefixing with |hal::| would be better.

::: widget/gonk/nsWindow.cpp
@@ +724,5 @@
>  
> +/*static*/ ScreenConfiguration
> +nsScreenGonk::GetConfiguration()
> +{
> +    ScreenOrientation o = ComputeOrientation(sScreenRotation,

s/o/orientation/
Attachment #621378 - Flags: review?(mounir) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> Comment on attachment 621376 [details] [diff] [review]
> part 1: Generalize ScreenOrientation into ScreenConfiguration, and add
> screen rect and color+pixel depth to it
> 
> Review of attachment 621376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments applied
> 
> ::: dom/base/nsScreen.cpp
> @@ +47,5 @@
> >  #include "nsDOMEvent.h"
> >  
> >  using namespace mozilla;
> >  using namespace mozilla::dom;
> > +using namespace mozilla::hal;
> 
> nit: I would prefer to keep |hal::| explicit. And I think you are only
> saving two |hal::| with this line.
> > ::: dom/base/nsScreen.h
> @@ -46,2 @@
> >  #include "nsDOMEventTargetHelper.h"
> > -#include "mozilla/Observer.h"
> 
> Why don't you need that anymore?
> 

Hal.h pulls it in.

> ::: hal/android/AndroidHal.cpp
> @@ +193,5 @@
> > +    NS_ERROR("Can't find nsIScreenManager!");
> > +    return;
> > +  }
> > +
> > +  nsIntRect r;
> 
> s/r/rect/
> > @@ +195,5 @@
> > +  }
> > +
> > +  nsIntRect r;
> > +  PRInt32 colorDepth, pixelDepth;
> > +  ScreenOrientation o;
> 
> s/o/orientation/
> > @@ +202,5 @@
> > +  screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> > +  screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> > +  screen->GetColorDepth(&colorDepth);
> > +  screen->GetPixelDepth(&pixelDepth);
> > +  o = static_cast<ScreenOrientation>(bridge->GetScreenOrientation());
> 
> Don't static_cast here, that method should return the enum.
>

This opens a Pandora's box of random includes of AndroidBridge.h in random places where dom::ScreenOrientation isn't usable.  You tried to change this before, remember?
 
> ::: hal/fallback/FallbackScreenOrientation.cpp
> @@ +29,5 @@
> >      NS_ERROR("Can't find nsIScreenManager!");
> >      return;
> >    }
> >  
> > +  nsIntRect r;
> 
> s/r/rect/
> > @@ +31,5 @@
> >    }
> >  
> > +  nsIntRect r;
> > +  PRInt32 colorDepth, pixelDepth;
> > +  dom::ScreenOrientation o;
> 
> s/o/orientation/, please :)
> > @@ +40,5 @@
> > +  screen->GetColorDepth(&colorDepth);
> > +  screen->GetPixelDepth(&pixelDepth);
> > +  o = r.width >= r.height
> > +      ? dom::eScreenOrientation_LandscapePrimary
> > +      : dom::eScreenOrientation_PortraitPrimary;
> 
> nit: I think having something like that would follow the coding style better:
> orientation = rect.width >= rect.height
>                 ? dom::...Landscape...
>                 : dom::...Portrait...
> 

I think this is the same syntax?

> ::: widget/android/AndroidBridge.cpp
> @@ +2158,3 @@
> >  {
> >      ALOG_BRIDGE("AndroidBridge::GetScreenOrientation");
> > +    return mJNIEnv->CallStaticShortMethod(mGeckoAppShellClass, jGetScreenOrientation);
> 
> As long as you are here, you should do:
> 
>   JNIEnv *env = AndroidBridge::GetJNIEnv();
>   if (!env)
>     return;
> 
> and use |env| instead of |mJNIEnv|.
> > @@ +2179,3 @@
> >  {
> >    ALOG_BRIDGE("AndroidBridge::LockScreenOrientation");
> > +  mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jLockScreenOrientation, aOrientation);
> 
> ditto
> > ::: widget/android/AndroidBridge.h
> @@ +443,5 @@
> >      // This method doesn't take a ScreenOrientation because it's an enum and
> >      // that would require including the header which requires include IPC
> >      // headers which requires including basictypes.h which requires a lot of
> >      // changes...
> > +    uint32_t GetScreenOrientation();
> 
> I would prefer if that was returning a |mozilla::dom::ScreenOrientation|.
> 

That would be a massive pain.  I started down that path and gave up when I had to rewrite the world.

> @@ +448,3 @@
> >      void EnableScreenOrientationNotifications();
> >      void DisableScreenOrientationNotifications();
> > +    void LockScreenOrientation(uint32_t aOrientation);
> 
> I would prefer if that was taking a |mozilla::dom::ScreenOrientation|.
> 

See above.

> ::: widget/android/nsAppShell.cpp
> @@ +574,5 @@
> > +            NS_ERROR("Can't find nsIScreenManager!");
> > +            break;
> > +        }
> > +
> > +        nsIntRect r;
> 
> s/r/rect/
> > @@ +576,5 @@
> > +        }
> > +
> > +        nsIntRect r;
> > +        PRInt32 colorDepth, pixelDepth;
> > +        dom::ScreenOrientation o;
> 
> s/o/orientation/
> > @@ +583,5 @@
> > +        screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> > +        screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> > +        screen->GetColorDepth(&colorDepth);
> > +        screen->GetPixelDepth(&pixelDepth);
> > +        o = static_cast<dom::ScreenOrientation>(curEvent->ScreenOrientation());
> 
> It's the third occurrence of that code in this patch.
> Could you write a helper that would prefer re-writing over and over that
> code?

I don't think this logic is worth sharing.  And it's not really clear how it could be shared.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> Comment on attachment 621378 [details] [diff] [review]
> part 2: Implement the ScreenConfiguration hal for gonk
> 
> Review of attachment 621378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments taken into account.
> 
> ::: hal/Makefile.in
> @@ -139,5 @@
> >  # Fallbacks for backends implemented on Android only.
> >  ifneq (android,$(MOZ_WIDGET_TOOLKIT))
> > -CPPSRCS += \
> > -  FallbackNetwork.cpp \
> > -  FallbackScreenOrientation.cpp \
> 
> Could you actually rename that file to FallbackScreenConfiguration.cpp?
> Might be better to do that in the other patch.
> 

Yeah ok.

> @@ +142,5 @@
> > +endif
> > +
> > +# Fallbacks for backends implemented on Gonk and Android only.
> > +ifeq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> > +CPPSRCS += FallbackScreenOrientation.cpp
> 
> I would prefer to add the file in the big ifeq/else/endif block above
> instead of adding a special "android and gonk only" block.
> 

Why?  This is simpler.

In the future, we should split this up into a switch statement and mix 'n match for each platform.  But I'd rather do that in a followup.

> ::: hal/gonk/GonkHal.cpp
> @@ +615,5 @@
> > +
> > +bool
> > +LockScreenOrientation(const dom::ScreenOrientation& aOrientation)
> > +{
> > +  // FIXME/bug XXXXXX: implement
> 
> Please, put the bug number here, it's 743638.
> 

I knew you would know that! :)

✓

> @@ +622,5 @@
> > +
> > +void
> > +UnlockScreenOrientation()
> > +{
> > +  // FIXME/bug XXXXXX: implement
> 
> bug 743638
> > ::: widget/gonk/nsScreenManagerGonk.h
> @@ +46,5 @@
> >  #include "nsIScreenManager.h"
> >  
> >  class nsScreenGonk : public nsBaseScreen
> >  {
> > +    typedef mozilla::hal::ScreenConfiguration ScreenConfiguration;
> 
> nit: IMO, |using mozilla;| and prefixing with |hal::| would be better.
> 

The style here is what's used the in the rest of the widget code.  When in Rome ...

> ::: widget/gonk/nsWindow.cpp
> @@ +724,5 @@
> >  
> > +/*static*/ ScreenConfiguration
> > +nsScreenGonk::GetConfiguration()
> > +{
> > +    ScreenOrientation o = ComputeOrientation(sScreenRotation,
> 
> s/o/orientation/
(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10)
> > @@ +202,5 @@
> > > +  screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> > > +  screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> > > +  screen->GetColorDepth(&colorDepth);
> > > +  screen->GetPixelDepth(&pixelDepth);
> > > +  o = static_cast<ScreenOrientation>(bridge->GetScreenOrientation());
> > 
> > Don't static_cast here, that method should return the enum.
> >
> 
> This opens a Pandora's box of random includes of AndroidBridge.h in random
> places where dom::ScreenOrientation isn't usable.  You tried to change this
> before, remember?

Arf, indeed...

> > @@ +40,5 @@
> > > +  screen->GetColorDepth(&colorDepth);
> > > +  screen->GetPixelDepth(&pixelDepth);
> > > +  o = r.width >= r.height
> > > +      ? dom::eScreenOrientation_LandscapePrimary
> > > +      : dom::eScreenOrientation_PortraitPrimary;
> > 
> > nit: I think having something like that would follow the coding style better:
> > orientation = rect.width >= rect.height
> >                 ? dom::...Landscape...
> >                 : dom::...Portrait...
> > 
> 
> I think this is the same syntax?

Notice the two spaces before '?' and ':'.

> > ::: widget/android/AndroidBridge.h
> > @@ +443,5 @@
> > >      // This method doesn't take a ScreenOrientation because it's an enum and
> > >      // that would require including the header which requires include IPC
> > >      // headers which requires including basictypes.h which requires a lot of
> > >      // changes...
> > > +    uint32_t GetScreenOrientation();
> > 
> > I would prefer if that was returning a |mozilla::dom::ScreenOrientation|.
> > 
> 
> That would be a massive pain.  I started down that path and gave up when I
> had to rewrite the world.

Could you change "take" to "use" in the comment and "This method" to "These methods".

> @@ +583,5 @@
> > > +        screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> > > +        screen->GetRect(&r.x, &r.y, &r.width, &r.height);
> > > +        screen->GetColorDepth(&colorDepth);
> > > +        screen->GetPixelDepth(&pixelDepth);
> > > +        o = static_cast<dom::ScreenOrientation>(curEvent->ScreenOrientation());
> > 
> > It's the third occurrence of that code in this patch.
> > Could you write a helper that would prefer re-writing over and over that
> > code?
> 
> I don't think this logic is worth sharing.  And it's not really clear how it
> could be shared.

I will not fight for that but I believe this might be annoying to maintain if we add stuff to ScreenConfiguration.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> > @@ +142,5 @@
> > > +endif
> > > +
> > > +# Fallbacks for backends implemented on Gonk and Android only.
> > > +ifeq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> > > +CPPSRCS += FallbackScreenOrientation.cpp
> > 
> > I would prefer to add the file in the big ifeq/else/endif block above
> > instead of adding a special "android and gonk only" block.
> > 
> 
> Why?  This is simpler.

Simpler but a door for ugly things. Adding "FallbackScreenOrientation.cpp" to windows, linux and macos doesn't seem too hard, you just have to add that at the right place in the Makefile...
Blocks: 743638
> > > @@ +40,5 @@
> > > > +  screen->GetColorDepth(&colorDepth);
> > > > +  screen->GetPixelDepth(&pixelDepth);
> > > > +  o = r.width >= r.height
> > > > +      ? dom::eScreenOrientation_LandscapePrimary
> > > > +      : dom::eScreenOrientation_PortraitPrimary;
> > > 
> > > nit: I think having something like that would follow the coding style better:
> > > orientation = rect.width >= rect.height
> > >                 ? dom::...Landscape...
> > >                 : dom::...Portrait...
> > > 
> > 
> > I think this is the same syntax?
> 
> Notice the two spaces before '?' and ':'.
> 

I'm not quite sure what you were requesting but I changed this.

> > > ::: widget/android/AndroidBridge.h
> > > @@ +443,5 @@
> > > >      // This method doesn't take a ScreenOrientation because it's an enum and
> > > >      // that would require including the header which requires include IPC
> > > >      // headers which requires including basictypes.h which requires a lot of
> > > >      // changes...
> > > > +    uint32_t GetScreenOrientation();
> > > 
> > > I would prefer if that was returning a |mozilla::dom::ScreenOrientation|.
> > > 
> > 
> > That would be a massive pain.  I started down that path and gave up when I
> > had to rewrite the world.
> 
> Could you change "take" to "use" in the comment and "This method" to "These
> methods".
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11)
> > > @@ +142,5 @@
> > > > +endif
> > > > +
> > > > +# Fallbacks for backends implemented on Gonk and Android only.
> > > > +ifeq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> > > > +CPPSRCS += FallbackScreenOrientation.cpp
> > > 
> > > I would prefer to add the file in the big ifeq/else/endif block above
> > > instead of adding a special "android and gonk only" block.
> > > 
> > 
> > Why?  This is simpler.
> 
> Simpler but a door for ugly things. Adding "FallbackScreenOrientation.cpp"
> to windows, linux and macos doesn't seem too hard, you just have to add that
> at the right place in the Makefile...

I made this change.  If it bounces on tryserver, it will not have been worth the time.
Comment on attachment 621806 [details] [diff] [review]
part 4: Rename some things

Review of attachment 621806 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to add FallbackScreenConfiguration.cpp to linux target.

::: hal/fallback/FallbackScreenOrientation.cpp
@@ +40,5 @@
>    screen->GetColorDepth(&colorDepth);
>    screen->GetPixelDepth(&pixelDepth);
>    orientation = rect.width >= rect.height
> +                ? dom::eScreenOrientation_LandscapePrimary
> +                : dom::eScreenOrientation_PortraitPrimary;

I meant something like:

orientation = rect.width >= rect.height
                ? dom::eScreenOrientation_LandscapePrimary
                : dom::eScreenOrientation_PortraitPrimary;

But really, this is a nit you can ignore.
Attachment #621806 - Flags: review?(mounir)
Yep, burned on try.  w00t
Attachment #621806 - Attachment is obsolete: true
Attachment #621829 - Flags: review?(mounir)
Attachment #621829 - Flags: review?(mounir) → review+
Blocks: 1183828
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: