Closed
Bug 1187668
Opened 9 years ago
Closed 9 years ago
Update Settings to be ready for L10n API v3
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(3 files)
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•9 years ago
|
||
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 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 | ||
Comment 3•9 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•9 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)
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 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•9 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•9 years ago
|
Attachment #8642124 -
Flags: review?(gasolin)
Comment 8•9 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•9 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•9 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+
Assignee | ||
Comment 11•9 years ago
|
||
Downloads piece committed: https://github.com/mozilla-b2g/gaia/commit/f33da62d93ef341a5dbd7e60bfe160495df3dc1d
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)
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 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•9 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•9 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•9 years ago
|
||
Comment on attachment 8638983 [details] [review]
pull request
Great! Thanks!
Attachment #8638983 -
Flags: review?(ehung) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Commit: https://github.com/mozilla-b2g/gaia/commit/a39c7a54649ccf52ce08cdc52f9b17040b28ad50
Thanks a lot Evelyn and Fred!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•