Last Comment Bug 745145 - Crash getting screen dimensions from gonk content process
: Crash getting screen dimensions from gonk content process
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
:
Mentors:
Depends on:
Blocks: 743638 b2g-e10s-work 1183828
  Show dependency treegraph
 
Reported: 2012-04-13 03:51 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2015-07-14 13:46 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (16.82 KB, patch)
2012-05-05 00:11 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP 2 (38.25 KB, patch)
2012-05-05 18:14 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 0a: Use the normal plugin-container for gonk (2.24 KB, patch)
2012-05-05 20:23 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mwu.code: review+
Details | Diff | Splinter Review
part 0b: Ensure that the gonk system workers are never started in content processes (1.09 KB, patch)
2012-05-05 20:24 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
philipp: review+
Details | Diff | Splinter Review
part 0c: Only use nsIScreen on the main thread (2.99 KB, patch)
2012-05-05 20:24 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mwu.code: review+
Details | Diff | Splinter Review
part 1: Generalize ScreenOrientation into ScreenConfiguration, and add screen rect and color+pixel depth to it (30.43 KB, patch)
2012-05-05 20:25 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mounir: review+
Details | Diff | Splinter Review
part 2: Implement the ScreenConfiguration hal for gonk (10.07 KB, patch)
2012-05-05 20:25 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mounir: review+
Details | Diff | Splinter Review
part 3: Implement PuppetScreen* analogues to PuppetWidget (10.32 KB, patch)
2012-05-05 20:26 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mwu.code: review+
roc: superreview+
Details | Diff | Splinter Review
part 4: Rename some things (4.92 KB, patch)
2012-05-07 17:37 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 4: Rename some things, v2 (4.34 KB, patch)
2012-05-07 18:18 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mounir: review+
Details | Diff | Splinter Review
Rollup patch (56.12 KB, patch)
2012-05-08 12:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Rollup that applies on cgjones github fork (43.13 KB, patch)
2012-05-08 14:28 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-13 03:51:29 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-16 23:52:02 PDT
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.)
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 00:11:08 PDT
Created attachment 621260 [details] [diff] [review]
WIP

This makes the gaia browser app work, with jlebar's patches applied.  Needs cleanup.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 18:14:44 PDT
Created attachment 621362 [details] [diff] [review]
WIP 2
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:23:57 PDT
Created attachment 621373 [details] [diff] [review]
part 0a: Use the normal plugin-container for gonk
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:24:21 PDT
Created attachment 621374 [details] [diff] [review]
part 0b: Ensure that the gonk system workers are never started in content processes
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:24:42 PDT
Created attachment 621375 [details] [diff] [review]
part 0c: Only use nsIScreen on the main thread
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:25:07 PDT
Created attachment 621376 [details] [diff] [review]
part 1: Generalize ScreenOrientation into ScreenConfiguration, and add screen rect and color+pixel depth to it
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:25:36 PDT
Created attachment 621378 [details] [diff] [review]
part 2: Implement the ScreenConfiguration hal for gonk
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-05 20:26:06 PDT
Created attachment 621379 [details] [diff] [review]
part 3: Implement PuppetScreen* analogues to PuppetWidget
Comment 10 Mounir Lamouri (:mounir) 2012-05-07 10:41:09 PDT
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?
Comment 11 Mounir Lamouri (:mounir) 2012-05-07 11:06:27 PDT
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/
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-07 16:41:05 PDT
(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.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-07 16:45:41 PDT
(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/
Comment 14 Mounir Lamouri (:mounir) 2012-05-07 17:00:27 PDT
(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...
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-07 17:36:48 PDT
> > > @@ +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 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-07 17:37:54 PDT
Created attachment 621806 [details] [diff] [review]
part 4: Rename some things
Comment 17 Mounir Lamouri (:mounir) 2012-05-07 18:05:51 PDT
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.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-07 18:18:36 PDT
Created attachment 621829 [details] [diff] [review]
part 4: Rename some things, v2

Yep, burned on try.  w00t
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-08 12:41:30 PDT
Created attachment 622091 [details] [diff] [review]
Rollup patch
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-08 14:28:03 PDT
Created attachment 622143 [details] [diff] [review]
Rollup that applies on cgjones github fork
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-08 21:38:00 PDT
Followup
https://hg.mozilla.org/mozilla-central/rev/dd29535bac5f

(Argh!  Got lost in rebase somehow.)

Note You need to log in before you can comment on or make changes to this bug.