1050023 - When dolphin(v1.4) downloading, power key can't turn off the screen

Status: VERIFIED FIXED

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

Description

[Shufang.Xu]

Attachment: bug337361_when_download_press_the_power_key_can_not_destroy_the_screen_bright_screen_automatically.patch

repro Test:
1)Download using the default browser, wifi and PS download
2)Press the power button to turn off the screen, but can't destroy the screen, the screen will automatically bright again.

Comment 1

[Shufang.Xu]

Dear Fernando Jiménez Moreno && Michael Henretty:

please help check it,Thank you very much !Still have to trouble you again, I'm so sorry

Comment 2

[Shufang.Xu]

Dear Evelyn Hung:
please help to check it,I have to trouble you again,Thank you very much again!

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] ensure screen wake for cell broadcast

I created a PR, so that we can get the test suite to run. The put the commit in your name, so I don't feel strange about assigning myself to review my own pull request :)

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

[Blocking Requested - why for this release]:

This is coming from spreadtrum for 1.4, so I'm nomming on behalf of Shufang Xu.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469038 [details] [review]
[Gaia PR] ensure screen wake for cell broadcast

Shufang Xu, I might be missing something. It looks like your code makes sure we will wake the screen for cell-broadcast notifications. But was it not already doing that before? Also, how does this change fix the fact that you cannot turn off the turn off the screen when downloading?

Comment 6

[Shufang.Xu]

(In reply to Michael Henretty [:mhenretty] from comment #5)
> Comment on attachment 8469038 [details] [review]
> [Gaia PR] ensure screen wake for cell broadcast
> 
> Shufang Xu, I might be missing something. It looks like your code makes sure
> we will wake the screen for cell-broadcast notifications. But was it not
> already doing that before? Also, how does this change fix the fact that you
> cannot turn off the turn off the screen when downloading?

when downloading, the 'detail.manifestURL' return null,so don't turn on the screen

Comment 7

[Wayne Chang [:wchang]]

William,

Can you check the bug does repro with the STR in comment 0?

Comment 8

[William Hsu [:whsu]]

(In reply to Wayne Chang [:wchang] from comment #7)
> William,
> 
> Can you check the bug does repro with the STR in comment 0?

Hi, Wayne,

I can reproduce it with the steps on comment 0.
Here is demo video. FYI.
- http://youtu.be/CEQ1zL60PNQ

Comment 9

[Wayne Chang [:wchang]]

Thanks William, that doesn't look good :(

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469038 [details] [review]
[Gaia PR] ensure screen wake for cell broadcast

We need to take a different approach here. The logic has gotten a little confusing. We do the same thing inside an if block and and else block. Also, we are fixing a case for downloads, but the code changes pertain to a case which is already working. If someone were to look at the git log, it would not be clear what is happening. Let's do something more explicit, and add code to not wake the screen for download notifications.

Also, with this patch the download notification will indeed not wake the screen. But once the user does turn on the screen, the phone will vibrate. We need to fix this too. I'll help out here.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] do not wake screen for download notifications

Alexandre, would you have a look here? We need to make sure cell broadcast wakes the screen, while downloading notifications do not.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Francis, I believe you are the Download Manager UX guy. Can you verify that for download notifications we don't want to wake the screen? I don't believe that was in the original spec, but looking at this video http://youtu.be/CEQ1zL60PNQ it seems like something we want to change.

Comment 13

[Shufang.Xu]

(In reply to Michael Henretty [:mhenretty] from comment #10)
> Comment on attachment 8469038 [details] [review]
> [Gaia PR] ensure screen wake for cell broadcast
> 
> We need to take a different approach here. The logic has gotten a little
> confusing. We do the same thing inside an if block and and else block. Also,
> we are fixing a case for downloads, but the code changes pertain to a case
> which is already working. If someone were to look at the git log, it would
> not be clear what is happening. Let's do something more explicit, and add
> code to not wake the screen for download notifications.
> 
> Also, with this patch the download notification will indeed not wake the
> screen. But once the user does turn on the screen, the phone will vibrate.
> We need to fix this too. I'll help out here.

Thank you very much !

Comment 14

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #11)
> Created attachment 8469586 [details] [review]
> [Gaia PR] do not wake screen for download notifications
> 
> Alexandre, would you have a look here? We need to make sure cell broadcast
> wakes the screen, while downloading notifications do not.

From what I read on the PR it looks fine, but I would like to make sure it's okay not breaking anything.

Also, I don't see any test to assert the behavior, is it because of the emergency?

Comment 15

[:gerard-majax]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Reflag me if you need another round for tests. I'll test on device, but so far the code looks okay.

Comment 16

[:gerard-majax]

needinfo? myself to check on device.

Comment 17

[:gerard-majax]

Michael, your PR will need to be updated for v1.4 branch: master is different, patch do not apply.

Comment 18

[:gerard-majax]

Attachment: bug1050023-dolphin-download-notification-v1_4.patch

v1.4 patch

Comment 19

[:gerard-majax]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Works great after adapting it to v1.4 branch, thanks!

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

It seems that this is already in good hands, so I'm clearing the ni.

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks Alexandre for doing a 1.4 specific fix. I'll look into adding a test today before landing.

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Added unit tests to make sure email and download notifications do not call ScreenManager.turnScreenOn. Now we will just wait for the tests to go green.

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Rebased, since bug 1051788 landed last night and it conflicted. Also fixed some failing tests.

Comment 24

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

A new test case needs to be written to cover this issue

Comment 25

[Francis Djabri [:djabber]]

(In reply to Michael Henretty [:mhenretty] from comment #12)
> Francis, I believe you are the Download Manager UX guy. Can you verify that
> for download notifications we don't want to wake the screen? I don't believe
> that was in the original spec, but looking at this video
> http://youtu.be/CEQ1zL60PNQ it seems like something we want to change.

The "Downloading..." notification seems to be constantly waking up the screen, which I agree is weird and annoying. I can confirm that we should prevent this notification from waking up the display, but would prefer it if the "Download complete" and "Download failed" notifications still wake up the display.

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Based on Francis' feedback in comment 25, we need to modify this patch slightly. It is enough of a change that it warrants another review.

Comment 27

[:gerard-majax]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

That looks good, thanks. Please make sure you address the comments on Github before landing :)

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/3718306f22485fc058eefed584e3cb238dcd1819
v2.0: https://github.com/mozilla-b2g/gaia/commit/2b42351cbc8f652b1cbafb76768386546d752204
v1.4: https://github.com/mozilla-b2g/gaia/commit/518c6afa28dc3b87e3a7360ee4b751885884d03d

Comment 29

[Ryan VanderMeulen [:RyanVM]]

So.....as of about a month ago, 1.4+ blocking status doesn't grant auto-approval to land on v2.0. The change in policy was announced on dev-gaia and the B2G Landing Page was updated to reflect it. This needs to be reverted from v2.0 or retroactive approval needs to be granted.

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Hmmm, I read this wrong: https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.0.0_2

I thought this meant you need either one of these, not both:
    blocking-b2g:1.3+/1.4+/2.0+ (without NO_UPLIFT, POVB, or NPOTB)
    approval-gaia-v2.0+

reverted v2.0: https://github.com/mozilla-b2g/gaia/commit/3bf4da87bea06cb67f9a549645ea465b5dcd3077

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4
> * 1.4+ blocking bugs need an explicit approval to land on 2.0
> * Please use approval-gaia-v2.0? for gaia and approval-mozilla-b2g32? for gecko to request uplift as necessary

That said, please do request v2.0 approval on this if it does need to land there.

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

[Approval Request Comment]
This bug is bad UX for a user, is in 2.1 and 1.4, so right now it is a regression only affecting 2.0

[Bug caused by] (feature/regressing bug #):
Bug 1033889

[User impact] if declined:
User will not be able to turn off the screen when downloading.

[Testing completed]:
Adding unit tests, and have tested on device.

[Risk to taking this patch] (and alternatives if risky):
This patch affects screen wake (and vibration) behavior for notifications on the phone. However, this is pretty thoroughly tested. The only alternative is not to accept the patch.


[String changes made]:
None.

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8469586 [details] [review]
[Gaia PR] do not wake screen for download notifications

Aprroving this given its a 2.0 regression and flame would be impacted as well. Also adding verifyme for QA to help verify once this lands on 2.0.

Comment 34

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/1c22419716b9c8fdb4b3aa14dc4cbc7a23ba4a85

Comment 35

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

A test case needs to be written to cover this issue

Comment 36

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case added to moztrap:

https://moztrap.mozilla.org/manage/case/14353/

Comment 37

[William Hsu [:whsu]]

Hi, everyone,

A problem still needs your help.
After download finished, FxOS still wakes screen.
Demo video: http://youtu.be/X3qfJ9qqGq8

A bug is filed. 
Bug 1056021 - [Dolphin][Flame] FxOS wakes the screen after download finished.

Comment 38

[William Hsu [:whsu]]

(In reply to William Hsu [:whsu] from comment #37)
> Hi, everyone,
> 
> A problem still needs your help.
> After download finished, FxOS still wakes screen.
> Demo video: http://youtu.be/X3qfJ9qqGq8
> 
> A bug is filed. 
> Bug 1056021 - [Dolphin][Flame] FxOS wakes the screen after download finished.

Thanks for Alexandre's help!
I missed the comment 25.

Please skip the comment 37.
Sorry for any inconvenience.

Comment 39

[jbolton]

This issue is verified as fixed on 1.4, 2.0, and 2.1.

Actual Result: When a download is in progress, pressing the power button executes as expected. The screen is locked and the download does not continue to light up the screen. 

Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141126001202
Gaia: db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko: 211eae88f119
Version: 34.0 (2.1) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Device: Flame 2.0 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141126000203
Gaia: f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko: 45112935086f
Version: 32.0 (2.0)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 1.4 (319mb)(Jellybean Base)(Shallow Flash
BuildID: 20141126000201
Gaia: 22c80a708329321a2fdeed4ece019498c0cec90d
Gecko: 429d90dd383c 
Version: 30.0 (1.4)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0