Closed Bug 1033889 Opened 6 years ago Closed 6 years ago

[dolphin] unable to read cell broadcast message on lock screen

Categories

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

x86_64
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:1.4+, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: Shufang.Xu, Assigned: Shufang.Xu, NeedInfo)

References

()

Details

(Whiteboard: [partner-blocker][sprd326683])

Attachments

(2 files)

Testing Steps :
1.  Registered in 8960, 2 G area 
2.  When the phone starts, the phone lock screen 
3.  The instrument send cell broadcast to the phone
4.  When the mobile phone in the lock screen state receive cell broadcast, unable to click on read and delete all.
Flags: needinfo?(ehung)
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(pcheng)
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(pcheng)
Summary: When the mobile phone in the lock screen state receive cell broadcast, unable to click on read and delete all. Because of do not turn screen on for Email notifications ,This change has affected the cell broadcast.This is found from Dolphin test. → [dolphin] unable to read cell broadcast message on lock screen
Whiteboard: [partner-blocker][sprd326683]
This issue was introduced by the fix for bug 915236: disable turning on the screen for email notifications
Comment on attachment 8449921 [details] [diff] [review]
Bug326683_cell_broadcast_can't_read_and_clearAll_in_lockscreen.patch

Thanks Shufang.Xu!

as I mentioned in the other bug, the fix LGTM, but I'd like to get confirmation from Michael that the check for the email app is still needed or we have a better alternative to avoid turning the screen on for some app notifications that doesn't involve hard-coding the app origin in this code.

BTW, you need to send a pull request to Gaia to get this patch merged. Check https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch please.

Thanks!
Attachment #8449921 - Flags: review+
Attachment #8449921 - Flags: feedback?(mhenretty)
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(ehung)
Per comment 2, please send a pull request for merging your patch. Thanks.
Flags: needinfo?(Shufang.Xu)
blocking-b2g: --- → 1.4?
https://github.com/mozilla-b2g/gaia/pull/21424

Now I have already send a pull request ,please find persion to merge it,thank you very much!
Flags: needinfo?(Shufang.Xu)
Comment on attachment 8449921 [details] [diff] [review]
Bug326683_cell_broadcast_can't_read_and_clearAll_in_lockscreen.patch

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #2)
> Comment on attachment 8449921 [details] [diff] [review]
> Bug326683_cell_broadcast_can't_read_and_clearAll_in_lockscreen.patch
> as I mentioned in the other bug, the fix LGTM, but I'd like to get
> confirmation from Michael that the check for the email app is still needed
> or we have a better alternative to avoid turning the screen on for some app
> notifications that doesn't involve hard-coding the app origin in this code.

Unfortunately, we still have no way from an API standpoint to control things like turning on the screen.


Thanks for the patch Shufang.Xu! Please fix the linting error (the line you changed is over 80 characters, which causes our build to fail) and we should be good to land.
Attachment #8449921 - Flags: feedback?(mhenretty) → feedback+
Assignee: nobody → Shufang.Xu
(In reply to Michael Henretty [:mhenretty] from comment #5)
> Comment on attachment 8449921 [details] [diff] [review]
> Bug326683_cell_broadcast_can't_read_and_clearAll_in_lockscreen.patch
> 
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #2)
> > Comment on attachment 8449921 [details] [diff] [review]
> > Bug326683_cell_broadcast_can't_read_and_clearAll_in_lockscreen.patch
> > as I mentioned in the other bug, the fix LGTM, but I'd like to get
> > confirmation from Michael that the check for the email app is still needed
> > or we have a better alternative to avoid turning the screen on for some app
> > notifications that doesn't involve hard-coding the app origin in this code.
> 
> Unfortunately, we still have no way from an API standpoint to control things
> like turning on the screen.
> 
> 
> Thanks for the patch Shufang.Xu! Please fix the linting error (the line you
> changed is over 80 characters, which causes our build to fail) and we should
> be good to land.


  I don't understand what you mean, that's my patch failed to merge, I don't have to send a pull request? I want to confirm it. What I need to do next?
(In reply to Shufang.Xu from comment #6)
>   I don't understand what you mean, that's my patch failed to merge, I don't
> have to send a pull request? I want to confirm it. What I need to do next?

The line you changed is now too many characters long [1]. We have automated tests that check code style, and one of the rules is that no javascript lines can be over 80 characters. Your changes make that line 93 characters long. Not to worry, it's an easy fix, just break that `if` statement into two lines. Here is an example to show you what I mean:

https://github.com/mozilla-b2g/gaia/blob/8b4af492e7835c190d61b8459be7accfbfcc44c2/apps/system/js/notifications.js#L240


Also (someone correct me if I'm wrong) but I think we want this code in master too. Shufang, please submit your changes against the master branch instead of 1.4, and then I will help uplift the code into the 1.4 branch.


1.) https://travis-ci.org/mozilla-b2g/gaia/jobs/29285111#L516
Now I have already send a pull request to the master according to you said ,please find people to merge it,thank you very much!
(In reply to Shufang.Xu from comment #8)
> Now I have already send a pull request to the master according to you said
> ,please find people to merge it,thank you very much!

(In reply to Michael Henretty [:mhenretty] from comment #7)
> (In reply to Shufang.Xu from comment #6)
> >   I don't understand what you mean, that's my patch failed to merge, I don't
> > have to send a pull request? I want to confirm it. What I need to do next?
> 
> The line you changed is now too many characters long [1]. We have automated
> tests that check code style, and one of the rules is that no javascript
> lines can be over 80 characters. Your changes make that line 93 characters
> long. Not to worry, it's an easy fix, just break that `if` statement into
> two lines. Here is an example to show you what I mean:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 8b4af492e7835c190d61b8459be7accfbfcc44c2/apps/system/js/notifications.js#L240
> 
> 
> Also (someone correct me if I'm wrong) but I think we want this code in
> master too. Shufang, please submit your changes against the master branch
> instead of 1.4, and then I will help uplift the code into the 1.4 branch.
> 
> 
> 1.) https://travis-ci.org/mozilla-b2g/gaia/jobs/29285111#L516

(In reply to Michael Henretty [:mhenretty] from comment #7)
> (In reply to Shufang.Xu from comment #6)
> >   I don't understand what you mean, that's my patch failed to merge, I don't
> > have to send a pull request? I want to confirm it. What I need to do next?
> 
> The line you changed is now too many characters long [1]. We have automated
> tests that check code style, and one of the rules is that no javascript
> lines can be over 80 characters. Your changes make that line 93 characters
> long. Not to worry, it's an easy fix, just break that `if` statement into
> two lines. Here is an example to show you what I mean:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 8b4af492e7835c190d61b8459be7accfbfcc44c2/apps/system/js/notifications.js#L240
> 
> 
> Also (someone correct me if I'm wrong) but I think we want this code in
> master too. Shufang, please submit your changes against the master branch
> instead of 1.4, and then I will help uplift the code into the 1.4 branch.
> 
> 
> 1.) https://travis-ci.org/mozilla-b2g/gaia/jobs/29285111#L516

(In reply to Michael Henretty [:mhenretty] from comment #7)
> (In reply to Shufang.Xu from comment #6)
> >   I don't understand what you mean, that's my patch failed to merge, I don't
> > have to send a pull request? I want to confirm it. What I need to do next?
> 
> The line you changed is now too many characters long [1]. We have automated
> tests that check code style, and one of the rules is that no javascript
> lines can be over 80 characters. Your changes make that line 93 characters
> long. Not to worry, it's an easy fix, just break that `if` statement into
> two lines. Here is an example to show you what I mean:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 8b4af492e7835c190d61b8459be7accfbfcc44c2/apps/system/js/notifications.js#L240
> 
> 
> Also (someone correct me if I'm wrong) but I think we want this code in
> master too. Shufang, please submit your changes against the master branch
> instead of 1.4, and then I will help uplift the code into the 1.4 branch.
> 
> 
> 1.) https://travis-ci.org/mozilla-b2g/gaia/jobs/29285111#L516


  Now I have already send a pull request to the master according to you said ,please find people to merge it,thank you very much!

https://github.com/mozilla-b2g/gaia/pull/21474
blocking-b2g: 1.4? → 1.4+
(In reply to Michael Henretty [:mhenretty] from comment #7)
> (In reply to Shufang.Xu from comment #6)
> >   I don't understand what you mean, that's my patch failed to merge, I don't
> > have to send a pull request? I want to confirm it. What I need to do next?
> 
> The line you changed is now too many characters long [1]. We have automated
> tests that check code style, and one of the rules is that no javascript
> lines can be over 80 characters. Your changes make that line 93 characters
> long. Not to worry, it's an easy fix, just break that `if` statement into
> two lines. Here is an example to show you what I mean:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 8b4af492e7835c190d61b8459be7accfbfcc44c2/apps/system/js/notifications.js#L240
> 
> 
> Also (someone correct me if I'm wrong) but I think we want this code in
> master too. Shufang, please submit your changes against the master branch
> instead of 1.4, and then I will help uplift the code into the 1.4 branch.
> 
> 
> 1.) https://travis-ci.org/mozilla-b2g/gaia/jobs/29285111#L516



I don't know why failed again,I am so sorry!

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

   Now I have already send a pull request to the master according to you said ,please find people to merge it,thank you very much!
Ah, now th(In reply to Shufang.Xu from comment #11)
> 
>   https://github.com/mozilla-b2g/gaia/pull/21488
> 
>    Now I have already send a pull request to the master according to you
> said ,please find people to merge it,thank you very much!

Ah, almost there! Now there is extra space at the end of the line, after the '||' or symbol. That is what is causing the failure. Btw, we use jshint to lint our javascript, so you can run that locally by executing 'make hint' from the gaia repository. Or, you could copy and past the notification.js file into here: http://jshint.com/

Btw, you can find me on IRC (mhenretty) in #gaia if you have any questions.
Oh, nm, you did fix that in your latest PR :) The other errors are vertical homecreen integration tests that are unrelated to your patch (and actively being fixed by :kgrandon). I'm going to merge and uplift.
master: https://github.com/mozilla-b2g/gaia/commit/d9c990ca52be018e5a16e71fbb936cf094e6ee28
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
(In reply to Michael Henretty [:mhenretty] from comment #15)
> v1.4:
> https://github.com/mozilla-b2g/gaia/commit/
> c73c46a7c9d418c14c74cbf0f88d93d98b169454
> v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> 1dd043857399c713e3b509c0ed31bdf20326f08b
> 
> Thanks again Shufang.Xu!!



I am also very thank you for your patience guidance, sorry to trouble you, that I did not notice the problem at ordinary times, thank you for reminding me, really appreciate it.You are a particularly serious and responsible person, I like it.
For the cell broadcast revision, the last time I modify has a problem, when download, holding down the power button cannot destroy the screen, the screen will automatically bright, I modified again, in the local verification is passed, please check it.For this matter,I am so sorry to that. Thank you very much again!
Flags: needinfo?(mhenretty)
Hi Shufang.Xu,

It sounds like there is another bug to fix. Can you file a follow-up bug to this, with a description of the problem. Then attach your code as a github pull request, so we can verify that the tests run properly.

Also, did you need to get this fix into v1.4 as well?
Flags: needinfo?(Shufang.Xu)
Flags: needinfo?(mhenretty)
Flags: in-moztrap?(ychung)
New test case needs to be added. There is no existing test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14319/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
Depends on: 1056021
You need to log in before you can comment on or make changes to this bug.