Bug 855524 - Localize lazy loaded contents #9035

Merged
merged 1 commit into from Apr 11, 2013

Conversation

Projects
None yet
3 participants
@fabi1cazenave

This comment has been minimized.

Show comment Hide comment
@fabi1cazenave

fabi1cazenave Apr 8, 2013

Contributor

I’m not familiar with the Browser code base but this patch worries me a bit:

  • (nitpick) the braces are missing for the if statement.
  • mozL10n.translate() works on DOM elements only — I’m not sure what “file” is in this context, maybe it’s just a misleading variable name?
  • does your code make sure that mozL10n is ready before getting in there? You should probably rely on mozL10n.ready(callback) or check the mozL10n.readyState property.
Contributor

fabi1cazenave commented Apr 8, 2013

I’m not familiar with the Browser code base but this patch worries me a bit:

  • (nitpick) the braces are missing for the if statement.
  • mozL10n.translate() works on DOM elements only — I’m not sure what “file” is in this context, maybe it’s just a misleading variable name?
  • does your code make sure that mozL10n is ready before getting in there? You should probably rely on mozL10n.ready(callback) or check the mozL10n.readyState property.
@fabi1cazenave

View changes

apps/browser/js/browser.js
+ } else {
+ mozL10n.ready(localizeElements);
+ }
+

This comment has been minimized.

Show comment Hide comment
@fabi1cazenave

fabi1cazenave Apr 11, 2013

Contributor

if you use mozL10n.ready(), you don’t need to check the readyState property. This would be fine:

var mozL10n = navigator.mozL10n;
mozL10n.ready(function browser_localizeElements() {
  elementsToLoad.forEach(function l10nElement(element) {
    mozL10n.translate(element);
  });
});

Thanks for the filesToLoad / elementsToLoad separation, it’s very clear now. :-)

@fabi1cazenave

fabi1cazenave Apr 11, 2013

Contributor

if you use mozL10n.ready(), you don’t need to check the readyState property. This would be fine:

var mozL10n = navigator.mozL10n;
mozL10n.ready(function browser_localizeElements() {
  elementsToLoad.forEach(function l10nElement(element) {
    mozL10n.translate(element);
  });
});

Thanks for the filesToLoad / elementsToLoad separation, it’s very clear now. :-)

benfrancis added a commit that referenced this pull request Apr 11, 2013

Merge pull request #9035 from crh0716/855524
Bug 855524 - Localize lazy loaded contents

@benfrancis benfrancis merged commit 2d48fb6 into mozilla-b2g:master Apr 11, 2013

1 check failed

default The Travis build failed
Details

benfrancis added a commit that referenced this pull request Apr 11, 2013

Merge pull request #9035 from crh0716/855524
Bug 855524 - Localize lazy loaded contents(cherry picked from commit 2d48fb6)

benfrancis added a commit that referenced this pull request Apr 11, 2013

Merge pull request #9035 from crh0716/855524
Bug 855524 - Localize lazy loaded contents(cherry picked from commit 2d48fb6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment