908058 - add orientation keyword 'default' to express normal orientation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[viral [:viralwang]]

We would like to add a new keyword 'default' to the orientation API.
The 'default' keyword is either mapped to 'portrait-primary' or 'landscape-primary', it will depend on the device.
For example, if the panel resolution is 1280*800, 'default' will make it landscape, if the resolution is 800*1280, 'default' will make it to portrait.

Now we have to consider the cases in
1) orientation in manifest
2) screen.lockOrientation

It should make System app of Gaia working on the portrait phone and landscape tablet in same code.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

We need API to know default orientation means landscape or portrait.
Because system app needs to rotate the window during opening and closing transition, and "default" makes us don't know which degree to rotate.

This needs gaia side fix otherwise the transition would fail.

Comment 2

[viral [:viralwang]]

Attachment: add orientation keyword 'default' to express normal orientation

We add 'default' keyword mapped to either 'portrait-primary' or 'landscape-primary' in OrientationObserver base on width and height of screen.

Comment 3

[viral [:viralwang]]

Attachment: add comments on mozLockOrientation

add comments on mozLockOrientation in webidl, it can be landscape-primary, landscape-secondary, landscape, portrait-primary, portrait-secondary, portrait, or default.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 797154 [details] [diff] [review]
add comments on mozLockOrientation

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

Mounir has owned this API so far

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 797151 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation

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

Instead of using this ugly global variable, why don't use lock the screen to DEFAULT and then the hal backend we decide what DEFAULT means. Then, nsScreen.cpp will get an event about the new screen orientation. No need for the DOM to know about what DEFAULT means.

::: dom/base/ScreenOrientation.h
@@ +16,5 @@
>  static const ScreenOrientation eScreenOrientation_PortraitPrimary    = 1u << 0;
>  static const ScreenOrientation eScreenOrientation_PortraitSecondary  = 1u << 1;
>  static const ScreenOrientation eScreenOrientation_LandscapePrimary   = 1u << 2;
>  static const ScreenOrientation eScreenOrientation_LandscapeSecondary = 1u << 3;
> +static const ScreenOrientation eScreenOrientation_default = 1u << 4;

nit: style

::: dom/base/nsScreen.cpp
@@ +330,5 @@
>        orientation |= eScreenOrientation_LandscapePrimary;
>      } else if (item.EqualsLiteral("landscape-secondary")) {
>        orientation |= eScreenOrientation_LandscapeSecondary;
> +    } else if (item.EqualsLiteral("default")) {
> +      orientation |=eScreenOrientation_default;

nit: style

::: widget/gonk/OrientationObserver.cpp
@@ +27,5 @@
>  
>  using namespace mozilla;
>  using namespace dom;
>  
> +extern bool naturallyPortrait;

Why are you using a global variable here?

Comment 6

[viral [:viralwang]]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> ::: widget/gonk/OrientationObserver.cpp
> @@ +27,5 @@
> >  
> >  using namespace mozilla;
> >  using namespace dom;
> >  
> > +extern bool naturallyPortrait;
> 
> Why are you using a global variable here?

Thank you for your feedback!

My original thought is set the 'default' in nsScreen::MozLockOrientation() and check what the 'default' means in OrientationObserver::LockScreenOrientation().

I'm trying to get the panel resolution in OrientationObserver::LockScreenOrientation().

Firstly, I would like to used the API we have now as below:

nsCOMPtr<nsIScreen> screen = GetPrimaryScreen();
screen->GetRect(&left, &top, &width, &height)

However, the width and height is variant since the 'sVirtualBounds' shows the information about the current windows, like 1280*800 in landscape and 800*1280 in portrait.

So we should use the information of width and height from 'gScreenBounds' which is static.
We can not access 'gScreenBounds' here, that's why I choose a global variable to save the panel is portrait or landscape even though it is ugly.

Comment 7

[viral [:viralwang]]

Attachment: add orientation keyword 'default' to express normal orientation (v2)

Hi Mounir,

I think we can used the information we saved in DetectDefaultOrientation().
When we need to LockScreenOrientation to 'default', we can set it as portrait or landscape directly.

Thank you!

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 801492 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation (v2)

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

We need every backend to be updated before landing this.

::: dom/base/ScreenOrientation.h
@@ +16,5 @@
>  static const ScreenOrientation eScreenOrientation_PortraitPrimary    = 1u << 0;
>  static const ScreenOrientation eScreenOrientation_PortraitSecondary  = 1u << 1;
>  static const ScreenOrientation eScreenOrientation_LandscapePrimary   = 1u << 2;
>  static const ScreenOrientation eScreenOrientation_LandscapeSecondary = 1u << 3;
> +static const ScreenOrientation eScreenOrientation_default            = 1u << 4;

nit: eScreenOrientation_Default

::: widget/gonk/OrientationObserver.cpp
@@ +267,5 @@
> +                             eScreenOrientation_default));
> +
> +  if (aOrientation == eScreenOrientation_default) {
> +    aOrientation = (sOrientationOffset == sDefaultPortrait) ? eScreenOrientation_PortraitPrimary :
> +            eScreenOrientation_LandscapePrimary;

nit: indentation seems pretty messy around here.

Comment 9

[viral [:viralwang]]

Attachment: add orientation keyword 'default' to express normal orientation (v3)

Hi Mounir,

Could you please help to share more about the backend in your suggestion?
My original thought is to enable this 'default' orientation, and then Gaia team can start to move some of their setting to 'default'. I'm not so sure about what the backend we should check.
Thank you!

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 802193 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation (v3)

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

There are other backends for the Screen Orientation API. See the code in hal/. For every backend, we should support this new keyword, otherwise we can't ship it.

Comment 11

[viral [:viralwang]]

Hi Mounir,

I think android is the only backend we need to consider besides b2g.

I follow the instructions below to build android application.
https://wiki.mozilla.org/Mobile/Fennec/Android#Linux

I'm trying to add android:screenOrientation in mobile/android/base/AndroidManifest.xml.in to check the orientation behavior.
I expect we can used same attribute (landscape-primary, portrait-primary...) like we used in b2g.
However, I can not use it by adding android:screenOrientation="landscape-primary" in xml file.

I'm thinking about android is using standard api should not include MozLockOrientation.
I would like to write a test app and use Fennec on android to verify.

Comment 12

[viral [:viralwang]]

Attachment: add orientation keyword 'default' to express normal orientation (v4)

Support gonk/android for this new orientation keyword 'default'

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 809061 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation (v4)

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

Brad, could you review the Android changes?

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 809061 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation (v4)

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

r=blassey assuming my assumption is correct, r- othewise

::: dom/base/ScreenOrientation.h
@@ +16,5 @@
>  static const ScreenOrientation eScreenOrientation_PortraitPrimary    = 1u << 0;
>  static const ScreenOrientation eScreenOrientation_PortraitSecondary  = 1u << 1;
>  static const ScreenOrientation eScreenOrientation_LandscapePrimary   = 1u << 2;
>  static const ScreenOrientation eScreenOrientation_LandscapeSecondary = 1u << 3;
> +static const ScreenOrientation eScreenOrientation_Default            = 1u << 4;

it might be good to add a comment as to what "default" means. I'm taking it to mean that we're ignoring the sensor, is that correct?

Comment 15

[viral [:viralwang]]

Attachment: Bug 908058 - add orientation keyword 'default' to express normal orientation (v5)

Hi Brad,

the "default" means either "portrait-primary" or "landscape-primary", it will depend on the display resolution of devices.

If display resolution is 1280*800, "default" means landscape, if the resolution is 800*1280, "default" means portrait.

For android backend, I use SCREEN_ORIENTATION_NOSENSOR directly. It should be same as our expect.

Thank you for your suggestion!

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 810420 [details] [diff] [review]
Bug 908058 - add orientation keyword 'default' to express normal orientation (v5)

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

thanks, comment makes it clearer

Comment 17

[viral [:viralwang]]

Attachment: Bug 908058 - add orientation keyword 'default' to express normal orientation (final)

add reviewer in comment.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/11ab102e7e87

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11ab102e7e87

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5721fe0d5284

Comment 21

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/Screen.lockOrientation
and
https://developer.mozilla.org/en-US/Firefox/Releases/26