add orientation keyword 'default' to express normal orientation

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: viralwang, Assigned: viralwang)

Tracking

({dev-doc-complete})

unspecified
1.3 Sprint 2 - 10/11
ARM
Gonk (Firefox OS)
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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.
(Assignee)

Updated

5 years ago
Blocks: 905004
(Assignee)

Comment 2

5 years ago
Created attachment 797151 [details] [diff] [review]
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.
Attachment #797151 - Flags: review?(mounir)
(Assignee)

Comment 3

5 years ago
Created attachment 797154 [details] [diff] [review]
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.
Attachment #797154 - Flags: review?(jonas)
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
Attachment #797154 - Flags: review?(jonas) → review?(mounir)
(Assignee)

Updated

5 years ago
Blocks: 911668

Updated

5 years ago
blocking-b2g: koi? → koi+
Keywords: dev-doc-needed
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?
Attachment #797151 - Flags: review?(mounir) → review-
Attachment #797154 - Flags: review?(mounir)
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 801492 [details] [diff] [review]
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!
Attachment #797151 - Attachment is obsolete: true
Attachment #797154 - Attachment is obsolete: true
Attachment #801492 - Flags: review?(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.
Attachment #801492 - Flags: review?(mounir) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 802193 [details] [diff] [review]
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!
Attachment #801492 - Attachment is obsolete: true
Attachment #802193 - Flags: review?(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.
Attachment #802193 - Flags: review?(mounir)
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 809061 [details] [diff] [review]
add orientation keyword 'default' to express normal orientation (v4)

Support gonk/android for this new orientation keyword 'default'
Attachment #802193 - Attachment is obsolete: true
Attachment #809061 - Flags: review?(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?
Attachment #809061 - Flags: review?(mounir)
Attachment #809061 - Flags: review?(blassey.bugs)
Attachment #809061 - Flags: review+
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?
Attachment #809061 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 810420 [details] [diff] [review]
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!
Attachment #809061 - Attachment is obsolete: true
Attachment #810420 - Flags: review?(blassey.bugs)
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
Attachment #810420 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 811785 [details] [diff] [review]
Bug 908058 - add orientation keyword 'default' to express normal orientation (final)

add reviewer in comment.
Attachment #810420 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/11ab102e7e87
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 2 - 10/11
https://hg.mozilla.org/releases/mozilla-aurora/rev/5721fe0d5284
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.