Closed Bug 1011464 Opened 7 years ago Closed 7 years ago

[appmgr v2] get l10n right

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 2 obsolete files)

So we have landed code here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/

It is not compiled yet, and before doing so, I'd like to make sure we're getting l10n right. Locales code is not hosted in /browser/locales/ because we want this tool to be able to live outside of Firefox at some point.

Can someone from l10n could look at this and help me make sure we're doing the right thing here?
Blocks: build-am2
'webide' sounds like a huge pile of strings, there are currently only a few. I'm curious what the final scope of the project is going to be.

Technically, the placing of the files should work. I'd really like to leave those file out of localization until we're switching the feature on by default, or only shortly before that. In particular if the UI you have right now is going through some churn still, we don't need to do stuff like key changes until we officially start the l10n process.
(In reply to Axel Hecht [:Pike] from comment #1)
> 'webide' sounds like a huge pile of strings, there are currently only a few.
> I'm curious what the final scope of the project is going to be.

The goal is to replace the App Manager. See some screenshots there:
https://bugzilla.mozilla.org/show_bug.cgi?id=1007371#c3

> Technically, the placing of the files should work.

Apparently, it does.

> I'd really like to leave
> those file out of localization until we're switching the feature on by
> default, or only shortly before that. In particular if the UI you have right
> now is going through some churn still, we don't need to do stuff like key
> changes until we officially start the l10n process.

How do I do that without removing the .dtd and .properties?
Oh, a test fail on Linux:

KeyError: 'webide'
make[3]: *** [repackage-zip] Error 1
make[2]: *** [repackage-zip-x-test] Error 2
make[1]: *** [l10n-check] Error 2
make: *** [l10n-check] Error 2

Any idea of what I'm doing wrong?
I think the best way to do that is to make the jar.mn and the chrome files register and reference chrome://content instead of chrome://locale, and to make sure there's no l10n.ini in which browser/devtools/webide is picked up.

I wish our packager wasn't so picky in this situation, or at least explained why it's failing.
Attached patch don't use chrome://locales (obsolete) — Splinter Review
Ok. First step, we move .dtd and .properties in content. We'll revert that once the project is stable enough.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8423942 - Flags: review?(l10n)
Blocks: 1010387
Comment on attachment 8423942 [details] [diff] [review]
don't use chrome://locales

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

Yep, that looks good, thanks.
Attachment #8423942 - Flags: review?(l10n) → review+
Keywords: checkin-needed
Setting leave-open, because I think that's what we want. I'm expecting follow-ups to actually switch stuff on when it's time, and that's fine to do in this bug. Paul, agree?
Keywords: leave-open
(In reply to Axel Hecht [:Pike] from comment #8)
> Setting leave-open, because I think that's what we want. I'm expecting
> follow-ups to actually switch stuff on when it's time, and that's fine to do
> in this bug. Paul, agree?

Yes.
No longer blocks: 1010387
Blocks: enable-webide
No longer blocks: build-am2
Depends on: 1028079
Attached patch v1 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=da3a18dbacbb
Attachment #8423942 - Attachment is obsolete: true
Could someone help me understand/reproduce this failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=44965007&tree=Try
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
I'm far from familiar with the build system, but why are you keeping l10n files in /webide/locales?
Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?
Try again with this patch, and that will make the error message a bit more explicit about what's wrong:

diff --git a/python/mozbuild/mozpack/packager/l10n.py b/python/mozbuild/mozpack/packager/l10n.py
--- a/python/mozbuild/mozpack/packager/l10n.py
+++ b/python/mozbuild/mozpack/packager/l10n.py
@@ -73,20 +73,26 @@ def _repack(app_finder, l10n_finder, cop
     for e in l10n.entries:
         if isinstance(e, ManifestChrome):
             base = mozpack.path.basedir(e.path, app.bases)
             l10n_paths.setdefault(base, {})
             l10n_paths[base][e.name] = e.path
 
     # For chrome and non chrome files or directories, store what langpack path
     # corresponds to a package path.
-    paths = dict((e.path,
-                  l10n_paths[mozpack.path.basedir(e.path, app.bases)][e.name])
-                 for e in app.entries
-                 if isinstance(e, ManifestEntryWithRelPath))
+    paths = {}
+    for e in app.entries:
+        if instance(e, ManifestEntryWithRelPath):
+            base = mozpack.path.basedir(e.path, app.bases)
+            if base not in l10n_paths:
+                errors.fatal("Locale doesn't contain %s/" % base)
+            if e.name not in l10n_paths[base]:
+                errors.fatal("Locale doesn't have a manifest entry for '%s'" %
+                    e.name)
+            path[e.path] = l10n_paths[base][e.name]
 
     for pattern in non_chrome:
         for base in app.bases:
             path = mozpack.path.join(base, pattern)
             left = set(p for p, f in app_finder.find(path))
             right = set(p for p, f in l10n_finder.find(path))
             for p in right:
                 paths[p] = p
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
(In reply to Francesco Lodolo [:flod] from comment #13)
> I'm far from familiar with the build system, but why are you keeping l10n
> files in /webide/locales?
> Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?

We're trying to keep WebIDE's code selfcontained, for maybe one day extract it as an addon or a xulrunner app.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #15)
> (In reply to Francesco Lodolo [:flod] from comment #13)
> > I'm far from familiar with the build system, but why are you keeping l10n
> > files in /webide/locales?
> > Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?
> 
> We're trying to keep WebIDE's code selfcontained, for maybe one day extract
> it as an addon or a xulrunner app.

Maybe it's not that simple. If not, I'll just move code into browser/locales
Note, we're missing a fragment in browser/locales/l10n.ini to pick up browser/devtools/webide.

I'm not too exited that we're having overlays of overlays of overlays in the browser file tree, tbh. It's just horrible to find the en-US files at this point. I'd really prefer to have devtools files in one spot beneath browser/devtools/locales/en-US

No idea how the packaging side works, though.
Attached patch v2Splinter Review
I'm not confident enough about how locales work to bypass the usual locales build system. And the value of isolating locales in /webide/ is almost null at this time. Let's go for the regular way.
Attachment #8465343 - Attachment is obsolete: true
Attachment #8466287 - Flags: review?(jryans)
(In reply to Axel Hecht [:Pike] from comment #18)
> Note, we're missing a fragment in browser/locales/l10n.ini to pick up
> browser/devtools/webide.
> 
> I'm not too exited that we're having overlays of overlays of overlays in the
> browser file tree, tbh. It's just horrible to find the en-US files at this
> point. I'd really prefer to have devtools files in one spot beneath
> browser/devtools/locales/en-US
> 
> No idea how the packaging side works, though.

So for now, I'll move webide files into /browser/locales/. But we could file a bug about moving locales in /browser/devtools/.
Pike: bug 1047499
Comment on attachment 8466287 [details] [diff] [review]
v2

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

I realized I haven't looked at the strings in a while. Some minor nits since you're going to expose them for localization (not sure if you want to that here on in another bug).

::: browser/devtools/webide/locales/en-US/webide.dtd
@@ -62,5 @@
> -
> -<!ENTITY projectPanel_myProjects "My Projects">
> -<!ENTITY projectPanel_runtimeApps "Runtime Apps">
> -<!ENTITY runtimePanel_USBDevices "USB Devices">
> -<!ENTITY runtimePanel_WiFiDevices "WiFi Devices">

Wi-Fi, not WiFi

::: browser/devtools/webide/locales/en-US/webide.properties
@@ -21,5 @@
> -
> -notification_showTroubleShooting_label=troubleshooting
> -notification_showTroubleShooting_accesskey=t
> -
> -error_operationTimeout=Operation timed out: %1$S

Can you add localization comments on variables (error_* strings)?

@@ -31,5 @@
> -error_cantFetchAddonsJSON=Can't fetch the add-on list: %S
> -
> -addons_stable=stable
> -addons_unstable=unstable
> -addons_simulator_label=Firefox OS %1$S Simulator (%2$S)

Same here: no clue what these variables are without a comment.
Comment on attachment 8466287 [details] [diff] [review]
v2

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

Seems good to me.  Up to you if you'd like to address flod's comments here or in a follow-up.
Attachment #8466287 - Flags: review?(jryans) → review+
Thank you Flod. I addressed your comments.

https://hg.mozilla.org/integration/fx-team/rev/9a3b88192df4
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9a3b88192df4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.