Based on comment #2, this may not be a regression but a change in the window manager. The contacts app is always in portrait mode. In 1.3, this meant that when picking an image we were always locked in portrait mode as well.
Is it intentional that in 1.4 we are able to change orientation like this as part of the pick activity? Needinfo on Alive for the answer to that.
Does the gallery app correctly handle orientation changes while cropping a photo when you edit it? If so, then this is only broken during a pick activity, not in the general case.

Peter,
In the 1/9 build when the bug did not reproduce, was that because, the orientation would not change at all? Or did the orientation change, but the image was resized correctly?
In the former case, it means that a window manager change (or bug) has exposed a missing feature that we've never needed before. In the later case this is just a regression.

(In reply to Peter Bylenga from comment #6)
> In the 1/9 build the bug did not reproduce due to the fact that the
> orientation would not change. Sorry about that I tried to call that out in
> Comment 2.
That implies this is likely a window manager regression with the fact that were allowing a portrait-locked app to rotate to landscape.
This might be a dupe also - I think we fixed an issue like this recently.

Yes, the change is to fix bug 948226. The rationale now is: if activity's manifest has orientation specified, use it. Otherwise use the activity caller's orientation.
I will update the algorithm again to fix this and not cause bug 948226.
Russ are you working on this or just find out the regression range?

I was working on it, initially to find the change that caused the regression. But I'd like to understand the changes you made for bug #948226. I'm not seeing that the changes correspond to the logic in your comment #10. They way I see the changes, they change which manifest to examine -- activity's manifest first then activity caller's -- they don't consider whether the activity's manifest has orientation before considering whether the activity caller's manifest has orientation. Because even if this.manifest.orientation is undefined, the code decides to use this.manifest as the manifest only if this.manifest is not null.
It seems the code should be something like:
var orientation1 = (this.manifest) ? this.manifest.orientation : null;
var orientation2 = (this.config.manifest) ? this.config.manifest.orientation : null;
var orientation3 = (this.activityCaller.manifest) ? this.activityCaller.manifest.orientation : null;
var orientation = orientation1 || orientation2 || orientation3;
With that said, I'm still coming up to speed with ffos dev so I lack context here. I assume the activity caller's manifest is the manifest.webapp of the caller. What is the activity's manifest? Is that what is defined in the MozActivity? What is config.manifest?
Also, can you give me some context on how this code from your commit -- https://github.com/alivedise/gaia/blob/a98b09c55fb819cc5689b898a9bd92c04844f8d2/apps/system/js/activity_window.js#L133-L140 -- helped fix bug #948226?
Thanks.

Russ: unlockOrientation() is the call that allows the screen to rotate when the user rotates the phone. If an app specifies portrait or landscape then it is locked in that orientation. Otherwise it is "unlocked".
Alive: if you'd prefer to fix this yourself, feel free to take it from Russ. If you're busy with the rocketbar workweek, and think that Russ is on the right track here, your guidance is appreciated on this bug.

Created attachment 8370534[details]
Suggested change
Hi Alive, I've attached a diff with my suggested change. I believe it's functionally equivalent to the existing code except for the difference in logic for getting the orientation.

Comment on attachment 8370534[details]
Suggested change
Thanks for your patch.
I'd appreciate if you have a unit test for this change (see activity_window_test.js)
and file a github pull request then ask review.
Thanks again!

My 2cent:
If an activity page needs a specific orientation lock than the one in its manifest,
I wonder we could:
(1) add orientation in the activity object in manifest.
(2) let the activity to call the orientation when it's loaded.
But for now the patch is fine to go IMO.

Alive, I added unit tests for the new logic. Although there are now only three test cases for the setOrientation function even though there are four decisions in the code to determine the orientation. This is because, from my reading of the code, it's not possible for this.manifest to be null while this.config.manifest is not null (and vice-versa) -- because the ActivityWindow constructor copies all the config attributes to the ActivityWindow object. Am I missing something? Is it necessary or useful to test this.manifest and this.config.manifest?

(In reply to Russ Nicoletti [:russn] from comment #18)
> Alive, I added unit tests for the new logic. Although there are now only
> three test cases for the setOrientation function even though there are four
> decisions in the code to determine the orientation. This is because, from my
> reading of the code, it's not possible for this.manifest to be null while
> this.config.manifest is not null (and vice-versa) -- because the
> ActivityWindow constructor copies all the config attributes to the
> ActivityWindow object. Am I missing something? Is it necessary or useful to
> test this.manifest and this.config.manifest?
Nice!
You are right that currently only app with manifest could have be an activity.
It's up to you to remove this.manifest checkage or not.

Hey Russ - Just letting you know that all merges needs to have their commits squashed before merging. I noticed that ~4 patches landed as part of this bug, which will make any reverting or cherry-picking more difficult. You can do this with git rebase against master. Just letting you know for the future. NI? on you so you are aware. Thanks!