l10n.js should warn when entities are missing

RESOLVED FIXED in B2G C3 (12dec-1jan)

Status

Firefox OS
Gaia
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kaze, Assigned: kaze)

Tracking

({l12y, regression})

unspecified
B2G C3 (12dec-1jan)
x86_64
Linux
l12y, regression

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: QARegressExclude)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 691523 [details] [diff] [review]
patch proposal

STR:
1. start an application with missing l10n entities, e.g. Settings
2. adb logcat | grep l10n

Expected result, we should get these two lines in the logcat:
  [l10n] #tones is undefined
  [l10n] #ringer is undefined

Actual result: nothing in the logcat.

This is a regression caused by bug 810983 — which I reviewed. :-(

I propose to fix it with three gDEBUG levels:
  0: don’t display any console message
  1: display only warnings, not logs
  2: display all console messages
… and I suggest to use 1 as default value for now, in order to find the missing entities.
(Assignee)

Updated

5 years ago
Assignee: nobody → kaze
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
Keywords: l12y, regression
(Assignee)

Updated

5 years ago
Attachment #691523 - Flags: review?(ttaubert)
Comment on attachment 691523 [details] [diff] [review]
patch proposal

Review of attachment 691523 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #691523 - Flags: review?(ttaubert) → review+
Triage: BB+, C3, P1 - l10n + regression
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
(Assignee)

Comment 3

5 years ago
https://github.com/mozilla-b2g/gaia/commit/b08d3cdd765089d5941c5eed071c0d29b11a7e96
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Should this be generating warnings for the case where a default value is provided?  Specifically, mozL10n.get() takes a 3rd parameter that provides a default string.  At least in the source file, the comments don't make it clear what the semantics for providing a default string values are.

The use-case we are using this for in the e-mail app is translating error codes into error messages with a fallback if we didn't create a specific string for the error code.  This was sort-of a hack; it probably makes more sense for us to have an explicit dict or switch-lookup that maps error codes to error strings.  This would let us map error codes to close error strings, plus deal with the localization idiom reality where we need to bump 'string-id' to 'string-id2' when we change our phrasing for the string.  But I thought I'd chime in since we are using the functionality...

I'd probably vote for just keeping the code like it is now, but adding a comment to mozL10n.get to explain what the purpose of the default string argument is meant to be.
(Assignee)

Comment 5

5 years ago
(In reply to Andrew Sutherland (:asuth) from comment #4)
> Should this be generating warnings for the case where a default value is
> provided?

Yes, I think we should.

I agree it’s confusing, and I now think that the fallback mechanism in `navigator.mozL10n.get' is not so good. At the very early ages of l10n.js it was handy to either specify a string fallback or see {{key}} when #key was undefined; but now I’d prefer to see an empty string if #key is undefined, so we could handle the fallback like this:

  str = _('errorCode', args) || 'something bad happened.';

instead of:

  str = _('errorCode', args, 'something bad happened.');

I don’t want to break this fallback mechanism before the basecamp release, but I can add a comment to say that the `fallback' argument will be deprecated and won’t prevent l10n warnings if the string couldn’t be translated.

Does this make sense to you? Would it be an issue for you to drop the current fallback mechanism after the basecamp release?
(In reply to Fabien Cazenave [:kaze] from comment #5)
> Does this make sense to you? Would it be an issue for you to drop the
> current fallback mechanism after the basecamp release?

Yes, it makes a lot of sense and seems like the right call.  I can drop the fallback mechanism now; I've got a patch on bug 816039 that's changing that area of code anyways.  (And am sadly introducing a bunch of new strings, so there's no need for the fallback case anymore anyways.)

Updated

5 years ago
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.