Closed
Bug 1171206
Opened 10 years ago
Closed 9 years ago
Update SMS to be ready for L10n API v3
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
(Keywords: qawanted)
Attachments
(1 file, 1 obsolete file)
Since SMS is one of the first apps to utilize the new architecture, I'd like to be able to use it to work on features/performance and memory of L10n API v3.
For that I need to change a few things:
- remove mozL10n.gets
- remove mozL10n.ready/once
- remove l10n_date stuff
I have the first version of the patch ready. It's actually pretty nice as it mostly removes stuff :)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gandalf
Assignee | ||
Comment 1•10 years ago
|
||
Julien, can you take a look at this?
I feel like it cleans up the code nicely, I was able to remove all gets/ready/onces, and l10n_date uses.
I encounter two problems in information_test.js and had to skip them. I don't know where reports.readTimestamp is supposed to be created in those tests and without it, generating 'read' report is impossible.
I'd appreciate your help with this piece.
Attachment #8614949 -
Flags: feedback?(felash)
Comment 2•9 years ago
|
||
Comment on attachment 8614949 [details]
pull request
I really like these changes.
I left several comments, and of course we need to not regress :)
Many thanks for this work, I realize it took a lot of your time. Sorry for the delay in reviewing this as I really wanted to have enough time in front of me :)
Attachment #8614949 -
Flags: feedback?(felash) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
Two follow up items we found so far:
1) Attachment.name, if unknown, should be localized in the view code.
2) Intl datetime strings should be updated when language/timeformat changes.
I'll deal with those in followups.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8614949 [details]
pull request
Ok, with Julien's help I got the patch to work. Let's get it reviewed and landed.
Thanks a lot Julien!
Attachment #8614949 -
Flags: review?(felash)
Assignee | ||
Comment 5•9 years ago
|
||
Also, I will not land this patch until patch in bug 1172699 lands because this will preload Intl and then I'll be able to measure perf/mem impact of this patch. Only if the patch will not regress either will I land.
Comment 6•9 years ago
|
||
Comment on attachment 8614949 [details]
pull request
r=me
let's keep an eye on the performance part.
Also please take care to do the change in the split view as well. If you land after bug 1162028 you'll need to do the change to the split conversation.html as well. Please don't forget !
Attachment #8614949 -
Flags: review?(felash) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Hi Julien!
Just a heads up that I think we're getting close to landing it. I'm planning to land the System switch to Intl tomorrow and then the SMS either tomorrow or on Monday.
I rebased the patch and added the split view l10n_date.js removal.
I think we're in a good shape to land it, and I'll work on additional tweaks (like Attachment.name) in a follow up.
Let me know if you're still ok with me landing that :)
Flags: needinfo?(felash)
Comment 8•9 years ago
|
||
Yes !
You only need to mirror some changes to views/conversation/index.html :)
Flags: needinfo?(felash)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(ktucker)
Assignee | ||
Comment 9•9 years ago
|
||
Perf/mem looks great. Performance numbers look perfectly on par while memory wise this patch drops ~50kb (tested on default locale, on Aries KK).
Landing.
Assignee | ||
Comment 10•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•9 years ago
|
||
agh. Not closing yet. There will be a small follow up to get Attachment.name fixed :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•9 years ago
|
||
You can file a follow-up for this, could make things easier to follow :)
Assignee | ||
Comment 13•9 years ago
|
||
Yeah, you're right.
Closing this one.
Also,
Raptor shows slightly bigger win than I anticipated for USS! Around 1mb (sic!) - http://raptor.mozilla.org/#/dashboard/script/apps-memory.js?device=flame-kk&branch=master&memory=319&panelId=8&fullscreen
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•9 years ago
|
||
Filed bug 1187627.
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8638951 [details]
[gaia] zbraniecki:1187627-sms-l10n-api-part2 > mozilla-b2g:master
>https://github.com/mozilla-b2g/gaia/pull/31105
Attachment #8638951 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> Raptor shows slightly bigger win than I anticipated for USS! Around 1mb
> (sic!) -
> http://raptor.mozilla.org/#/dashboard/script/apps-memory.js?device=flame-
> kk&branch=master&memory=319&panelId=8&fullscreen
Unfortunately it looks like it was just a measuring artefact ;)
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #17)
> Unfortunately it looks like it was just a measuring artefact ;)
What do you base it on?
I still see a drop from ~21.20 USS on 24th at 11:00, to ~20.10 USS on 24th at 18:50 and it stays in this zone since then.
I measured on Aries with GC-on-fullyLoaded raptor and the win was much smaller, within 100kb, but on raptor.m.o (which doesn't do GC) it seems to be a stable 1mb.
Comment 19•9 years ago
|
||
Ah yeah I agree, I was looking at the drop on the 25th. Thanks !
You need to log in
before you can comment on or make changes to this bug.
Description
•