1013891 - Move pref and message-manager strings to constants

Status: RESOLVED WONTFIX

(Firefox :: Translation, defect)

Description

[Mike de Boer [:mikedeboer]]

In Translation.jsm I noticed that pref names and message identifiers are hardcoded and duplicated inside methods.

Preferably, we'd move these strings to the top of each file to
1) improve readability
2) improve usability
3) prevent typos in the future.

Comment 1

[Mike de Boer [:mikedeboer]]

Marco, can you add this to the current iteration?

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: move often used strings to shared constants

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8426891 [details] [diff] [review]
Patch v1: move often used strings to shared constants

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

I think the cost of hiding the constant values in a separate file and of having to use the preprocessor outweighs the benefit of reducing the typo risk.

I think I would prefer having the constants defined at the top of both file. That's a small duplication, but the risk of typo while copying whole lines seems almost non existent.

::: browser/components/translation/constants.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#endif
> +
> +const TRANSLATION_SOURCE_LANGUAGES = ["en", "zh", "ja", "es", "de", "fr", "ru", "ar", "ko", "pt"];
> +const TRANSLATION_TARGET_LANGUAGES = ["en", "pl", "tr", "vi"];

These 2 arrays weren't duplicated, right?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I think the cost of hiding the constant values in a separate file and of
> having to use the preprocessor outweighs the benefit of reducing the typo
> risk.
> 
> I think I would prefer having the constants defined at the top of both file.
> That's a small duplication, but the risk of typo while copying whole lines
> seems almost non existent.

Sure, matter of taste, but sure. I'll post an updated patch later.

NB: the reason I opted for this method is because I generally like to avoid having to browse through a list of constants before I get to the meat of a module. I consider it boilerplate and not volatile code (hence the term 'constant'). However, the cost of separating constants to their own respective module is too much work and, as we all know, module loading is not cheap. Hence the choice to include it using pre-processor directive, because that's free at run-time.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I actually don't like the indirection of using constants for this - the typo risk seems small (and should be covered by tests!), and it adds an extra step when grepping for where messages are sent/observed, which I find make understanding the code slightly harder.