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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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: nobody → gandalf
Attached file pull request (deleted) —
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)
Blocks: 1170963
Depends on: 1171856
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+
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.
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)
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 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+
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)
Yes ! You only need to mirror some changes to views/conversation/index.html :)
Flags: needinfo?(felash)
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
Keywords: qawanted
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
agh. Not closing yet. There will be a small follow up to get Attachment.name fixed :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You can file a follow-up for this, could make things easier to follow :)
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 ago9 years ago
Resolution: --- → FIXED
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
(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 ;)
(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.
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.

Attachment

General

Created:
Updated:
Size: