Review use cases of: - mozL10n.translate - mozL10n.localize - mozL10n.get

Francisco, can you take a look at this? I cleaned up all mozL10n.translate uses (obsolete) and migrated mozL10n.localize to mozL10n.setAttributes. I also moved a lot of mozL10n.get to use data-l10n-id on Nodes. Some of those are trivial, others required some changes to the API (like utils.overlay and utils.status). I didn't touch a few complex cases, where I wasn't sure where the API is used and I also didn't touch any ConfirmDialogs/CustomDialogs. That will be covered by bug 1041404. I think it's enough for this bug to clean up what is in the PR.

Comment on attachment 8471209 [details] pull request @gandalf, I'm so sorry, it took me so much time that now it doesnt merge, could you rebase? I'll promise I will put some love on this.

Comment on attachment 8471209 [details] pull request Sure! Updated the patch. I still left plenty of mozl10n.get's (like almost 200). Most of them should be clean up, but they're entangled in API's like ConfirmDialog (bug 1041404) and TAG_OPTIONS and I didn't want to touch that here.

ping? :)

Comment on attachment 8471209 [details] pull request Hi, amazing job here, just left a couple of nits, really tiny modifications. Appart from that I found this: E/GeckoConsole( 7812): [JavaScript Error: "ReferenceError: _ is not defined" {file: "app://communications.gaiamobile.org/contacts/js/tag_options.js" line: 7}] while running, I guess we are missing the tag_options file probably relies on a global definition of '_' I think that the integration tests in js and python are failing cause of that, once fixed, sure everything comes back to green. Thanks again and sorry for the delay!

Comment on attachment 8471209 [details] pull request updated the patch, the build is green, re-requesting review

Comment on attachment 8471209 [details] pull request Jose, I've been talking with :gandalf before and we decided not to merge this one during the first sprint, since we had several things to fix and could potentially cause several conflicts. But now we could land this on master without any problem. All the apps are moving to the new l10n system and contacts must do it as well, so would you pind putting some love in this patch? Thanks!

Comment on attachment 8471209 [details] pull request this PR is huge and introduce many regression risk. My recommendation would be to separate this in smaller pieces in order to make it easy to review and to backout in case of regressions. My recommentation here is to separate this in: + changes that affect common componentes (overlay, status, etc.) + changes that affect views (list, settings, form, tags) So we would need several PRs to be reviewed and tested one by one and incrementally.

Makes sense Jose. I'll start with the more challenging part - commons. I'm mostly turning utils.status and utils.overlay to use l10n DOM attrs here. I'd like to get this landed first, because it will unblock gauv to work on bug 1058985 and then I'll work on the simpler pieces in communications itself.

Comment on attachment 8491197 [details] pull request, commons part Gandalf, thanks for separating out. We also need a review from a FTU peer.

Comment on attachment 8491197 [details] pull request, commons part FTU part looks nice, thanks gandalf.

Commit: https://github.com/zbraniecki/gaia/commit/00c4b3da2d89c8ac2212a7e1eda94bb8dbd70939 Merge: https://github.com/mozilla-b2g/gaia/commit/457efc37fadec22ae796a150a00d39c064576159

And that's the simpler part. Just views, almost exclusively switching to DOM API, and removing one LazyL10n wrapper.

Comment on attachment 8493441 [details] pull request, views part the PR is still too big: Please separate in smaller parts. - Contacts common part - settings - details - form - list thanks

Ok, I tried to split this into three parts that I found most logically divisible: - dialer - contacts commons - contacts views Let me know if that works for you

Comment on attachment 8494784 [details] pull request, dialer part please ask a dialer peer for a review thanks

Comment on attachment 8494887 [details] pull request, contacts commons part can we aim at smaller PRs? I would like to keep separated contact duplicates stuff, fb link stuff, the smaller the better please let me know if you need help in separating out things. I can help you thanks!

Comment on attachment 8495004 [details] pull request, views part :gandalf, I believe we need another round in this PR. Nonetheless, I'm happy to see that is small and manageable. Please check the comments on GH. thanks for the effort!

Comment on attachment 8495004 [details] pull request, views part Nits addressed and questions answered. Rerequesting r?

Comment on attachment 8494887 [details] pull request, contacts commons part Chunked out the smallest batch that I believe makes sense. Let me know if this one will work for you :)

Comment on attachment 8494784 [details] pull request, dialer part Thanks for doing this, and for getting index.js to pass jshint. I'm getting this error after hanging up from a call (r- only for this): W/Usage ( 5423): [JavaScript Error: "too much recursion" {file: "app://costcontrol.gaiamobile.org/shared/js/l10n.js" line: 921}] This is likely coming from something in the call log. This otherwise works well and looks good. Though I'm confused about the intended scope of this. Do you want to do this incrementally? There are still plenty of LazyL10n references and mozL10n.get()'s, though the latter are admittedly not as bad. I'm fine with not hitting everything right away, but I'm not sure to what extent you want me to mention when I notice something is missing.

It's mainly a cleanup and update to the new API. I'm not planning to remove LazyL10n unless it is not needed as we switch to declarative API. I believe I looked through all uses of mozL10n.get and the ones that I left there mostly made sense, so I'm ok with keeping them :) I'll look into the recursion error and rerequest review once I fix that.

That's an interesting case. It seems like the recursion happens only in pseudo-locales (that's the only scenario in which we use walkContent). I suspect that this should also happen without this patch, but need to test more. Do you have any guess on what may be causing this?

Comment on attachment 8494784 [details] pull request, dialer part Doug, I verified that the bug exists on master and is unrelated to this patch. I reported it as bug 1073338 and am rerequesting review on this patch :)

Comment on attachment 8494784 [details] pull request, dialer part Ok, thanks for checking. I left one nit on the PR.

Comment on attachment 8495004 [details] pull request, views part thanks :gandalf for fixing and cleaning the code good work!

Comment on attachment 8494887 [details] pull request, contacts commons part thanks for splitting this up, easier to read and review good work!

Dialer part: Commit: https://github.com/zbraniecki/gaia/commit/8259e82d4e0e13a8b93d5fa0f6dfcaf1fd2155ca Merge: https://github.com/mozilla-b2g/gaia/commit/3871880f8abb7ef89c34ac0a063b137045c3f8ad

Contacts commons: Commit: https://github.com/zbraniecki/gaia/commit/6f00d88871bedad4bcb79a72aab1a4516e8f3fc9 Merge: https://github.com/mozilla-b2g/gaia/commit/59898f01e2397ea46e1248964523e05fbcdbdee4

Contacts views: Commit: https://github.com/zbraniecki/gaia/commit/096574a826cc8688feaaa5d4f087b4ce728a2d38 Merge: https://github.com/mozilla-b2g/gaia/commit/eeb860f0a2266fc3b535fb7a654ea740003674ba

This may be the last piece of the puzzle. There are two more things that I could do: - move TAG_OPTIONS to use l10nId - move Notification API to use l10nId What do you think. Worth it?

Comment on attachment 8496400 [details] pull request, utils I left substantive comments on GH. We need another round thanks

Clearing needinfo on me. I'm fixing the bug from comment 26 in bug 1073338.

Comment on attachment 8496400 [details] pull request, utils Thanks for the feedback! I applied changes and am rerequesting r?

Comment on attachment 8496400 [details] pull request, utils r=me provided the latest nits and comments are addressed properly. please check thanks for the work, Gandalf

Commit: https://github.com/zbraniecki/gaia/commit/bb852eadb69ab9fd7d30a9b7588b2a1dba3da89b Merge: https://github.com/mozilla-b2g/gaia/commit/16cc555fd45f796295b82d494534246eaf938529 I think we're good here for now. Notifications API is a major API rewrite that will require its own bug, TAG_OPTIONS is fairly isolated and can be fixed if you'll ever be doing a refactor of that code, and everything else has to wait for us to get more features in l10n API. I'm closing this bug. Thanks for all your help and reviews Jose!