790274 - Remove Portrait and Landscape special ScreenOrientation values

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

There is no meaning in having these two values being special values now that the user can compose orientations using an array of strings.

Comment 1

[Jason Smith [:jsmith]]

(In reply to Mounir Lamouri (:mounir) from comment #0)
> There is no meaning in having these two values being special values now that
> the user can compose orientations using an array of strings.

Not even for a purpose of having a shortcut to specify two orientations in one in one word instead of two? Or are we aiming for consistency in how this is specified in the API and manifest (i.e. explicitly define each orientation to allow rather than allow shortcuts).

Comment 2

[Mounir Lamouri (:mounir)]

Sorry, I should have been more explicit: this is only for the inner code, that isn't going to change the API. Right now, the code should consider "portrait" as "portrait-secondary" | "portrait-primary" instead of special-casing it because the consumer of the API could also pass ["portrait-secondary", "portrait-primary"].

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 660422 [details] [diff] [review]
Patch

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

::: dom/base/nsScreen.cpp
@@ +278,5 @@
>  
> +  NS_ASSERTION(mOrientation | (eScreenOrientation_PortraitPrimary |
> +                               eScreenOrientation_PortraitSecondary |
> +                               eScreenOrientation_LandscapePrimary |
> +                               eScreenOrientation_LandscapeSecondary),

This assert was wrong. I fixed it by checking that mOrientation is *equal* to any of those values.

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 660422 [details] [diff] [review]
Patch

Yay.

>diff --git a/dom/base/ScreenOrientation.h b/dom/base/ScreenOrientation.h
>--- a/dom/base/ScreenOrientation.h
>+++ b/dom/base/ScreenOrientation.h
>@@ -13,17 +13,15 @@ namespace dom {
> // Make sure that any change here is also made in
> // * mobile/android/base/GeckoScreenOrientationListener.java
> // * embedding/android/GeckoScreenOrientationListener.java
> typedef uint32_t ScreenOrientation;
> 
> static const ScreenOrientation eScreenOrientation_None               = 0;
> static const ScreenOrientation eScreenOrientation_PortraitPrimary    = 1;  // 00000001
> static const ScreenOrientation eScreenOrientation_PortraitSecondary  = 2;  // 00000010
>-static const ScreenOrientation eScreenOrientation_Portrait           = 3;  // 00000011
> static const ScreenOrientation eScreenOrientation_LandscapePrimary   = 4;  // 00000100
> static const ScreenOrientation eScreenOrientation_LandscapeSecondary = 8;  // 00001000
>-static const ScreenOrientation eScreenOrientation_Landscape          = 12; // 00001100

Can we use PR_BIT, while we're at it?

>diff --git a/dom/base/nsScreen.cpp b/dom/base/nsScreen.cpp
>--- a/dom/base/nsScreen.cpp
>+++ b/dom/base/nsScreen.cpp
>@@ -271,19 +271,20 @@ nsScreen::GetAvailRect(nsRect& aRect)
> }
> 
> void
> nsScreen::Notify(const hal::ScreenConfiguration& aConfiguration)
> {
>   ScreenOrientation previousOrientation = mOrientation;
>   mOrientation = aConfiguration.orientation();
> 
>-  NS_ASSERTION(mOrientation != eScreenOrientation_None &&
>-               mOrientation != eScreenOrientation_Portrait &&
>-               mOrientation != eScreenOrientation_Landscape,
>+  NS_ASSERTION(mOrientation | (eScreenOrientation_PortraitPrimary |
>+                               eScreenOrientation_PortraitSecondary |
>+                               eScreenOrientation_LandscapePrimary |
>+                               eScreenOrientation_LandscapeSecondary),

Um.  I think you want something like

NS_ASSERTION(!(mOrientation & ~(portraitprimary | portraitsecondary | ...)));

But to be clear, the old code didn't accept portraitPrimary all by itself; you
want to change that behavior here, right?

>@@ -301,33 +302,32 @@ nsScreen::Notify(const hal::ScreenConfig

> NS_IMETHODIMP
> nsScreen::GetMozOrientation(nsAString& aOrientation)
> {
>   switch (mOrientation) {
>-    case eScreenOrientation_None:
>-    case eScreenOrientation_Portrait:
>-    case eScreenOrientation_Landscape:
>-      NS_ASSERTION(false, "Shouldn't be used when getting value!");
>-      return NS_ERROR_FAILURE;
>     case eScreenOrientation_PortraitPrimary:
>       aOrientation.AssignLiteral("portrait-primary");
>       break;
>     case eScreenOrientation_PortraitSecondary:
>       aOrientation.AssignLiteral("portrait-secondary");
>       break;
>     case eScreenOrientation_LandscapePrimary:
>       aOrientation.AssignLiteral("landscape-primary");
>       break;
>     case eScreenOrientation_LandscapeSecondary:
>       aOrientation.AssignLiteral("landscape-secondary");
>       break;
>+    case eScreenOrientation_None:
>+    default:
>+      MOZ_NOT_REACHED();
>+      return NS_ERROR_FAILURE;

Note that this will cause undefined behavior in release builds if we ever get
here.  I'm kind of tempted to MOZ_ASSERT(false); instead, because at least then
we're guaranteed to do the right thing in case of a bug.

Hm.  I am now less convinced that this was a good change to make than I was
initially.  I did not expect (portraitPrimary | portraitSecondary) to be used
in all the ways that it's used.  :-/

Comment 6

[Justin Lebar (not reading bugmail)]

> This assert was wrong. I fixed it by checking that mOrientation is *equal* to any of those values.

Much better, thanks.  :)

Comment 7

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6125b883672b

Comment 8

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/6125b883672b