Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash getting screen dimensions from gonk content process

RESOLVED FIXED in mozilla15

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla15
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 3 obsolete attachments)

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.)
Created attachment 621260 [details] [diff] [review]
WIP

This makes the gaia browser app work, with jlebar's patches applied.  Needs cleanup.
Assignee: nobody → jones.chris.g
Created attachment 621362 [details] [diff] [review]
WIP 2
Attachment #621260 - Attachment is obsolete: true
Created attachment 621373 [details] [diff] [review]
part 0a: Use the normal plugin-container for gonk
Attachment #621362 - Attachment is obsolete: true
Attachment #621373 - Flags: review?(mwu)
Created attachment 621374 [details] [diff] [review]
part 0b: Ensure that the gonk system workers are never started in content processes
Attachment #621374 - Flags: review?(philipp)
Created attachment 621375 [details] [diff] [review]
part 0c: Only use nsIScreen on the main thread
Attachment #621375 - Flags: review?(mwu)
Created attachment 621376 [details] [diff] [review]
part 1: Generalize ScreenOrientation into ScreenConfiguration, and add screen rect and color+pixel depth to it
Attachment #621376 - Flags: review?(mounir)
Created attachment 621378 [details] [diff] [review]
part 2: Implement the ScreenConfiguration hal for gonk
Attachment #621378 - Flags: review?(mounir)
Created attachment 621379 [details] [diff] [review]
part 3: Implement PuppetScreen* analogues to PuppetWidget
Attachment #621379 - Flags: superreview?(roc)
Attachment #621379 - Flags: review?(mwu)
Attachment #621374 - Flags: review?(philipp) → review+

Updated

5 years ago
Attachment #621373 - Flags: review?(mwu) → review+

Updated

5 years ago
Attachment #621375 - Flags: review?(mwu) → review+
Attachment #621379 - Flags: superreview?(roc) → superreview+

Updated

5 years ago
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.
Created attachment 621806 [details] [diff] [review]
part 4: Rename some things
Attachment #621806 - Flags: review?(mounir)
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)
Created attachment 621829 [details] [diff] [review]
part 4: Rename some things, v2

Yep, burned on try.  w00t
Attachment #621806 - Attachment is obsolete: true
Attachment #621829 - Flags: review?(mounir)
Attachment #621829 - Flags: review?(mounir) → review+
Created attachment 622091 [details] [diff] [review]
Rollup patch
Created attachment 622143 [details] [diff] [review]
Rollup that applies on cgjones github fork
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d963bae859
https://hg.mozilla.org/integration/mozilla-inbound/rev/94ebe5e63dd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ce3cbe0ca1
https://hg.mozilla.org/integration/mozilla-inbound/rev/251188d5a55c
https://hg.mozilla.org/integration/mozilla-inbound/rev/727b2eb545bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a53cca0a312
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d173a8f63e2
https://hg.mozilla.org/mozilla-central/rev/23d963bae859
https://hg.mozilla.org/mozilla-central/rev/94ebe5e63dd2 (empty changeset???)
https://hg.mozilla.org/mozilla-central/rev/61ce3cbe0ca1
https://hg.mozilla.org/mozilla-central/rev/251188d5a55c
https://hg.mozilla.org/mozilla-central/rev/727b2eb545bd
https://hg.mozilla.org/mozilla-central/rev/3a53cca0a312
https://hg.mozilla.org/mozilla-central/rev/4d173a8f63e2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Followup
https://hg.mozilla.org/mozilla-central/rev/dd29535bac5f

(Argh!  Got lost in rebase somehow.)
Blocks: 1183828
You need to log in before you can comment on or make changes to this bug.