Update SMS to be ready for L10n API v3

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

({qawanted})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

3 years ago
Assignee: nobody → gandalf
(Assignee)

Comment 1

3 years ago
Created attachment 8614949 [details] [review]
pull request

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)
(Assignee)

Updated

3 years ago
Blocks: 1170963
(Assignee)

Updated

3 years ago
Depends on: 1171856
Comment on attachment 8614949 [details] [review]
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

3 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

3 years ago
Comment on attachment 8614949 [details] [review]
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

3 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 on attachment 8614949 [details] [review]
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

2 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)
Yes !

You only need to mirror some changes to views/conversation/index.html :)
Flags: needinfo?(felash)
(Assignee)

Updated

2 years ago
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
Keywords: qawanted
(Assignee)

Comment 9

2 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

2 years ago
Commit: https://github.com/mozilla-b2g/gaia/commit/0df8d88320492897f0b4eb9e522099e103f09274
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

2 years ago
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 :)
(Assignee)

Comment 13

2 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
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

2 years ago
Filed bug 1187627.
Created attachment 8638951 [details] [review]
[gaia] zbraniecki:1187627-sms-l10n-api-part2 > mozilla-b2g:master
(Assignee)

Comment 16

2 years ago
Comment on attachment 8638951 [details] [review]
[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 ;)
(Assignee)

Comment 18

2 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.
Ah yeah I agree, I was looking at the drop on the 25th. Thanks !
(Assignee)

Updated

2 years ago
Blocks: 1027117
Depends on: 1187878
You need to log in before you can comment on or make changes to this bug.