1187668 - Update Settings to be ready for L10n API v3

Status: RESOLVED FIXED

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

Description

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

There are still 58 uses of mozL10n.get, some leftover LazyL10n's and l10n_date.js uses.

I want to migrate everything to be ready for l20n.

Comment 1

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

Attachment: pull request

Comment 2

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

Evelyn, just wanted to put it on your radar.

It's a major cleanup of how L10n is used in Settings. It's pretty big but it will make future work with L10n in settings much easier and it makes Settings basically ready for transition to l20n.js, so it'll pay off.

It should also give some perf/memory wins because we remove a lot of once/ready wrappers and move away from l10n_date.js which costs a lot.

I think the patch is almost ready, it has only one test failing and I want to polish it a bit more (there will be probably more things I can remove now like mozL10n.ready|once etc.).

Do you think I should split it into several patches to make it easier to review? Also, who should be my reviewer here?

Comment 3

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

Comment on attachment 8638983 [details] [review]
pull request

Evelyn, I think the patch is ready for review.

Comment 4

[Evelyn Hung]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> 
> Do you think I should split it into several patches to make it easier to
> review? Also, who should be my reviewer here?

Yep, it will help if you could split so we could have more peer involved in different parts. Thanks!

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master

Comment 6

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

Comment on attachment 8642124 [details] [review]
[gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master

Splitting patches is the hardest thing to do! :)

I separated out one major chunk that is clearly isolated - downloadformatter. It was actually 1/3 of the patch so I'm happy about this split and I think it's ready to be reviewed.

As for the rest, I'm not sure how to divide it since it's just a bunch of scattered code updates all around. Barely any logic changes. And lots of it are tests which cannot be separated out into different patch.

Would the new patch (with 52 files affected) be reviewable in your opinion or would you be able to recommend a strategy to split it?

Comment 7

[Evelyn Hung]

I guess I could start from reviewing download part as a warm-up. I will also invite Fred to review; I need another eyes on it. Thank you so much. :)

Comment 8

[Evelyn Hung]

Comment on attachment 8642124 [details] [review]
[gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master

Hi Zibi, I think the patch is great and you've modified everywhere related. I just left a comment on Github for updating the usage of |DownloadItem.create|. Well done, thank you so much! 

I will keep reviewing your another patch and it may take a couple of days.

Comment 9

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

(In reply to Evelyn Hung [:evelyn] from comment #8)
> Hi Zibi, I think the patch is great and you've modified everywhere related.

Cool, thanks!

> I just left a comment on Github for updating the usage of
> |DownloadItem.create|.

Updated the comment.

> I will keep reviewing your another patch and it may take a couple of days.

Yeah, I'm sorry for such a scratched patch. Unfortunately the nature of the changes makes it hard to avoid. The good news is that this will be the last change of that type as it makes Settings very compatible with upcoming L20n API :)

Comment 10

[[e-mail]]

Comment on attachment 8642124 [details] [review]
[gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master

LGTM, thanks!

Comment 11

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

Downloads piece committed: https://github.com/mozilla-b2g/gaia/commit/f33da62d93ef341a5dbd7e60bfe160495df3dc1d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert the downloads piece in https://github.com/mozilla-b2g/gaia/commit/91068221506ff05692aa187ac314e15443db68fd because it broke Gu tests https://treeherder.mozilla.org/logviewer.html#?job_id=2483551&repo=b2g-inbound

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master

Comment 14

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

Updated commit with the fix for the Gu test (it was just a missed update to mockL10n):
 https://github.com/mozilla-b2g/gaia/commit/0ccc7e67dedf82eabbcba5f306ea7021d2596af1

Comment 15

[Evelyn Hung]

Comment on attachment 8638983 [details] [review]
pull request

Alright, I'm finally done my review. As we've discussed on Github, the major concern is the side effect of being asynchronized on those pop-up windows. Some cases we could workaround by disabling UI, but some seem to be hard to deal with. Hopefully it shouldn't take too much time to get the string if we've loaded them, and a pop-up window usually be shown after getting user input which should happen after document loaded. So I guess it's fine, right?

Thanks for the great job; I also learned a lot by reviewing this big patch! :)

Comment 16

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

Comment on attachment 8638983 [details] [review]
pull request

Evelyn, sorry for bothering you again. I addressed your feedback comments and wanted to ask for a review just of the updated bits. I kept them in a separate commit that I'll squash after your review.

Thanks!

Comment 17

[Evelyn Hung]

Comment on attachment 8638983 [details] [review]
pull request

Great! Thanks!

Comment 18

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

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

Thanks a lot Evelyn and Fred!