This bug is about updating Clock to use modern L10n API and Intl API. This is part of an effort to remove l10n_date, migrate to Intl API, create mozIntl as a staging shim for future Intl API extensions and prepare for transition to l20n.js.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master This is by no means a patch ready for review. I still have some more porting to do, in particular: - I need to hunt down a couple bugs in UI - I need to update tests - I want to migrate duration time ("23h:50m:13s" style) to mozIntl - I want to consider using intlHelper for Intl.DateTimeFormat caching - ensure perf/memory But I think that those will not take too much code and I'd like to get a general feedback on the direction I'm taking. In principle I am: - moving Clock to use Intl API - introducing mozIntl as a shim for future Intl API (rev. 3 and next) - migrating all mozL10n.get to use mozL10n.formatValue - moving away from treating l10n as a lib, and into the "shim for Web API" world. Marcus, can you give me feedback on the code that I have there already? Thanks!

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master This looks good at first glance; I'm in support of the general direction you're taking. Unfortunately, we haven't been able to put much time into Clock's UI for more than a year, so some of its idioms are quite dated and are potentially fragile. Feel free to make any changes as necessary to make your integration easier. The only thing that stands out to be particularly aware of is making the logic async -- while I'm in favor of that, I'm not sure offhand how much that will affect the tests, which are themselves not in the greatest shape either. (Also, some of the tests are disabled on CI due to intermittents we've never had time to address; I believe most of these are just that the tests themselves are commented out.) In any case, it's probably fine, and I can also try to help if you run into problems there.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Ok, I think it's time for a review. I dived deep and refactored a lot of code focusing an using all modern localization and internationalization patterns. The big picture is: * (almost) full RTL support for date, time and numbers * no more l10n_date.js * no more mozL10n.get * use Intl API * use newly incarnated mozIntl API for features that we're standardizing in Ecma 402 * use IntlHelper for caching both Intl and mozIntl formatters * isolate reformatting * internationalize lots of small things like time separators, list separtors, time patterns * remove en-US strings from HTML * remove hardcoded assumptions about calendar. It's not fully multi-calendar yet, but it should handle other calendars once we get to it * remove all manual entity setting in Clock - everything happens via data-l10n-id * fixed multiple code paths that lead to redundant reformatting I'm really happy with this patch. While the codebase is still aged, I was able to remove everything that was there related to date/time/numbers/intl/l10n and replace it with our shiny new toys like Intl and IntlHelper. I may want to move mozIntl outside of this patch and into a separate bug to land it independently in case the review takes longer because I'd like to start using it as I refactor other apps. While I tested the app extensively, and I was able to use all its features while switching languages and time formats on fly, I consider it a fairly big refactor that will require solid QA. My vision for how to get to landing it is: - get your review on code - get stas's feedback on mozIntl - get ahmed's feedback on RTL improvements - get tests updated - land it and request QA Let me know what you think :)

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Stas, can you look at mozIntl and let me know what you think? I keep it in the patch because this patch is an excellent example of how the API will be used, but I may want to move it out into a separate bug and request a review from you. The specific API I'm using in mozIntl is not very strict and I'm ok updating once we get closer to ECMA 402 rev. 3 release and update all uses.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Ahmed, can you please test this app in RTL and let me know if I missed something obvious? I know I didn't cover full RTL - I see a few minor issues like the fact that the stopwatch is realigining - but I feel like I'm getting close to the capacity of human coder and reviewer to keep the code in place and I'm also afraid of bitrotting, so I'd prefer to land it first, and then work with you on follow ups. I believe that all three screens of the app now handle proper RTL, including date, time, numbers and localizations. The one remaining piece is time picker which is part of the System app and as such, out of scope for this bug. Let me know what you think! I'd love to get it in 2.5 to help you guys with RTL in 2.5 effort :)

PSA: This patch by no means indicate my interest or willingness to become an owner of Clock. I do not consent to the "whoever touched the code last owns it" rule.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Flod: can you look at the changes to the localization files? I moved a couple strings to new IDs because I need to update their variable names, and I added a couple rather complex strings to shared/date that will help with mozIntl. Originally I thought about introducing shared/mozintl/mozintl.properties file, but decided against so that we don't have to migrate date.properties that will be used by mozIntl (relative date). I do plan to kill l10n_date in this cycle and that will remove most of the strings from date.properties. What's left will stay there until Intl API rev 3. (next year). If you have any ideas for how to make the string comments more helpful to the localizers, let me know! One thing I noticed is that the listSeparator can't have space before value - so no ' ,', but ', ' works. That's an intrinsic limitation of the .properties format and will work once we switch to l20n.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Strings look good with a few nits fixed. I think we really need to keep an eye out, especially on Pootle locales, to see how they work on these formatting strings (also DMY and HM from another recent bug).

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Everything I've seen looks good so far. I've only brushed over the details of the l10n api because others are covering that area, but the clock changes seem sound. One area, as mentioned on github, is that since you changed the database format of repeating alarms, we'll need to build in an upgrade path for that. Fortunately, that's entirely a matter of updating the normalization function in alarm_database.js (i.e. we don't need to mess with indexeddb schema upgrades or anything), and I think the alarm_database_test.js is in reasonable condition to make adding a test for the upgrade path simple. Clearing review for now as I'd like to have another look at the Clock changes before it lands to verify the database upgrade and spend a few moments testing on-device. Nothing major though, everything else looks good and should be a nice cleanup. :)

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master f=me with comments left in the PR. Please document the limitations of token formatting and make sure shared/js/intl_helper.js works with l20n.js (DOMLocalized?).

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master The patch so far is better than expected :) However there are a couple of bugs I found Choosing the snooze duration will bring up a menu with the same string, which is "minutes دقائق" which means "minutes minutes". The duration of the snooze under vibration option in new alarm view uses Western Arabic numerals instead of eastern like everywhere else in the app shows exactly "minutes 10". The little notification in the bottom of the app when you set a new alarm tells you how much left before the alarm goes on using also Western Ar numerals In Timer view: +1 Min also uses Western Arabic numerals (1 instead of ١) Thanks! :)

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Ready for a new review. The patch has been updated to :stas and :mcav feeedback, tests have been updated and mocks have been created. Seems like the review process goes quick with this one, so I may want to keep mozIntl as part of this patch

Thanks Ahmed for the feedback! (In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #12) > Choosing the snooze duration will bring up a menu with the same string, > which is "minutes دقائق" which means "minutes minutes". Oh, that was a bug also in en-US! THanks for spotting it. Fixed and added numFormatter to transform digits. > The duration of the snooze under vibration option in new alarm view uses > Western Arabic numerals instead of eastern like everywhere else in the app > shows exactly "minutes 10". Fixed as well. > The little notification in the bottom of the app when you set a new alarm > tells you how much left before the alarm goes on using also Western Ar > numerals Fixed! > In Timer view: +1 Min also uses Western Arabic numerals (1 instead of ١) That's actually localization bug: http://hg.mozilla.org/gaia-l10n/ar/file/6ce3b0d1c93e/apps/clock/clock.properties#l51

I moved mozIntl to bug 1208347. I'll keep it in this patch until it lands on master, so that the patch is testable.

Performance and memory looks good. Tested on Flame-kk master with 31 runs: master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------- | -------- | | navigationLoaded | 3589.645 | 3604 | 3000 | 3716 | 114.503 | 3665.850 | | navigationInteractive | 3681.710 | 3692 | 3088 | 3807 | 115.468 | 3766.650 | | visuallyLoaded | 3941.806 | 3915 | 3417 | 5388 | 283.457 | 4047.300 | | contentInteractive | 3942.290 | 3916 | 3418 | 5388 | 283.342 | 4047.300 | | fullyLoaded | 3942.516 | 3916 | 3418 | 5389 | 283.469 | 4048.250 | | pss | 26.247 | 26.448 | 25.472 | 26.549 | 0.280 | 26.530 | | uss | 21.233 | 21.434 | 20.457 | 21.535 | 0.280 | 21.516 | | rss | 43.620 | 43.824 | 42.848 | 43.922 | 0.280 | 43.906 | patch: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------- | -------- | | navigationLoaded | 3586.677 | 3616 | 2968 | 3704 | 121.691 | 3667.650 | | navigationInteractive | 3721.097 | 3747 | 3142 | 3838 | 115.136 | 3799.800 | | visuallyLoaded | 3922.452 | 3946 | 3394 | 4048 | 106.839 | 4000.600 | | contentInteractive | 3922.645 | 3946 | 3394 | 4048 | 106.888 | 4001.550 | | fullyLoaded | 3922.903 | 3946 | 3395 | 4048 | 106.718 | 4001.550 | | uss | 20.928 | 21.066 | 20.551 | 21.145 | 0.234 | 21.098 | | pss | 25.938 | 26.077 | 25.561 | 26.154 | 0.234 | 26.109 | | rss | 43.318 | 43.457 | 42.938 | 43.531 | 0.235 | 43.492 | the performance metrics are comparable with possibility of slight perf-win, but with 50ms range. The memory is more promising with USS showing ~200-500kb drop (depending if you look at mean, median or p95)

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Stas, can you review the IntlHelper changes?

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Wow, that's a great re-write. r=me on moz_intl changes.

And by moz_intl, I meant IntlHelper :)

I had to remove some number formatters because I discovered that they don't work well with plural forms in mozL10n - in arabic, the plural form was always 'other'. I believe the right solution is to make mozL10n format numbers passed to it and I filled bug 1208800 to enable that.

Marcus: while testing my patch I discovered an error when alarm goes off. The error is here: https://github.com/mozilla-b2g/gaia/blob/566ca621e80c40d71b818d3aa8d39e2e96ff85d5/apps/clock/js/audio_manager.js#L178 Instead of this._interruptHandler(e); it should probably be this._interruptHandler.onInterrupt(e); The problem is that when I switch it, the alarm only rings one and then goes silent, so maybe it should be there at all? I know it's not related to this patch but if you know how to solve it and it's a one liner (change line or remove the listener), I can add it to the patch :)

Good catch! Yes, that should be changed; r+ from me to include it in this patch.

Ok, but you need to tell me which way :) Should I remove the listener or fix it? As I said, I'm not sure why, but when I fix the call it silences the alarm/timer after one ring, which I don't think is the proper behavior... Also, I rebased my patch to be on top of patch from bug 1208698 - :stas is reviewing there the mozIntl/IntlFormat bits.

Ah yes, right. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1209123 to track this; let's spin it off.

Comment on attachment 8664088 [details] [gaia] zbraniecki:1207044-refactor-clock-l10n-and-intl > mozilla-b2g:master Tested locally on-device and I don't see anything unusual; the upgrade test coverage looks good to me.

Thanks everyone! Commit: https://github.com/mozilla-b2g/gaia/commit/9cfe94bf29e76e24cb7558d36f8a9337f5ea7ffd

Reopening for causing bug 1210234.

Backed out in master at https://github.com/mozilla-b2g/gaia/commit/177b34a6a5c4f90366a6573e1c17c1fae8b643dd

Relanded: https://github.com/mozilla-b2g/gaia/commit/4f1093c1eafb3e932cbe55463fb664aac73426bd Fixes: https://github.com/mozilla-b2g/gaia/commit/a0bd10ed4f1d923d2cf98ba7e2d8cfea3ec7c926