Maybe: 1. Date and time taken. 2. Dimensions 3. Portrait or landscape 4. Human readable location

I chose to have the localization in the media frame object, and in the video player widget. Mostly, because they encapsulate the markup, and setting attributes on different elements should be done from there. This requires us to have shared media translatable strings, but I think the case for that has already been made with the video player controls in bug 1068998.

Adding dimensions gets really chatty (e.g. "two thousand five hundred thirty six by one thousand eight hundred and twenty seven"). So for now, we have date and orientation. For videos, duration would be nice, but we only get that after the player loads the file.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Changing this to a "r?" since we discussed this on IRC.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Various comments on github. The main reason for r- is that I think this will break the Camera app, which also uses these modules.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 I think I addressed all of your comments on github. Camera app is working again!

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 r+ for the shared and Gallery changes, but I'm asking Diego to take a look at the Camera changes. I think there is a better way to handle the l10n_date dependency so it doesn't load at startup.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Converting my review to r- because I just realized (while reviewing an RTL bug) that the use of navigator.mozL10n.ready() in the MediaFrame() constructor causes a memory leak. If we do this, then the l10n.js will always have a reference to the bound localize method. And the bound localize method will always have a reference to the MediaFrame object. So MediaFrame objects will never be garbage collected. I'm pretty sure that Gallery onlys creates 3 MediaFrame objects and doesn't discard them. But I don't know about Camera. Even if these two apps are safe, we can't modify shared code to add a memory leak. Listening for the 'localized' event on the window object might be safe, assuming that the DOM uses some kind of weak reference for event handlers. I'm embarassed to say I don't actually know whether it does not not. We should probably also file a bug on l10n.js to get it to use WeakMap or WeakSet in its internal EventEmitter implementation so that it won't cause leaks like this.

I should add that the reason that Gallery can get away with using a ready() to trigger localization of its thumbnails is that it knows that there is only ever going to be a single ThumbnailList object. The code is trick, though, and I should change it. For this patch I think the best thing to do is have MediaFrame use once() to do one-time localization, and define a localize() method (as you already do). Then, modify Gallery and Camera so that they have a top-level ready() call that invokes that localize() method on any MediaFrame objects.

I filed bug 1135251 for the ThumbanilList issue, and bug 1135256 for l10n.js memory leak issue.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Changed it per your suggestion. Tell me if you think the solution I proposed in bug 1135256, comment #6 is feasible. It has the advantage of encapsulating MediaFrame.

I haven't looked at these latest changes yet, but I did realize that you need to update gallery/open.html to include the link to the new shared localization file. open.html is what handles the view activity for things like viewing a photo from the sms app.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 You still have a ready() call in video_player.js, and I think you need to modify the MediaFrame.localize() method to call the VideoPlayer.localize() method. Also, as noted in the comment above, both you and I forgot about the open/view activity in previous iterations of this patch. gallery/open.html is a separate entry point used by that activity. It also uses MediaFrame, and so will need a link to the new strings, and gallery/js/open.js will need the ready() call to handle localization.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 OK, made some changes. Specifically have each component localize itself and maintain a weakmap of active instances.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Sorry this review took so long. I've left some nits and other thoughts on github, but basically this looks good to me. I'd still like a camera peer to sign off on the camera app changes. If Diego does not respond, maybe as Justin D'Arcangelo. Wilson Page would also be good, but he is on vacation this week, I think.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 Built/tested Camera changes. LGTM!

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: Screen reader users will not be able to get information about photos and videos in fullscreen view in the gallery and preview in the camera app. [Testing completed]: Yes. [Risk to taking this patch] (and alternatives if risky): Medium, this is a change to shared JS. [String changes made]: Yes, description templated for videos and photos were added.

Comment on attachment 8556750 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27797 I'm going to upload a new PR rebased on v2.2 for approval.

Comment on attachment 8578778 [details] [gaia] eeejay:bug-1068976_v2.2 > mozilla-b2g:v2.2 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: Screen reader users will not be able to see fullscreen images and videos in gallery app, not in the camera preview screen. [Testing completed]: Yes. [Risk to taking this patch] (and alternatives if risky): Medium, changes to shared JS. [String changes made]: Yes, Added templates for photo and video descriptions.

The rebased version has the squashed one-line change from bug 1142100 that fixed a regression with screenshots.

v2.2: https://github.com/mozilla-b2g/gaia/commit/7935b92dbfc817fdfa3b27cc4f19bf05f8c0c725