Closed
Bug 745145
Opened 12 years ago
Closed 12 years ago
Crash getting screen dimensions from gonk content process
Categories
(Core :: Widget, defect)
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+
roc
:
superreview+
|
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.)
Assignee | ||
Comment 2•12 years ago
|
||
This makes the gaia browser app work, with jlebar's patches applied. Needs cleanup.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jones.chris.g
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #621260 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #621362 -
Attachment is obsolete: true
Attachment #621373 -
Flags: review?(mwu)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #621374 -
Flags: review?(philipp)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #621375 -
Flags: review?(mwu)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #621376 -
Flags: review?(mounir)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #621378 -
Flags: review?(mounir)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #621379 -
Flags: superreview?(roc)
Attachment #621379 -
Flags: review?(mwu)
Updated•12 years ago
|
Attachment #621374 -
Flags: review?(philipp) → review+
Updated•12 years ago
|
Attachment #621373 -
Flags: review?(mwu) → review+
Updated•12 years ago
|
Attachment #621375 -
Flags: review?(mwu) → review+
Attachment #621379 -
Flags: superreview?(roc) → superreview+
Updated•12 years ago
|
Attachment #621379 -
Flags: review?(mwu) → review+
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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+
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
(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...
Assignee | ||
Comment 15•12 years ago
|
||
> > > @@ +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.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #621806 -
Flags: review?(mounir)
Comment 17•12 years ago
|
||
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)
Assignee | ||
Comment 18•12 years ago
|
||
Yep, burned on try. w00t
Attachment #621806 -
Attachment is obsolete: true
Attachment #621829 -
Flags: review?(mounir)
Updated•12 years ago
|
Attachment #621829 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Assignee | ||
Comment 21•12 years ago
|
||
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
Comment 22•12 years ago
|
||
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
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Assignee | ||
Comment 23•12 years ago
|
||
Followup https://hg.mozilla.org/mozilla-central/rev/dd29535bac5f (Argh! Got lost in rebase somehow.)
You need to log in
before you can comment on or make changes to this bug.
Description
•