Last Comment Bug 784549 - Screen Orientation API: lockOrientation() should accept an Array in addition of a DOMString
: Screen Orientation API: lockOrientation() should accept an Array in addition ...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Mounir Lamouri (:mounir)
: Nobody; OK to take it and work on it
Mentors:
Depends on: 787532 790274 792957
Blocks: 784574
  Show dependency treegraph
 
Reported: 2012-08-21 16:55 PDT by Jason Smith [:jsmith]
Modified: 2013-08-23 02:57 PDT (History)
9 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
-


Attachments
Patch (10.64 KB, patch)
2012-08-31 12:14 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch v2 (13.20 KB, patch)
2012-09-08 09:52 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2012-08-21 16:55:25 PDT
Steps:

1. Go to testmanifest.com
2. Create a web app with more than one orientation property specified (e.g. portrait-primary,landscape-primary)
3. Install the app
4. Launch the app
5. Switch the orientation of the found to each orientation

Expected:

The orientation should lock to only the items called out in the comma-separated list. Other orientations cannot be accessed.

Actual:

A portrait-primary orientation is used instead no matter what configuration is used.

Additional Notes:

Also saw this similar issue when I provided illegal characters after the comma with portrait-primary,<illegal>landscape-secondary. Are we even handling the case for the comma-separated list?
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-22 10:36:54 PDT
The way this *should* work is as follows:

By default, pages should be locked to the orientations enumerated in the manifest. I.e. if a page specifies "landscape, portrait-primary" then we should allow orienting in the landscape-primary, landscape-secondary and portrait-primary orientations.

If a page then calls lockOrientation from javascript, then the value passed to that function should override what's in the manifest. Once the page calls unlockOrientation we should switch back to using the values from the manifest.

Note that lockOrientation also takes a comma-separated list of orientations.
Comment 2 Mounir Lamouri (:mounir) 2012-08-29 08:48:06 PDT
The DOM API doesn't allow a comma-separated list of orientations nor does the spec.
I could fix that if we think that's needed. The Android backend isn't going to support all possibilities there but the Gonk backend might.
Comment 3 Jason Smith [:jsmith] 2012-08-29 09:54:54 PDT
(In reply to Mounir Lamouri (:mounir) from comment #2)
> The DOM API doesn't allow a comma-separated list of orientations nor does
> the spec.

? The spec here - http://mozilla.github.com/webapps-spec/#optional-properties states that the orientation property uses a comma-separated list. We confirmed this in triage as well. It's in debate though right now to be considered to be an array instead, but that's a separate issue.

> I could fix that if we think that's needed. The Android backend isn't going
> to support all possibilities there but the Gonk backend might.

Why wouldn't the Android backend support all of the possibilities? We recently landed support for the orientation property on Android.
Comment 4 Wesley Johnston (:wesj) 2012-08-29 09:58:11 PDT
(In reply to Jason Smith [:jsmith] from comment #3)
> Why wouldn't the Android backend support all of the possibilities? We
> recently landed support for the orientation property on Android.
The API's Android gives us don't translate well over to this. Making it work involves basically rewriting things ourselves and trying to get information about the device that Android doesn't make easily available (AFAIK). Since supporting something like "portrait-secondary,landscape-secondary" or "portrait-primary,landscape-secondary" is a pretty rare thing, its not a priority.
Comment 5 Mounir Lamouri (:mounir) 2012-08-29 10:20:14 PDT
(In reply to Jason Smith [:jsmith] from comment #3)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > The DOM API doesn't allow a comma-separated list of orientations nor does
> > the spec.
> 
> ? The spec here -
> http://mozilla.github.com/webapps-spec/#optional-properties states that the
> orientation property uses a comma-separated list. We confirmed this in
> triage as well. It's in debate though right now to be considered to be an
> array instead, but that's a separate issue.

No, I meant http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html
Both specs have to match.

I think we want to support this. However, I'm not sure if that *has* to be done for Basecamp. Depending on other priorities, I could work on that.
Comment 6 Jason Smith [:jsmith] 2012-08-29 10:27:13 PDT
(In reply to Mounir Lamouri (:mounir) from comment #5)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > (In reply to Mounir Lamouri (:mounir) from comment #2)
> > > The DOM API doesn't allow a comma-separated list of orientations nor does
> > > the spec.
> > 
> > ? The spec here -
> > http://mozilla.github.com/webapps-spec/#optional-properties states that the
> > orientation property uses a comma-separated list. We confirmed this in
> > triage as well. It's in debate though right now to be considered to be an
> > array instead, but that's a separate issue.
> 
> No, I meant
> http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html
> Both specs have to match.

Oh, good point. Can you and Anant figure out how to resolve the spec differences here?

> 
> I think we want to support this. However, I'm not sure if that *has* to be
> done for Basecamp. Depending on other priorities, I could work on that.

Fair enough, I agree. Putting this back on the nomination queue then.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-29 10:36:36 PDT
I agree, this shouldn't block basecamp.
Comment 8 Jason Smith [:jsmith] 2012-08-29 10:40:47 PDT
(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Mounir Lamouri (:mounir) from comment #5)
> > (In reply to Jason Smith [:jsmith] from comment #3)
> > > (In reply to Mounir Lamouri (:mounir) from comment #2)
> > > > The DOM API doesn't allow a comma-separated list of orientations nor does
> > > > the spec.
> > > 
> > > ? The spec here -
> > > http://mozilla.github.com/webapps-spec/#optional-properties states that the
> > > orientation property uses a comma-separated list. We confirmed this in
> > > triage as well. It's in debate though right now to be considered to be an
> > > array instead, but that's a separate issue.
> > 
> > No, I meant
> > http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html
> > Both specs have to match.
> 
> Oh, good point. Can you and Anant figure out how to resolve the spec
> differences here?

Actually, dug into this with Anant. The screen orientation API does support a comma-separated list as per the wiki - https://wiki.mozilla.org/WebAPI/ScreenOrientation. Am I misreading something?
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-29 14:26:46 PDT
My recollection too was that we wanted a comma-separated list. But I admit that my memory could be more wishful thinking.
Comment 10 Mounir Lamouri (:mounir) 2012-08-29 14:32:11 PDT
Yes indeed. I just specified what we had implemented given that going from one value to a list of value is backward-compatible.
Comment 11 Mounir Lamouri (:mounir) 2012-08-31 12:13:39 PDT
I'm going to mute that bug in having lockOrientation taking an array.
Comment 12 Mounir Lamouri (:mounir) 2012-08-31 12:14:18 PDT
Created attachment 657391 [details] [diff] [review]
Patch
Comment 13 Justin Lebar (not reading bugmail) 2012-09-04 13:39:18 PDT
Comment on attachment 657391 [details] [diff] [review]
Patch

>diff --git a/dom/interfaces/base/nsIDOMScreen.idl b/dom/interfaces/base/nsIDOMScreen.idl
>--- a/dom/interfaces/base/nsIDOMScreen.idl
>+++ b/dom/interfaces/base/nsIDOMScreen.idl
>@@ -1,16 +1,16 @@
> /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsIDOMEventTarget.idl"
> 
>-[scriptable, builtinclass, uuid(5a8294df-ffe4-48e5-803f-f57bebc29289)]
>+[scriptable, builtinclass, uuid(01e8587b-35a9-4a59-8349-c7ee93846fb2)]
> interface nsIDOMScreen : nsIDOMEventTarget
> {
>   readonly attribute long             top;
>   readonly attribute long             left;
>   readonly attribute long             width;
>   readonly attribute long             height;
>   readonly attribute long             pixelDepth;
>   readonly attribute long             colorDepth;
>@@ -25,16 +25,17 @@ interface nsIDOMScreen : nsIDOMEventTarg
>    *         portrait-primary or portrait-secondary.
>    */
>   readonly attribute DOMString       mozOrientation;
> 
>   [implicit_jscontext] attribute jsval      onmozorientationchange;
> 
>   /**
>    * Lock screen orientation to the specified type.
>+   * The parameter can be a DOMString or an Array of DOMString.

Nit: s/of DOMString/of DOMStrings/ or "DOMString's", if you prefer.

But more generally, can we write a real comment here explaining what's going
on, since the specs are still in flux?

You could say something like

  Lock the screen to the specified orientations(s).  This method returns true
  if the lock was acquired successfully, and false otherwise.

  The parameter can be a DOMString or an Array of DOMStrings.  If you pass a
  string, we lock the screen to that one orientation.  If you pass an Array, we
  ensure that the screen is always in one of the given orientations.

  Valid orientations are "portrait", "portrait-primary", "portrait-secondary",
  "landscape", "landscape-primary", and "landscape-secondary".  These tokens
  are case-sensitive.

  If you pass a string that's not one of the valid orientations, or if you pass
  an array of orientations and any of the orientations in the array is
  not valid, we reject the lock and return false.

  The "-primary" orientations correspond to holding the device right-side up,
  while the "-secondary" orientations correspond to holding the device
  upside-down.  Locking the orientation in "portrait" is the same as locking
  the orientation in ['portrait-primary', 'portrait-secondary'], and the
  "landscape" orientation similarly corresponds to the set
  ['landscape-primary', 'landscape-secondary'].


>diff --git a/dom/base/ScreenOrientation.h b/dom/base/ScreenOrientation.h
>--- a/dom/base/ScreenOrientation.h
>+++ b/dom/base/ScreenOrientation.h
>+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
>+static const ScreenOrientation eScreenOrientation_EndGuard           = 15; // 00001111

Presumably you don't need the end guard now that you're not using an enum?

>diff --git a/dom/base/nsScreen.cpp b/dom/base/nsScreen.cpp
>--- a/dom/base/nsScreen.cpp
>+++ b/dom/base/nsScreen.cpp
>@@ -8,16 +8,17 @@
> #include "nsIDocShell.h"
> #include "nsPresContext.h"
> #include "nsCOMPtr.h"
> #include "nsDOMClassInfoID.h"
> #include "nsIDocShellTreeItem.h"
> #include "nsLayoutUtils.h"
> #include "nsDOMEvent.h"
> #include "nsGlobalWindow.h"
>+#include "nsJSUtils.h"
> 
> using namespace mozilla;
> using namespace mozilla::dom;
> 
> namespace {
> 
> bool
> IsChromeType(nsIDocShell *aDocShell)
>@@ -325,35 +326,70 @@ nsScreen::GetMozOrientation(nsAString& a
>       aOrientation.AssignLiteral("landscape-secondary");
>       break;
>   }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>-nsScreen::MozLockOrientation(const nsAString& aOrientation, bool* aReturn)
>+nsScreen::MozLockOrientation(const jsval& aOrientation, JSContext* aCx, bool* aReturn)

I'm not too comfortable with the JSAPI, so someone else should double-check this method.

>+  nsAutoTArray<nsString, 2> orientations;

If you really care about performance here (it's not clear you do, but I
remember you talking about this), you might as well make this 8 or something.
Stack space is very cheap.

>+  if (aOrientation.isString()) {
>+    nsDependentJSString item;
>+    item.init(aCx, aOrientation.toString());
>+    orientations.AppendElement(item);
>   } else {
>-    return NS_OK;
>+    // We assume we have an array at that point.
>+    NS_ENSURE_TRUE(aOrientation.isObject(), NS_ERROR_INVALID_ARG);

We're not assuming anything -- in fact, we're doing a check.  How about

      // If we don't have a string, we must have an object (in particular, an
      // Array).

>+    JSObject& obj = aOrientation.toObject();
>+    uint32_t length;
>+    NS_ENSURE_TRUE(JS_GetArrayLength(aCx, &obj, &length) && length > 0,
>+                   NS_ERROR_INVALID_ARG);
>+
>+    orientations.SetLength(length);

You want SetCapacity, not SetLength; SetLength will insert |length| empty
strings at the front of the array.  In fact, I'm surprised any tests pass with
this...

>+    for (uint32_t i=0; i<length; ++i) {

                    i = 0; i < length;

>+      jsval value;
>+      NS_ENSURE_TRUE(JS_GetElement(aCx, &obj, i, &value), NS_ERROR_UNEXPECTED);
>+      NS_ENSURE_TRUE(value.isString(), NS_ERROR_INVALID_ARG);
>+
>+      nsDependentJSString item;
>+      item.init(aCx, value);
>+      orientations.AppendElement(item);
>+    }
>+  }
>+
>+  ScreenOrientation orientation = eScreenOrientation_None;
>+
>+  for (uint32_t i=0; i<orientations.Length(); ++i) {
>+    nsString& item = orientations[i];
>+
>+    if (item.EqualsLiteral("portrait")) {
>+      orientation |= eScreenOrientation_Portrait;

It would be a lot clearer to me if we didn't have the plain Portrait/Landscape
enum entries and had only the *{Primary,Secondary} entries.  Then we could
explicitly do an |or| here, and never use the non-primary/secondary enums
anywhere else.  That is, you'd do

       orientation |= eScreenOrientation_PortraitPrimary |
                      eScreenOrientation_PortraitSecondary;

At the moment, we have this weird situation where we have a "portrait"
orientation but the screen's current orientation can never be "portrait"!

>+    } else {
>+      // If we don't know that the token, we should just return 'false' without
>+      // throwing.

s/know that/recognize/

>diff --git a/dom/base/test/test_screen_orientation.html b/dom/base/test/test_screen_orientation.html
>--- a/dom/base/test/test_screen_orientation.html
>+++ b/dom/base/test/test_screen_orientation.html

Oh, it looks like you're not testing locking /to/ an array of strings.  That
probably explains why the test isn't failing due to the SetLength().  :)

It's sad that we don't have any positive tests here.  Can we add them and run
them just on Android?  Or can we give QA a manual test to run.  Clearly there's
the opportunity for bugs here.
Comment 14 Mounir Lamouri (:mounir) 2012-09-08 09:52:51 PDT
Created attachment 659497 [details] [diff] [review]
Patch v2

What about a follow-up for portrait/landscape removal?
Comment 15 Justin Lebar (not reading bugmail) 2012-09-08 11:09:21 PDT
> What about a follow-up for portrait/landscape removal?

sgtm
Comment 16 Justin Lebar (not reading bugmail) 2012-09-08 11:14:42 PDT
> NS_IMETHODIMP
>-nsScreen::MozLockOrientation(const nsAString& aOrientation, bool* aReturn)
>+nsScreen::MozLockOrientation(const jsval& aOrientation, JSContext* aCx, bool* aReturn)

I'd still like someone who knows JSAPI to look over this, if you don't mind.

>+    for (uint32_t i=0; i<length; ++i) {

I commented on the spacing here in my previous review.

r=me assuming that you tested this and it actually works this time, and that
you get someone to look over the JSAPI bits for sanity.
Comment 18 Mounir Lamouri (:mounir) 2012-09-11 07:13:21 PDT
The JSAPI bits have been reviewed by khuey during the MozCamp Europe.
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-09-11 18:47:13 PDT
https://hg.mozilla.org/mozilla-central/rev/bc30225833c4
Comment 20 Jason Smith [:jsmith] 2012-09-12 17:26:11 PDT
Arrays appear to be recognized that contain singular entities into the app manifest for orientation, although the original intention behind this bug wasn't fixed - multiple orientations in the array still don't work and revert back to using portrait-primary when multiple orientations is used (example below).

Mounir - Any ideas?

{
  "name":"Test App ({subdomain})",
  "description":"This app has been automatically generated by testmanifest.com",
  "version":"1.0",
  "icons":{
    "16":"http://testmanifest.com/icon-16.png",
    "48":"http://testmanifest.com/icon-48.png",
    "128":"http://testmanifest.com/icon-128.png"
  },
  "installs_allowed_from":[
    "*"
  ],
  "orientation": ["landscape-primary", "portrait-secondary"],
  "developer":{
    "name":"Gregory Koberger",
    "url":"http://gkoberger.net"
  }
}
Comment 21 Mounir Lamouri (:mounir) 2012-09-13 02:26:11 PDT
This is not about manifest but about the Screen Orientation API. You should check that screen.mozLockOrientation() works with an array. It is only expected to work on Android. I don't think manifest handles that and do not expect this to work on B2G/Gaia.
Comment 22 Jason Smith [:jsmith] 2012-09-13 11:24:40 PDT
(In reply to Mounir Lamouri (:mounir) from comment #21)
> This is not about manifest but about the Screen Orientation API. You should
> check that screen.mozLockOrientation() works with an array. It is only
> expected to work on Android. I don't think manifest handles that and do not
> expect this to work on B2G/Gaia.

Ah okay. I transferred the original issue for the multiple orientations over to Gaia. If this is Android, then I'm unassigning myself for now (for context - we're targeting verifications primarily on basecamp+ blockers).
Comment 23 Jason Smith [:jsmith] 2012-09-13 11:25:05 PDT
(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Mounir Lamouri (:mounir) from comment #21)
> > This is not about manifest but about the Screen Orientation API. You should
> > check that screen.mozLockOrientation() works with an array. It is only
> > expected to work on Android. I don't think manifest handles that and do not
> > expect this to work on B2G/Gaia.
> 
> Ah okay. I transferred the original issue for the multiple orientations over
> to Gaia. If this is Android, then I'm unassigning myself for now (for
> context - we're targeting verifications primarily on basecamp+ blockers).

One additional comment - or other areas that touch B2G specifically.
Comment 24 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-09-20 14:09:31 PDT
I'm getting mochitest errors with test_screen_orientation.html:
http://people.mozilla.org/~mwargers/mochitestjs2/test_screen_orientation.html
It complains about not exceptions being fired, when they should fire.

But afaict, the spec says no exceptions should fire: http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html#methods

Also, I'm confused why tinderbox isn't red, because this mochitest is failing.
Comment 25 Justin Lebar (not reading bugmail) 2012-09-20 14:16:49 PDT
> http://people.mozilla.org/~mwargers/mochitestjs2/test_screen_orientation.html
> It complains about not exceptions being fired, when they should fire.

The test is green for me.
Comment 26 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-09-20 14:47:00 PDT
Ok, weird, it's now working also for me after a browser restart. I did some weird stuff beforehand, so a restart might have fixed this.

The thing is that I'm still seeing it happening in b2g mochitest.
Comment 27 Justin Lebar (not reading bugmail) 2012-09-20 14:52:55 PDT
> The thing is that I'm still seeing it happening in b2g mochitest.

Sounds like we should discuss this further in a new bug, then.
Comment 28 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-09-20 14:53:35 PDT
I guess that might be happening, because of using a too old build.
And now that I'm saying that, the trunk build that I used was also too old!
Sorry for the confusion.

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