Update Settings to be ready for L10n API v3

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8638983 [details] [review]
pull request
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
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?
Flags: needinfo?(ehung)
(Assignee)

Updated

3 years ago
Blocks: 1020138
(Assignee)

Comment 3

3 years ago
Comment on attachment 8638983 [details] [review]
pull request

Evelyn, I think the patch is ready for review.
Attachment #8638983 - Flags: review?(ehung)

Comment 4

3 years ago
(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!
Flags: needinfo?(ehung)
Created attachment 8642124 [details] [review]
[gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master
(Assignee)

Comment 6

3 years ago
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?
Attachment #8642124 - Flags: review?(ehung)

Comment 7

3 years ago
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. :)

Updated

3 years ago
Attachment #8642124 - Flags: review?(gasolin)
(Assignee)

Updated

3 years ago
Blocks: 1027117

Comment 8

3 years ago
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.
Attachment #8642124 - Flags: review?(ehung) → review+
(Assignee)

Comment 9

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

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

LGTM, thanks!
Attachment #8642124 - Flags: review?(gasolin) → review+
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
Flags: needinfo?(gandalf)
Created attachment 8644634 [details] [review]
[gaia] zbraniecki:1187668-settings-l10n-downloads > mozilla-b2g:master
(Assignee)

Comment 14

3 years ago
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
Flags: needinfo?(gandalf)

Comment 15

3 years ago
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! :)
Attachment #8638983 - Flags: review?(ehung) → review+
(Assignee)

Comment 16

3 years ago
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!
Attachment #8638983 - Flags: review+ → review?(ehung)

Comment 17

3 years ago
Comment on attachment 8638983 [details] [review]
pull request

Great! Thanks!
Attachment #8638983 - Flags: review?(ehung) → review+
(Assignee)

Comment 18

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

Thanks a lot Evelyn and Fred!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1194127
You need to log in before you can comment on or make changes to this bug.