[Message] Lazy init settings in notify.js

RESOLVED FIXED in 2.2 S5 (6feb)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
2.2 S5 (6feb)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [p=1])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
steveck
: review+
steveck
: feedback+
Details | Review
No description provided.
Posted 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+
Status: NEW → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 5 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.