Last Comment Bug 776600 - Implement the "orientation" property of the app manifest for web apps on Android
: Implement the "orientation" property of the app manifest for web apps on Android
Status: RESOLVED FIXED
[blocking-webrtandroid1?]
: dev-doc-needed
Product: Firefox for Android
Classification: Client Software
Component: Web Apps (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 17
Assigned To: Wesley Johnston (:wesj)
: Aaron Train [:aaronmt]
: Myk Melez [:myk] [@mykmelez]
Mentors:
Depends on:
Blocks: Apps-Dev-Doc-Needed
  Show dependency treegraph
 
Reported: 2012-07-23 10:46 PDT by Jason Smith [:jsmith]
Modified: 2012-08-24 16:56 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
-


Attachments
WIP Patch (21.15 KB, patch)
2012-07-27 14:29 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Path 1/3 (7.69 KB, patch)
2012-07-31 15:08 PDT, Wesley Johnston (:wesj)
blassey.bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 2/3 (7.44 KB, patch)
2012-07-31 15:11 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch (7.56 KB, patch)
2012-07-31 15:16 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 3/3 - Webapps.jsm changes (708 bytes, patch)
2012-07-31 15:17 PDT, Wesley Johnston (:wesj)
felipc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2012-07-23 10:46:01 PDT
Implement the orientation property for web apps on Android. See https://developer.mozilla.org/en/Apps/Manifest for a description of what the orientation property does. 

Note - Current docs (as of the time of this bug filing) are slightly off - we interpret these properties:

- portrait-primary
- landscape-primary
- portrait-secondary
- landscape-secondary
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-25 12:12:16 PDT
Note that the value is a comma-separated list.

We should support the following:

- portrait-primary

Locked to a single portrait direction. If the device has an obvious primary orientation in portrait mode, this is that orientation. If there is no obvious primary orientation due to landscape being the primary orientation for the device, this is the orientation if rotating the device 90 degrees clock-wise from the primary landscape mode.

- landscape-primary

Locked to a single landscape direction. If the device has an obvious primary orientation in landscape mode, this is that orientation. If there is no obvious landscape orientation due to portrait being the primary orientation for the device, this is the orientation if rotating the device 90 degrees clock-wise from the primary portrait mode.

- portrait-secondary

The portrait mode opposite of "portrait-primary".

- landscape-secondary

The landscape mode opposite of "landscape-primary".

- portrait

Equivalent to "portrait-primary,portrait-secondary".

- landscape

Equivalent to "landscape-primary,landscape-secondary".
Comment 2 Wesley Johnston (:wesj) 2012-07-25 17:34:00 PDT
I think I am going to set these as a "default" state. If the app changes them using DOM Api's, I'll honor the new settings. If they then unlock using DOM API's, we'll switch back to the manifest values? Flash fullscreen also locks the orientation, but I figure we can do the same thing? Honor Flash. Revert back when it unlocks? Sound ok?
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-26 14:59:00 PDT
That sounds awesome!
Comment 4 Jason Smith [:jsmith] 2012-07-26 15:00:31 PDT
Jonas - Do you know of a sample app that works on Firefox OS using the screen orientation manifest properties?
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-26 15:11:05 PDT
Unfortunately not.
Comment 6 Wesley Johnston (:wesj) 2012-07-27 14:29:39 PDT
Created attachment 646721 [details] [diff] [review]
WIP Patch

We have to rewrite our orientation implementation on mobile for this to work. The current spec allows you to specify something like primary_portrait,secondary_landscape and allow only those two orientations. AFAICT, Android doesn't allow that, so I'm trying to listen to rotation changes here, figure out what orientation they correspond to, and adjust.

Android doesn't seem to provide much info on what orientation a particular rotation value corresponds to either. I'm trying to use getRotation along with getConfiguration().orientation to determine the default state of the device. Seems to work, kinda sorta sometimes on the devices I have here, but I think I probably need something smarter.... or maybe we just say "Android won't let us do that" for now. Feedback/ideas blassey?
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-28 00:30:10 PDT
I'm totally fine with not having perfect support on Android for now. The only real-world app behavior that I've seen that would require a comma separated list is apps that want to allow all orientations except upside-down, so something like "primary-portrait,landscape". I think it would be ok behavior to simply ignore the orientation manifest property if we run into something like that for now.
Comment 8 Wesley Johnston (:wesj) 2012-07-31 15:08:24 PDT
Created attachment 647700 [details] [diff] [review]
Path 1/3

This allows setting the default orientation, and creates a pref, read at startup, to set it.
Comment 9 Wesley Johnston (:wesj) 2012-07-31 15:11:43 PDT
Created attachment 647705 [details] [diff] [review]
Patch 2/3

This gives us the ability to write a defaultprefs.js file on install of the app. Those prefs are set on first run. This relies on the patch in bug 769821.

App manifests don't actually have support for orientation yet. I'm assuming here that we'll just get back a string from them...? That's simple and will be part 3 of this patch. But maybe we'd rather do the parsing in Gecko?
Comment 10 Wesley Johnston (:wesj) 2012-07-31 15:16:44 PDT
Created attachment 647707 [details] [diff] [review]
Patch

Grr. Forgot to qref.
Comment 11 Wesley Johnston (:wesj) 2012-07-31 15:17:34 PDT
Created attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

This is the simplest way to add orientation support (I think?)
Comment 12 :Felipe Gomes (needinfo me!) 2012-08-01 10:26:14 PDT
Comment on attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

Looks good to me. Simple change, I don't think it needs extra review from someone from dom
Comment 13 Wesley Johnston (:wesj) 2012-08-03 16:54:17 PDT
blassey, I want to point out that I didn't fix our orientation implementation here. I can file a follow up for that, but didn't want it to hold up webapps stuff.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-06 15:22:08 PDT
Comment on attachment 647707 [details] [diff] [review]
Patch

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js
>+          defaultPrefs = WebappsUI.readDefaultPrefs(FileUtils.getFile("ProfD", [WebappsUI.DEFAULT_PREFS_FILENAME]));

Move "readDefaultPrefs" to WebAppRT since it's not used in WebappsUI
Add a second const DEFAULT_PREFS_FILENAME in WebAppRT (I don't like to share)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  DEFAULT_PREFS_FILENAME: "defaultprefs.js",

default-prefs.js"

>   doInstall: function doInstall(aData) {

>+        (function(icon) {
>           var profilePath = sendMessageToJava({

var -> let

>           var file = null;

var -> let

>           if (profilePath) {
>             var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);

"var" saved you here, but remove the "var" and don't redefine "file"

>+  writeDefaultPrefs: function webapps_writeDefaultPrefs(aFile, aPrefs) {

>+      var ostream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);

var -> let

>+  readDefaultPrefs: function webapps_readDefaultPrefs(aFile) {

>+    var prefsString = NetUtil.readInputStreamToString(fstream, fstream.available(), {});

var -> let
Comment 16 Wesley Johnston (:wesj) 2012-08-08 12:43:04 PDT
Comment on attachment 647707 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New Webapps stuff
User impact if declined: Orientation isn't used, but we're also interested in using this functionality to send other information from Fennec to the apps (for instance, about whether to telemetry ping).
Testing completed (on m-c, etc.): Landed on inbound 8/8
Risk to taking this patch (and alternatives if risky): Low risk. Webapp only
String or UUID changes made by this patch: None.
Comment 17 Jason Smith [:jsmith] 2012-08-08 15:05:05 PDT
Flagging qawanted for testing the current changes landing.
Comment 19 Brad Lassey [:blassey] (use needinfo?) 2012-08-09 10:58:10 PDT
This specifically doesn't effect fennec
Comment 20 Wesley Johnston (:wesj) 2012-08-09 15:02:31 PDT
Comment on attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

If this is a blocking feature this stuff should move forward. If its not then its probably better to just hold it. But the increased testing on Aurora would be good for us. (note there's still more work to finish this feature)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New webapps feature
User impact if declined: webapps asking for manifest.orientation will fail
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): fairly low. worst case it should return "".
String or UUID changes made by this patch: None
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-10 12:23:57 PDT
Comment on attachment 647707 [details] [diff] [review]
Patch

only affects webapps, approving for Aurora.
Comment 22 Wesley Johnston (:wesj) 2012-08-10 15:04:20 PDT
blassey ping? I know you said you had questions about one of my patches. Can you drop them in here?
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2012-08-15 15:40:20 PDT
Comment on attachment 647700 [details] [diff] [review]
Path 1/3

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

::: mobile/android/base/GeckoScreenOrientationListener.java
@@ +119,5 @@
>    }
>  
> +  public void handleMessage(String event, JSONObject message) {
> +      try {
> +          if (event.equals("Preferences:Data")) {

as discussed, this stupid. Let's fix it in a follow up.

@@ +153,5 @@
> +      return orientationFromString(orientations.get(0));
> +  }
> +
> +  private short orientationFromString(String val) {
> +    if (val.equals("portrait"))

"portrait".equals(val) 

same for the rest
Comment 24 Jason Smith [:jsmith] 2012-08-18 11:15:57 PDT
Note - I tried testing portrait as an app manifest property, but that did not work at all.
Comment 25 Wesley Johnston (:wesj) 2012-08-18 15:07:20 PDT
This has not all landed yet.
Comment 27 Ed Morley [:emorley] 2012-08-21 06:31:25 PDT
https://hg.mozilla.org/mozilla-central/rev/0255e9c7f468
Comment 28 Jason Smith [:jsmith] 2012-08-21 18:59:41 PDT
FYI to Aaron for testing:

I recently did testing on this feature for Firefox OS in a session-based testing style. You can see the results of the testing I did here - https://wiki.mozilla.org/B2G/QA/Sessions/Screen_Orientation_App_Manifest_Property_%2808/21/2012%29. In short, the easiest way to test this is using testmanifest.com and creating an orientation property in the manifest with different orientation combinations.
Comment 29 Aaron Train [:aaronmt] 2012-08-22 06:53:40 PDT
This doesn't seem to be working at all for me with the supplied property values in the manifest.

`{ "orientation" : "landscape" }` doesn't seem to work on my Galaxy Nexus — when I launch the application with my device in portrait position, the application launches in portrait positioning and is not locked on rotation.

`{"orientation" : "portrait"}`as well doesn't seem to work on my Galaxy Nexus - when I launch the application with my device in landscape position, the application launches in landscape positioning and is not locked on rotation.

The same both apply with `{"orientation" : "landscape-primary"}` and `{"orientation" : "portrait-primary"}`.

Am I missing something here?
Comment 30 Wesley Johnston (:wesj) 2012-08-22 13:09:01 PDT
Comment on attachment 647700 [details] [diff] [review]
Path 1/3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New stuffbr
User impact if declined: orientation in manifests don't work. 
Testing completed (on m-c, etc.): Landed today
Risk to taking this patch (and alternatives if risky): Medium risk. Some changes to our orientation support but nothing should be changed.
String or UUID changes made by this patch: None.
Comment 31 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-23 15:03:44 PDT
Comment on attachment 647700 [details] [diff] [review]
Path 1/3

Approving for Aurora due to need for webapp testing.
Comment 32 Wesley Johnston (:wesj) 2012-08-23 22:37:47 PDT
(In reply to Aaron Train [:aaronmt] from comment #29)
> This doesn't seem to be working at all for me with the supplied property
> values in the manifest.
> 
> `{ "orientation" : "landscape" }` doesn't seem to work on my Galaxy Nexus —
> when I launch the application with my device in portrait position, the
> application launches in portrait positioning and is not locked on rotation.
> 
> `{"orientation" : "portrait"}`as well doesn't seem to work on my Galaxy
> Nexus - when I launch the application with my device in landscape position,
> the application launches in landscape positioning and is not locked on
> rotation.
> 
> The same both apply with `{"orientation" : "landscape-primary"}` and
> `{"orientation" : "portrait-primary"}`.
> 
> Am I missing something here?

Hmm.. not sure. I tested by hardcoding a value, but I will look tomorrow.
Comment 33 Wesley Johnston (:wesj) 2012-08-23 23:54:55 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/27e1ecbbc6ed

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