In order to ship a part of bug 1146239 in time for dev-edition 40, we decided to use an existing l10n string from another tool in /browser/devtools/performance/views/recordings.js This is bad because we shouldn't reuse strings from other contexts: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Don%27t_reuse_strings_in_different_contexts So in this bug, we should add a new dedicated string for when a record has finished and is being loaded, in performance.properties: # LOCALIZATION NOTE (recordingsList.loadingRecordLabel): # This string is displayed in the recordings list of the Profiler, # for an item that has finished recording and that is being loaded in the tool # (i.e. has not yet appeared). recordingsList.loadingRecordLabel=Loading record…

We'll have some unlocalized strings for 40.1 -- this is a bug to keep track of unlocalized changes landed to readd and revisit for 41.

Revisit: recordingsList.saveDialogTitle=Save profile… Changed in bug 1164593, both the save and import dialogs have been updated without l10n.

profiler.bufferStatus no longer used via bug 1166124 due to string change

We've needed to add or change a handful of strings for the 40.1 release on June 2nd -- right now they're hardcoded and uplifting next week from nightly to aurora 40.1. Is there a better way to handle these string change uplifts? Its my understanding we cannot uplift any l10n changes and did what we could to preempt what strings we'd need, but still some changes are necessary. Luckily, I believe a good amount of them are unlocaluzable strings (using technical descriptions that reference known names and intrinsic JS things) Any thoughts, recommendations, advice for this?

Mozilla-aurora is string frozen in order to give localizers 6 weeks to work on localization. If necessary it's possible to land new strings in mozilla-aurora, approval is on release-drivers, but in general we try to avoid that as much as possible. Also, devtools are actually localized only in a subset of locales, but landing strings make noise for all of them. It would be a lot easier to give a suggestion with a clear indication of the amount of string changes requested (to see if we can work around them for fx40), and when these changes would eventually be ready for landing.

Still not sure how many changes are needed; should know by end of week. With bug 1167317 looming, I wonder if this would be duplicated effort

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #5) > It would be a lot easier to give a suggestion with a clear indication of the > amount of string changes requested (to see if we can work around them for > fx40), and when these changes would eventually be ready for landing. I have a very preliminary try push here: https://hg.mozilla.org/try/pushloghtml?changeset=ed5fcd147ed5 (from Bug 1167252). Out of these, the only one that I see that currently needs string changes is Bug 1143004, which is coincidentally the only one I've requested uplift for. Jordan, am I missing anything? I know there was discussion about changing a couple of things soon (like Bug 1150761) but it seems like maybe there are just going to be a few strings that we are dealing with

And for reference, here is output of hg out -p with the current perf patches applied

To clarify, any strings after May 12th which should've been localized were just hardcoded strings (other than bug 1143004) -- those are listed in the comments at the beginning of this bug

So, unless I'm missing some strings. For profiler.properties: Loading record… Import recording… Save recording… Buffer ${percent}% full For timeline.properties Buffer ${percent}% full @Pike Any strong opinion about this? Even if not ideal, I think that we can live with some hard-coded strings in Firefox 40 for devtools, and let the proper fix ride the trains. Side note: if we decide to go ahead and land these on mozilla-aurora, it would help to land them in one step. That's why I think it makes more sense to think about Fx40+devtools+strings globally.

Details View Names in bug 1150761, hardcoded

I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle rather than more uplifting work (it's already a nightmare) and everything else that needs to get done is way more complicated with all the release changes than I'd like, unless there's strong feelings otherwise. If that's the case, we should hardcode the timestamp text in bug 1143004 that bgrins mentioned for now

Fine with me.

(In reply to Axel Hecht [:Pike] from comment #13) > Fine with me. OK, let's go with hard-coded strings for Aurora and make sure that they're probably localizable on central for fx41.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12) > I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle > rather than more uplifting work (it's already a nightmare) and everything > else that needs to get done is way more complicated with all the release > changes than I'd like, unless there's strong feelings otherwise. > > If that's the case, we should hardcode the timestamp text in bug 1143004 > that bgrins mentioned for now Sounds good Jordan, can you attach a patch with the hardcoded timestamp text here so I can track it for the uplift?

(In reply to Brian Grinstead [:bgrins] from comment #15) > (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12) > > I'd prefer to keep them hardcoded for the remaining 3 weeks in this cycle > > rather than more uplifting work (it's already a nightmare) and everything > > else that needs to get done is way more complicated with all the release > > changes than I'd like, unless there's strong feelings otherwise. > > > > If that's the case, we should hardcode the timestamp text in bug 1143004 > > that bgrins mentioned for now > > Sounds good Jordan, can you attach a patch with the hardcoded timestamp text > here so I can track it for the uplift? Actually, I guess what we really need is a copy of the patch already in Bug 1143004 attached over there, but with the hardcoded string instead of the localized string. We can then request uplift on that one separately since we're going to need to land a different version on Aurora than what landed on m-c.

The texts are still hardcoded (on central, aurora and beta). What's the plan to make them localizeable or use the translations?

We just need to update the localization for the hardcoded strings -- I can't imagine we'll want to uplift, due to low amount of l10n for devtools, as well as the internal-specific/technical text used frequently.

Outstanding ni? on GC/CC marker naming, emailed Nick but here's prob better: Going through the L10N stuff that we didnt do for these markers (or anything in the last month or so), and I know we had a few discussions somewhere about these, but wanted to run them by you, as I don't really remember. Currently: "GC Event" and "GC Event (Non-incremental)" are the labels used for gc events, with "Cause" and "Non-incremental Cause" in non-inc cases for the properties. "Cycle Collection" for CC event labels, with only a field for "Type" displaying Collect or ForgetSkippable. -- What we have now, I'll probably add a "Type" field to GC's (displaying incremental or non-incremental, making it obvious), and wonder if we can have both of the gc/cc markers have similar labels -- maybe "CC Event" and "GC Event"? Chrome has "Minor GC", maybe we can just have "GC (major/minor)" and "CC (major/minor)", but I believe you said the major/minor explanation wasn't the best way to describe it -- although it might be a good-enough description that we can follow up with more information (like the "Type" field will explain Collect/ForgetSkippable, and could link to MDN for more info)

Minor/major is whether it is a nursery collection or a full tenured heap collection. We shouldn't conflate existing definitions. Cycle collections don't have major/minor; we shouldn't try and force it on them and again conflate that with major/minor GC. Additionally, we only trace major gcs right now. Every major gc slice starts with a minor collection of the nursery before proceeding. We also shouldn't conflate "GC Events" with actual DOM events, because they aren't DOM events. It's just confusing. --------------8<----------------8<---------------8<------------- Here are the marker names and properties I'd go with: * "Minor GC" marker name for minor (aka nursery) collections (nice to have but not essential feature, not yet implemented) * "Incremental GC" marker name for incremental major gc slices - reason: gc reason * "Nonincremental GC" marker name for when we are forced into a nonincremental major gc (note: this is often very suboptimal) - reason "<the gc reason>" - nonincremental reason: "<the nonincremental reason>" * "Cycle Collection" marker name for when we are actually doing cycle collection (ie not ForgetSkippable) * "CC Graph Reduction" marker for ForgetSkippable. This is the best name I can think of, as the point of ForgetSkippable is to reduce the size of the CC graph considered so that it is faster to collect. --------------8<----------------8<---------------8<------------- "Cycle Collection" and "CC Graph Reduction" are two separate things often running at separate times and should really be treated as such. Yes, we should have docs for these on MDN, as we should have for all the markers.

Added the label names for GC/CC markers as per nick's comment

Comment on attachment 8634956 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8634956 [details] [diff] [review]: ----------------------------------------------------------------- I stopped after markers.properties because I don't understand this patch at all. You're adding strings to profiler.dtd that are already in the file. Is this patch against mozilla-aurora (Firefox 41)? You *can not* land a patch with strings in Firefox 41. We just need to fix the missing strings on trunk, and wait for the fix to ride the trains. ::: browser/locales/en-US/chrome/browser/devtools/markers.properties @@ +41,5 @@ > +# The following labels should most likely not be translated, as they correspond > +# to JS functions. > +marker.label.javascript.setInterval=setInterval > +marker.label.javascript.setTimeout=setTimeout > +marker.label.javascript.requestAnimationFrame=requestAnimationFrame I think you should completely remove these 3 strings. As you say they're function names, no need to make them localizable. @@ +61,5 @@ > +# Field names for stack values > +marker.field.stack=Stack: > +marker.field.startStack=Stack at start: > +marker.field.endStack=Stack at end: > +marker.field.asyncStack=(Async: %S) What is %S?

Waiting on updated patch addressing flod's comments.

Comment on attachment 8634956 [details] [diff] [review] 1163763-l10n.patch See previous comment.

Comment on attachment 8634956 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8634956 [details] [diff] [review]: ----------------------------------------------------------------- This is for Fx42 -- this cleans up several localizations that were in several different files previously for the two separate tools (profiler, timeline) and organizes them logically according to the UI (performance) as well as the subcomponents (l10n file just for markers themselves, which could be used elsewhere in the future). I'm not sure what you mean by adding strings to profiler.dtd that are already in the file (as everything was moved to performance.dtd)

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #27) > I'm not sure what you mean by adding strings to profiler.dtd that are > already in the file (as everything was moved to performance.dtd) It's been a while, and the only explanation is that I completely misread the patch (not sure how) :-\ Setting NI to check again with more attention. This is going to be pretty painful for localizers, with so many strings moving at the same time (tools should be able to cope with translation memory, not so much if you localize with text editors).

Comment on attachment 8634956 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8634956 [details] [diff] [review]: ----------------------------------------------------------------- A few nits together with the ones about markers.properties, but besides that I think strings should be OK. Sorry again for the confusion about file renaming. ::: browser/locales/en-US/chrome/browser/devtools/markers.properties @@ +51,5 @@ > +# with the details. For examples: Paint (200x100), or console.time (FOO) > +marker.fieldFormat=%1$S (%2$S) > + > +# LOCALIZATION NOTE (time.field.*): > +# Strings used in the waterfall sidebar as property names. There's a localization note but no time.field.* strings Did you mean marker.field.*? ::: browser/locales/en-US/chrome/browser/devtools/performance.dtd @@ +159,5 @@ > + > +<!-- LOCALIZATION NOTE (performanceUI.console.stopCommandStart/stopCommandEnd): > + - This string is displayed when a recording is selected that started via console.profile. > + - Indicates how to stop the recording, wrapping the command, like > + - "Stop recording by entering console.profilEnd("label") into the console." --> Extra copy and paste? ::: browser/locales/en-US/chrome/browser/devtools/performance.properties @@ +2,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +# LOCALIZATION NOTE These strings are used inside the Performance Tools > + Nit: remove empty line @@ +24,5 @@ > +# Used for the menuitem in the tool menu > +performance.commandkey=VK_F5 > +performance.accesskey=P > + > +# LOCALIZATION NOTE (performance.tooltip3): performance.tooltip @@ +47,5 @@ > + > +# LOCALIZATION NOTE (recordingsList.loadingLabel): > +# This string is displayed in the recordings list of the Performance Tools, > +# for an item that is finished and is loading. > +recordingsList.loadingLabel=Loading\u2026 Any specific reason to use \u2026 instead of …? @@ +91,5 @@ > +category.storage=Storage > +category.events=Input & Events > +category.tools=Tools > + > +# LOCALIZATION NOTE (graphs.ms): table.ms @@ +95,5 @@ > +# LOCALIZATION NOTE (graphs.ms): > +# This string is displayed in the call tree after units of time in milliseconds. > +table.ms=ms > + > +# LOCALIZATION NOTE (graphs.ms): table.percentage @@ +167,5 @@ > +# LOCALIZATION NOTE (profiler.bufferFull): > +# This string is displayed when recording, indicating how much of the > +# buffer is currently be used. > +# %S is the percentage of the buffer used, > +profiler.bufferFull=Buffer %S%% full Please add in the note why there are two %.

Addressed all comments!

Comment on attachment 8642201 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8642201 [details] [diff] [review]: ----------------------------------------------------------------- Strings look good, thanks.

Comment on attachment 8642201 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8642201 [details] [diff] [review]: ----------------------------------------------------------------- Thanks flod!

Comment on attachment 8642201 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8642201 [details] [diff] [review]: ----------------------------------------------------------------- This is so nice and clean now! ::: browser/devtools/performance/modules/logic/marker-utils.js @@ +301,3 @@ > "setInterval handler": "setInterval", > "setTimeout handler": "setTimeout", > "FrameRequestCallback": "requestAnimationFrame", Add some comments here about how these labels shouldn't be localized because they represent actual js API. @@ +352,5 @@ > */ > JSFields: function (marker) { > if ("causeName" in marker && !JS_MARKER_MAP[marker.causeName]) { > + let cause = PREFS["show-platform-data"] ? marker.causeName : GECKO_SYMBOL; > + return { [L10N.getStr("marker.field.causeName")]: cause }; Nit: keep consistent formatting. Newline after { and before } Are we sure it's safe to localize object properties? Are we relying on English property names anywhere? It's probably safe from an utf pov, but I'm worrying about assuming this property is always called "Reason" somewhere in the codebase. @@ +384,5 @@ > } > }, > > CycleCollectionFields: function (marker) { > + return { [L10N.getStr("marker.field.type")]: marker.name.replace(/nsCycleCollector::/g, "") }; Nit: keep consistent formatting. Newline after { and before }

Comment on attachment 8642201 [details] [diff] [review] 1163763-l10n.patch Review of attachment 8642201 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/modules/logic/marker-utils.js @@ +352,5 @@ > */ > JSFields: function (marker) { > if ("causeName" in marker && !JS_MARKER_MAP[marker.causeName]) { > + let cause = PREFS["show-platform-data"] ? marker.causeName : GECKO_SYMBOL; > + return { [L10N.getStr("marker.field.causeName")]: cause }; I'm more worried about utf-8'ing property names, but if they're a valid JS string, it should be fine. There isn't any place in the code base that relies on this field being "Reason", as this shouldn't be any different than any other localized field, which should be handled just fine

test fix

https://hg.mozilla.org/mozilla-central/rev/3301c85aa671 https://hg.mozilla.org/mozilla-central/rev/20ced115bd64

>> <!ENTITY performanceUI.table.selfDuration.tooltip "The amount of time spent only within this function."> >> <!ENTITY performanceUI.recordButton.tooltip "Toggle the recording state of a performance recording."> >> <!ENTITY performanceUI.options.gear.tooltiptext "Configure performance preferences."> >> etc. Once again, please make it a habit *not* to use trailing periods in tooltips, unless when instructing a user or using full sentences, which is very, very rare (such as performanceUI.bufferStatusTooltip.) Descriptions or actions should NOT use them, as they generally use infinitive mood, while adding them results in imperative mood, making users think they are ordered to do something, both in English and for any other locale where different verbs may be needed. As imperative and infinitive mood look similar in English, erroneously placed periods make the problem hard to recognize for non-English, but even more vital for English users for the reason above. As a localizer I keep running into improper (and inconsistent) use of end stops in tooltips on en-US devtools files on almost any FF version. I’ve commented on this issue 3 times before and even had to file a bug once to have some corrected, while people keep making this mistake. Now I might be wrong, but think localizers shouldn’t need to worry about, nor spend time on finding out whether or not infinitive or imperative mood is meant in strings, nor remind developers about proper use more than once. Anyone working on devtools files should seriously take this into account just like any other source file where people do so. Tooltips just shouldn’t use periods, period. If possible (yes, it probably is), please watch/fix this when entering/moving strings, as well as when doing QA, and always keep this in mind. Thanks.

Filed bug 1199515 for correcting the end stops in the performance tool. (In reply to Ton from comment #39) > As a localizer I keep running into improper (and inconsistent) use of end > stops in tooltips on en-US devtools files on almost any FF version. I’ve > commented on this issue 3 times before and even had to file a bug once to > have some corrected, while people keep making this mistake. We're a large team covering many tools, it's likely most (myself included) were not aware of the tooltip grammar rule. > Now I might be wrong, but think localizers shouldn’t need to worry about, nor spend time on > finding out whether or not infinitive or imperative mood is meant in > strings, nor remind developers about proper use more than once. Anyone > working on devtools files should seriously take this into account just like > any other source file where people do so. Tooltips just shouldn’t use > periods, period. We have many contributors, volunteers and those on the team, where english is not their native language, or may not know grammar rules of imperative and infinitive moods -- we try, but mistakes and lack of good grammar (speaking about myself here) will happen -- pinging someone from localization on any L10N changes/additions could be a solution for those less confident in string changes. I don't think it's unreasonable to remind an entire team more than once about this.

(In reply to Ton from comment #39) > >> <!ENTITY performanceUI.table.selfDuration.tooltip "The amount of time spent only within this function."> > >> <!ENTITY performanceUI.recordButton.tooltip "Toggle the recording state of a performance recording."> > >> <!ENTITY performanceUI.options.gear.tooltiptext "Configure performance preferences."> > >> etc. > > Once again, please make it a habit *not* to use trailing periods in > tooltips, unless when instructing a user or using full sentences, which is > very, very rare (such as performanceUI.bufferStatusTooltip.) Descriptions or > actions should NOT use them, as they generally use infinitive mood, while > adding them results in imperative mood, making users think they are ordered > to do something, both in English and for any other locale where different > verbs may be needed. As imperative and infinitive mood look similar in > English, erroneously placed periods make the problem hard to recognize for > non-English, but even more vital for English users for the reason above. > > As a localizer I keep running into improper (and inconsistent) use of end > stops in tooltips on en-US devtools files on almost any FF version. I’ve > commented on this issue 3 times before and even had to file a bug once to > have some corrected, while people keep making this mistake. Now I might be > wrong, but think localizers shouldn’t need to worry about, nor spend time on > finding out whether or not infinitive or imperative mood is meant in > strings, nor remind developers about proper use more than once. Anyone > working on devtools files should seriously take this into account just like > any other source file where people do so. Tooltips just shouldn’t use > periods, period. > > If possible (yes, it probably is), please watch/fix this when > entering/moving strings, as well as when doing QA, and always keep this in > mind. Thanks. Ton, These guidelines you are suggesting are probably worth formalizing and adding to places like https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices and https://wiki.mozilla.org/L10n/WorldReady That we often point contributors at to help improve skills that will lead to better and easier localization. Let's figure out how to get some of these items integrated there. Maybe jbeatty can help with that. Jordan, Your probably right that we might always need reminders, but its also good to point contributors to good docs that train in advance of hacking on stuff. Lets try and do more of that too. -chofmann

Adding to the MDN page, with examples, would be great for both contributors and internally! Also was thinking about adding to the l10n tests checking strings named "tooltip" and the last character of the string, but sounds like its dependent on how its used, so not automatable.

IMO that's not L10n, that's English copy guidelines for Firefox (each language should have its own guidelines), and they should be in a different document. And you want to involve Matej in that kind of discussion. That page already says: > If you have doubts about the quality of strings, ask a copywriter to do a copy > review of this text. Ideally, all strings landing in code should originate from > approved UX wireframes, and copy review should be part of the initial stage of > creating these wireframes. And I still think that's the way things should go (maybe because I wrote that…). Developers are generally not a good pick when it comes to design things, or explain them in strings.