Closed Bug 1087981 Opened 7 years ago Closed 6 years ago

[Message] Lazy init settings in notify.js


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

Not set


(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

2.2 S5 (6feb)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: julienw, Assigned: julienw)



(Whiteboard: [p=1])


(1 file)

46 bytes, text/x-github-pull-request
: review+
: feedback+
Details | Review
No description provided.
Attached file github PR
I haven't tested the patch on a device yet.

I removed the observer because in this specific case we don't need to be really quick.
Assignee: nobody → felash
Comment on attachment 8510264 [details] [review]
github PR

Works fine for me :)

Steve, what do you think?
Attachment #8510264 - Flags: review?(schung)
Comment on attachment 8510264 [details] [review]
github PR

I'm fine if you prefer less modification in order to uplift it. I think we just need additional assertion for vibrate when document is not hidden. Just ping me when the test is ready, thanks!
Attachment #8510264 - Flags: review?(schung)
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
I'm marking feature-b2g:2.2+, as it's blocking performance meta bug.
If it turns out the approach doesn't improve, then just close the bug or remove flag.
feature-b2g: 2.2? → 2.2+
Whiteboard: [p=1]
Target Milestone: --- → 2.2 S4 (23jan)
[changing flag for the performance improvement task]
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
Comment on attachment 8510264 [details] [review]
github PR

Hey Steve, I think it's a ready for a second review :) hopefully the final one ;)
Attachment #8510264 - Flags: review?(schung)
Comment on attachment 8510264 [details] [review]
github PR

Only some small suggestions on github, and looks good overall, thanks!
Attachment #8510264 - Flags: review?(schung) → review+
Comment on attachment 8510264 [details] [review]
github PR

Hey Steve, can you have a last look at the last commit? I replaced the settings name by variables. I'm not so sure it's better though, so I'd like your advice.
Attachment #8510264 - Flags: feedback?(schung)
Comment on attachment 8510264 [details] [review]
github PR

Yes, that's what I thought, thanks!
Attachment #8510264 - Flags: feedback?(schung) → feedback+
master: 0368004d46ae09c3e3694d087da3c7c37d27139c
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8510264 [details] [review]
github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: slightly less performance at startup
[Testing completed]: yes / manual and unit tests
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Note that this is not in the critical path for launching the SMS app, so maybe we should not uplift this. That said, it's a fairly safe patch.
Attachment #8510264 - Flags: approval-gaia-v2.2?
Target Milestone: 2.2 S4 (23jan) → 2.2 S5 (6feb)
Attachment #8510264 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.