1171206 - Update SMS to be ready for L10n API v3

Status: RESOLVED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :)

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: 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.

Comment 2

[Julien Wajsberg [:julienw]]

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 :)

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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!

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[Julien Wajsberg [:julienw]]

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 !

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :)

Comment 8

[Julien Wajsberg [:julienw]]

Yes !

You only need to mirror some changes to views/conversation/index.html :)

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/0df8d88320492897f0b4eb9e522099e103f09274

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

agh. Not closing yet. There will be a small follow up to get Attachment.name fixed :)

Comment 12

[Julien Wajsberg [:julienw]]

You can file a follow-up for this, could make things easier to follow :)

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Filed bug 1187627.

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1187627-sms-l10n-api-part2 > mozilla-b2g:master

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8638951 [details] [review]
[gaia] zbraniecki:1187627-sms-l10n-api-part2 > mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/31105

Comment 17

[Julien Wajsberg [:julienw]]

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

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

[Julien Wajsberg [:julienw]]

Ah yeah I agree, I was looking at the drop on the 25th. Thanks !