Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. r+. Looks good! Dale: you're the suggested reviewer. Can you take a look at this patch?

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. Sorry to pass the buck, but I no longer work on the gallery

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. djf seems to have 10 reviews in his queue. Trying Gallery peer instead :)

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. The patch looks good for gallery app. The only reason it's r- because shared/js/dialogs.js changes are going to break video app. Video is recently updated to use shared/js/dialogs.js Bug 1011772. I guess we need to update both gallery and video app at the same time because of this sharing dependency.

papples, do you have any ETA for this patch?

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1011772#c20, only l10nId based dialogs.js changes for video app are covered in this bug. Do we have a bug for cleanup mozL10n.get uses in Video App as that's needed for bug 1047215. Setting NI flag for gandlaf to help answer above query. The patch looks good and has my r+ . Setting review flag for Russ for video changes. Please squash your commits into single commit and make sure you have a green gaia-try run before landing. Thanks

(In reply to Punam Dahiya (OOO 8/11 - 8/15) from comment #8) > As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1011772#c20, > only l10nId based dialogs.js changes for video app are covered in this bug. > Do we have a bug for cleanup mozL10n.get uses in Video App as that's needed > for bug 1047215. > Setting NI flag for gandalf to help answer above query. No, we don't have a separate bug for all mozL10n.get calls in Video app. It should be a new bug with dependency set on bug 1020138. This patch only fixes the Dialogs use in Video app.

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. There are some changes required to the patch in order for it to be used by the video app. See my comments in the PR.

The gallery localisation has been fixed without breaking the video app. It is out of this bug's scope to further integrate the video app with the shared code. Thanks for listing those changes, I have put them in a more suitable place: https://bugzilla.mozilla.org/show_bug.cgi?id=1047215 I may take it up if I have time. But feel free to make those changes yourself.

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. r- because of the changes to shared code. Since shared/js/dialogs.js is shared we have to be particularly careful changing it, and we have a higher standard of API pickiness to live up to. I would like the changes to add new features to Dialogs.showDialog() without breaking the existing API. So instead of changing the message, confirmText and cancelText, please leave those they way they are and add new messageId, confirmId and cancedId properties for the new features. And instead of formattedMessage, let's use messageId and messageArgs. Don't worry about the changes to showOverlay(). That function appears to only be used by Gallery, and it should never have been made shared at all. Punam: when you are back would you file and take a bug to move Dialogs.showOverlay() back into gallery (assuming I'm right that it is not actually shared. There is too much app-specific detail like hardcoded localizatsion ids in that function for it to be shared.)

Giovanni or Gandalf: could one of you explain the motivation for these changes? I use data-l10n for static strings in my HTML. But when the proper localization id needs to be computed at runtime, why don't you want me calling get() myself? It seems more efficient than setting an attribute that triggers a mutation observer that calls get()... Is there an accessibility issue here?

(In reply to David Flanagan [:djf] from comment #13) > Giovanni or Gandalf: could one of you explain the motivation for these > changes? I use data-l10n for static strings in my HTML. But when the proper > localization id needs to be computed at runtime, why don't you want me > calling get() myself? It seems more efficient than setting an attribute that > triggers a mutation observer that calls get()... Is there an accessibility > issue here? Sure, let me take this question. We've been planning the HTML5 L10n API for years now and the l10n-id attribute vs. runtime .get() has been the code of this paradigm switch we're doing right now. (by we I mean L10n Drivers team at Mozilla - mostly Axel, Stas and me) There are multple issues with .get, but I'll focus on three major ones: - get's are synchronous. Localization is not. Having asynchronous API allows us to write localizable code that reacts to when the resources are loaded, fallback on locale errors, and then react to when we want to retranslate. There are multiple scenarios in which we may want to retranslate UI. Currently in Firefox OS we have the dynamic language change, but in the future we will have more - we call it responsive l10n. You can read more about it on http://l20n.org/ and http://l20n.github.io/preso/responsive/ - get's are imperative. Establishing declarative API for UI localization allows us to develop smarter localization patterns and optimize the behavior, like bootstrap, in the future. - get's are error prone. Almost all localization bugs in Firefox OS since 1.0 are related to misuse of get, or manual replication of what Mutation Observer does globally. Edge cases are hard, race conditions are common, and most developers are not familiar with the depth of the topic of UI localization. Settling on a simple, declarative DOM API will enable us to reduce code complexity and remove the whole class of those errors. Of course we don't want to remove get's. We will have them in one way or another, as they are necessary in certain circumstances, but overall, we want to minimize the amount of get's and remove the cases where get is only used to write textContent into a Node. But in the future, we will standardize WebL10n API around l10n-id attribute and push the l10n.js bindings to Gecko itself. Migrating as much of our code base as possible to use this API is making our code more reliable and stable. Hope that helps, if you want to discuss it further, I'd be happy to chat or brown bag that.

(In reply to David Flanagan [:djf] from comment #12) > Comment on attachment 8458166 [details] > Patches the gallery app to set the data-l10n attribute instead of using > mozL10n.get. > > r- because of the changes to shared code. > > Since shared/js/dialogs.js is shared we have to be particularly careful > changing it, and we have a higher standard of API pickiness to live up to. > > I would like the changes to add new features to Dialogs.showDialog() without > breaking the existing API. So instead of changing the message, confirmText > and cancelText, please leave those they way they are and add new messageId, > confirmId and cancedId properties for the new features. And instead of > formattedMessage, let's use messageId and messageArgs. > > Don't worry about the changes to showOverlay(). That function appears to > only be used by Gallery, and it should never have been made shared at all. > Punam: when you are back would you file and take a bug to move > Dialogs.showOverlay() back into gallery (assuming I'm right that it is not > actually shared. There is too much app-specific detail like hardcoded > localizatsion ids in that function for it to be shared.) Created bug 1055715 to bring showOverlay method back inside gallery app. Thanks

Comment on attachment 8458166 [details] Patches the gallery app to set the data-l10n attribute instead of using mozL10n.get. I left one query and one style nit on github, but otherwise this looks good to me. I particularly like the new API for Dialogs.confirm(). Thanks for that. Note that in the meantime, we've landed a patch in gallery to use <gaia-header> and that has caused merge conflicts, so you'll have to address those before you can land it. Please test carefully after you to to ensure that nothing broke.

My pleasure. The nit and the merge issues have been addressed. I have tested that the gaia-header remains consistent when changing the language while selecting photos. This looks ready to land.

Just a general cleanup. This code was used to resolve Bug 1047215, so this patch no longer needs to be merged.

(In reply to Giovanni Charles from comment #18) > Just a general cleanup. This code was used to resolve Bug 1047215, so this > patch no longer needs to be merged. Hi Giovanni Can you please check if bug 1047215 patch included clean up of mozL10n.get uses in Gallery app. Thanks!

Oh, you're right. Well I've rebased it. Could you merge the patch once the tests pass?

TBPL tests looks green except gaia-intergration js tests failures which are not related to this patch. Reopening to test and land fix in master.

Patch landed on master https://github.com/mozilla-b2g/gaia/commit/ff3840108c03d0954f3803950351be64ef8dcbb2