906627 - [Camera][Helix] The flash light is still open after user pressed the home button

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[William Hsu [:whsu]]

* Description:
  This case happened on Helix device. Leo device don't have this problem.
  If you open the flash light under recording mode and pressing the home button, the flash light is still open.


* Reproduction steps:
  1. Launch the camera app
  2. Switch to recording mode
  3. Turn on flash light
  4. Pressing home button

* Expected result:
  The flash light is turned off.

* Actual result:
  The flash light is still open

* Reproduction build:(Mozilla-b2g18_v1_1_0_hd-helix/2013-08-15-04-22-01)
  + Mercurial-Information
    - Gecko revision="bd0864949c0f"
  + Git-information
    - Gaia revision="d04d2d3969e4366fabafe9659f9b2705b7879ac2"

Thanks!

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

I fixed this issue by exposing two more methods which allow users directly turn the flash on/off. Because in the callback function at the bottom of the file shouldn't access the Camera's inner states, and original public methods can only toggle it, not clearly turn on/off it.

Comment 2

[Dale Harvey (:daleharvey)]

Comment on attachment 792712 [details]
Patch

Unfortunately I dont have a device which supports torch mode in which to test this, however the code looks good

I have left a few minor codestyle nits on github which would be nice to clean up before you push (if you cant push then just needinfo me with the nits addressed)

Thanks

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> Comment on attachment 792712 [details]
> Patch
> 
> Unfortunately I dont have a device which supports torch mode in which to
> test this, however the code looks good
> 
> I have left a few minor codestyle nits on github which would be nice to
> clean up before you push (if you cant push then just needinfo me with the
> nits addressed)
> 
> Thanks

Thanks for reviewing and I had finished the modification. Unfortunately I still lack the permission to merge it as you said. So please merge it for me.

https://github.com/mozilla-b2g/gaia/pull/11628

BTW: The Travis-CI seems in trouble today. I tried several times and other PRs are all failed. However I want to reply to make a note and waiting it turns to normal.

Comment 4

[William Hsu [:whsu]]

Thanks for the prompt help!
Please update the status if the patch merges into V1.1.0 hd.
Thank you!

Comment 5

[lecky]

The patch can turn off the flash after user pressed the home button or the power key.
But I have a question, as following: We don't record the state of the flash before we turn off it. Press the home or the power key isn't close the app, just let it on the background. If the user press the home key, and then open the camera app again, this time the flash is always off(On android, this time the flash's state is the same as before pressed the home or power key). 

It means that on android, when the flash is turn on, it will be turn off after pressed the home or power key, it will be turn on after open the camera app again.
But in our FFOS, add the patch, when the flash is turn on, it will be turn off after pressed the home or power key, but it still be turn off after open the camera app again.

Can we modify somewhere to let the phenomenon metioned aboved the same on both two platform?

Thanks.

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to lecky from comment #5)
> The patch can turn off the flash after user pressed the home button or the
> power key.
> But I have a question, as following: We don't record the state of the flash
> before we turn off it. Press the home or the power key isn't close the app,
> just let it on the background. If the user press the home key, and then open
> the camera app again, this time the flash is always off(On android, this
> time the flash's state is the same as before pressed the home or power key). 
> 
> It means that on android, when the flash is turn on, it will be turn off
> after pressed the home or power key, it will be turn on after open the
> camera app again.
> But in our FFOS, add the patch, when the flash is turn on, it will be turn
> off after pressed the home or power key, but it still be turn off after open
> the camera app again.
> 
> Can we modify somewhere to let the phenomenon metioned aboved the same on
> both two platform?
> 
> Thanks.

After saw your comment we did some tests to find out what is the most common behavior, and the result is that there're 3 different phones using 2 stategies:

1. Sony Xperia P (android): do NOT keep the status of the light. User must turn on it when he/she come back to the camera app

2. iPhone: do NOT keep the status of the light as well

3. Other Android phones: DO keep the status of the light, and turn on it automatically while user come back.

So this may be a UX issue and need to needinfo Neo.

Comment 7

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

change needinfo to ux team.

Comment 8

[Dale Harvey (:daleharvey)]

Yeh I had the same thoughts about persisting vs not, I ill land this one and the persistent state can be done in a follow up if thats what we want

Comment 9

[William Hsu [:whsu]]

Hi, Lecky,

Thanks for pointing out the problem.


Hi, Greg,

Could I have your help?
As I tested the behavior on the Leo device, the behavior of flash light is same as Lecky mentioned.
- The flash light is turned off after pressed the home button
- The flash light is turned on after returned to the camera app.
So, I think we should align the behavior on all FxOS devices.
Any thought?
Thank you!



### Clear the needinfo([e-mail]) because we had an existing behavior. ###

Comment 10

[Dale Harvey (:daleharvey)]

Agreed, I will needinfo Greg to see if he can follow up, it shouldnt be a big change.

Greg, as we have existing behaviour on the Leo, can we change this patch to follow it, and sorry for the delay, next time if you need me to check in a patch could you needinfo me so it shows up, thanks

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Dale Harvey (:daleharvey) from comment #10)
> Agreed, I will needinfo Greg to see if he can follow up, it shouldnt be a
> big change.
> 
> Greg, as we have existing behaviour on the Leo, can we change this patch to
> follow it, and sorry for the delay, next time if you need me to check in a
> patch could you needinfo me so it shows up, thanks

Ok, I will try to modify the patch and you may review it again.

Comment 12

[William Hsu [:whsu]]

Hi, Dale and Greg,

Sorry to disturb you again.
As I discussed the bug with Gary, Gary mentioned that we should file other bugs to trace this problem because it is a different problem.
Per our discussion, we also found nobody defined the flash light behavior before.
So, it may be a big problem and need much effort to solve it.
I would like to discuss remaining work on new filed bugs and mark this bug(906627) as "Resolved"
Any suggestion?
Thank you.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to William Hsu [:whsu] from comment #12)
> Hi, Dale and Greg,
> 
> Sorry to disturb you again.
> As I discussed the bug with Gary, Gary mentioned that we should file other
> bugs to trace this problem because it is a different problem.
> Per our discussion, we also found nobody defined the flash light behavior
> before.
> So, it may be a big problem and need much effort to solve it.
> I would like to discuss remaining work on new filed bugs and mark this
> bug(906627) as "Resolved"
> Any suggestion?
> Thank you.

Well, at least my patch had already solved the issues listed in this bug. So it would be nicer to left the undefined, or not discussed behavior in another individual bug. Then I, or someone also interesting in it can continue the work.

BTW: I still lack the permission to merge the patch and the Travis still report irrelated timeout problem and make my patch faild to build! So maybe [:dale] can merge it for me and we can migrate to the new bug and left this one closed.

Comment 14

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Dale and Lecky,
   As the comment#12, we found many behaviors about the flash light are not consistent.
  In leo devices, 
   for examples:
    scenario 1
    1. open then camera switch to video mode and  turn on the flash 
    2. press home button
    3. open the gallery app and click camera
    4. flash light is off
  
    scenario 2
    1. open then camera switch to video mode and  turn on the flash 
    2. press home button
    3. open camera again, flash light is on.

   
  We suggest to open new bug to track this issue. 
  Thank you.

Comment 15

[Dale Harvey (:daleharvey)]

Agreed, I dont have a working leo device to test these behaviours, however this fixes a fairly large bug that we definitely want fixed, defining the exact behaviour and ensuring its consistent across devices can be done in a follow up

Comment 16

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/4292d74f14438b49ebd01a4dbf89fd2748926f16

Comment 17

[William Hsu [:whsu]]

Attaching a related bug.

Bug 910631 - [Flashlight] Need to define the behavior of flashlight

Comment 18

[lecky]

Hi Wayne :
 Please help us to push to merge this case into V1.1 HD. It blocked the Helix Device .If you can not resolve it , please get the authorization from carrier.

Thanks.

Comment 19

[Wayne Chang [:wchang]]

This should be landed on v1.1HD as flagged.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

John, maybe you can assist?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Tim, can you please assist with the uplift? :)

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/commit/ec9cd27f0d18f808e7226ca7ea74c4b64abe1084

According to MDN we dropped the prefix on gecko18 so I manually resolved the patch conflict this way.
https://developer.mozilla.org/en-US/docs/Web/Guide/User_experience/Using_the_Page_Visibility_API

Comment 23

[Al Tsai [:atsai]]

[2013/10/21 Helix Testing]
Gaia:     c829a2042594b6c3a4899ee27979799a0f301534
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f7c657f6d019
BuildID   20131015042201
Version   18.0