Last Comment Bug 687851 - Create a locales directory in the devtools module, move all devtools l10n strings to it
: Create a locales directory in the devtools module, move all devtools l10n str...
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks: 699968 703275
  Show dependency treegraph
 
Reported: 2011-09-20 08:24 PDT by David Dahl :ddahl
Modified: 2011-11-17 08:42 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (11.92 KB, patch)
2011-10-12 07:13 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
rcampbell: review+
mark.finkle: review-
l10n: review+
gavin.sharp: feedback+
Details | Diff | Review
version 2 (18.51 KB, patch)
2011-10-19 05:39 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mark.finkle: review+
l10n: review+
Details | Diff | Review
upload 3 (18.50 KB, patch)
2011-10-26 09:43 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review

Description David Dahl :ddahl 2011-09-20 08:24:52 PDT
Most of our strings will be optional, and we would like to be able to review them ourselves
Comment 1 Axel Hecht [:Pike] 2011-10-06 04:40:04 PDT
Please don't put files into browser/devtools/locales/en-US or something. Just use a folder within the browser/locales/en-US/ hierarchy?

In the l10n sources, the /locales/en-US/ part is dropped, so if you add stuff within browser/something, the en-US location becomes ambiguous from an l10n point of view.

I'd suggest to go for browser/locales/en-US/chrome/devtools or ...chrome/browser/devtools.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-06 10:37:39 PDT
In browser/devtools there is a hierachy for each tool. I suggest we replicate that inside browser/locales/en-US/chrome/devtools.
Comment 3 Mihai Sucan [:msucan] 2011-10-06 10:40:58 PDT
(In reply to Joe Walker from comment #2)
> In browser/devtools there is a hierachy for each tool. I suggest we
> replicate that inside browser/locales/en-US/chrome/devtools.

We generally have a dtd and .properties file for each dev tool. Not sure there's value in having subfolders for each dev tool. Wouldn't it be sufficient to just put our dev tools dtd and properties files inside a single browser/locales/en-US/devtools folder?
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-06 11:03:44 PDT
(In reply to Mihai Sucan [:msucan] from comment #3)
> (In reply to Joe Walker from comment #2)
> > In browser/devtools there is a hierachy for each tool. I suggest we
> > replicate that inside browser/locales/en-US/chrome/devtools.
> 
> We generally have a dtd and .properties file for each dev tool. Not sure
> there's value in having subfolders for each dev tool. Wouldn't it be
> sufficient to just put our dev tools dtd and properties files inside a
> single browser/locales/en-US/devtools folder?

webconsole has several already, but I'm not fussed. IMHO it would slow navigation down if we got more than 20/30 files in the one directory.
Comment 5 Mihai Sucan [:msucan] 2011-10-06 11:05:30 PDT
Sure, that works then. Subfolders it is.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-06 13:54:34 PDT
Axel, in bug 656666 comment 14, you pointed out the cost in moving a strings file. Am I right in thinking that either we're doing this soon enough, or that the pain is worth it - i.e. that we should carry on ...
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 05:41:23 PDT
We're currently working on a patch to use:

    /browser/locales/en-US/chrome/browser/devtools/PROJECT

Where PROJECT is one of highlighter, scratchpad, shared, sourceeditor, styleinspector, webconsole, etc.

I think everyone is happy with this. Please shout ASAP if not.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-07 16:02:38 PDT
The extra nesting of a directory-per-tool seems overkill. Won't most of them have only one or two files? Directories with 20-30 files (are there even that many now?) isn't particularly troublesome.
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 23:58:52 PDT
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8)
> The extra nesting of a directory-per-tool seems overkill. Won't most of them
> have only one or two files? Directories with 20-30 files (are there even
> that many now?) isn't particularly troublesome.

It's hard to know how many each will have - Perhaps 2 would be the norm, but it depends on how we split our tools up. I would hope that we grow our tools rather than adding many new ones, which indicates more than 2 files. webconsole is already at least 4 (probably more but it's hard to tell from a quick scan)

Whether 30 files is troublesome depends on your definition of troublesome I guess. Obviously is no problems with directory size or anything, but it is possibly slower to find what you're looking for.

I'm not particularly fussed however.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-10-11 11:54:33 PDT
I prefer less nesting as a rule. ~30 files doesn't seem like too many to me, really.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-11 14:44:51 PDT
OK, Ok, ok. Clearly outvoted. We'll go flatter.
Joe.
Comment 12 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-12 07:13:55 PDT
Created attachment 566522 [details] [diff] [review]
upload 1


This patch moves all devtools strings to /browser/locales/en-US/chome/browser/devtools

It includes change to /mobile/chrome/content/content.js since it refers to headsUpDisplay.properties
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-12 07:21:44 PDT
Comment on attachment 566522 [details] [diff] [review]
upload 1

Firefox Mobile can't access anything in /browser, since that code is not built for Firefox Mobile. We depend on toolkit code for "shared" resources.

Since we are only using a single string, let's just move the string to a /mobile properties file.
Comment 14 Axel Hecht [:Pike] 2011-10-12 08:14:16 PDT
Comment on attachment 566522 [details] [diff] [review]
upload 1

I really wish we had done this earlier, but in the long term, it's going to pay off.

We'll need noise around this, and mentors to help out, to deal with the fallout on the l10n community.
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-12 08:23:38 PDT
(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 566522 [details] [diff] [review] [diff] [details] [review]
> upload 1
> 
> Firefox Mobile can't access anything in /browser, since that code is not
> built for Firefox Mobile. We depend on toolkit code for "shared" resources.
> 
> Since we are only using a single string, let's just move the string to a
> /mobile properties file.

Can you recommend one to use? There are no obvious candidates.
My best guess is to leave a stripped out headsUpDisplay.properties in place containing just the 2 strings in question.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-10-12 12:08:25 PDT
(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 566522 [details] [diff] [review] [diff] [details] [review]
> upload 1
> 
> Firefox Mobile can't access anything in /browser, since that code is not
> built for Firefox Mobile. We depend on toolkit code for "shared" resources.
> 
> Since we are only using a single string, let's just move the string to a
> /mobile properties file.

I'd recommend moving the string you need to /mobile rather than blocking this bug with an r-.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-10-12 12:10:35 PDT
Comment on attachment 566522 [details] [diff] [review]
upload 1

looks good.
Comment 18 Rob Campbell [:rc] (:robcee) 2011-10-12 12:12:31 PDT
PS, Mark, can you file a bug on your string migration and mark that is blocking this bug? We won't land until your move's complete. Hoping to get this landed by next week at the latest.
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-19 05:39:56 PDT
Created attachment 568015 [details] [diff] [review]
version 2


Mark: this implements my idea from comment 15 to keep the properties file in place stripped of the properties that you're not using.

I've r?ed Axel again because this changes the proposition.

We're keen to get one of these landed soon, but don't really care which way we do it.
Thanks.
Comment 20 Axel Hecht [:Pike] 2011-10-20 15:42:46 PDT
I'm a bit lost, I'm afraid. What's this review request about in detail?
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-20 16:11:43 PDT
(In reply to Axel Hecht [:Pike] from comment #20)
> I'm a bit lost, I'm afraid. What's this review request about in detail?

It's about moving all devtools l10n strings to the same place. You've r+ed it once before (see comment 14). That patch moved all the files to the 'right' place, however some time ago mobile began to use devtools strings, so this patch would have broken mobile.

There were 2 options:
1. in a separate bug, mobile creates their own new strings file
2. we copy rather than move, and then strip out all but the 2 strings mobile are using from the original

This is a busy week for mobile, so in an attempt to get things moving, I did 2.

I r?ed you because this changes the way we do things.
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-21 07:01:43 PDT
Comment on attachment 568015 [details] [diff] [review]
version 2

OK. This will keep Fennec working. Thanks!
Comment 23 Axel Hecht [:Pike] 2011-10-21 11:30:48 PDT
Comment on attachment 568015 [details] [diff] [review]
version 2

Looks good to me.

When we communicate about this, we should have a script that does the file moves/copies for an l10n repo in hand, too.

The change in this patch is basically that we just want to copy headsUpDisplay.properties for l10n, too.

We won't need to bother about removing the obsolete strings, that's something the localizers can probably safely do themselves.
Comment 24 Dão Gottwald [:dao] 2011-10-22 02:49:43 PDT
Comment on attachment 568015 [details] [diff] [review]
version 2

>copy from toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
>copy to browser/locales/en-US/chrome/browser/devtools/headsUpDisplay.properties

Can you rename this file to webconsole.properties while you're at it?
Comment 25 Rob Campbell [:rc] (:robcee) 2011-10-22 06:24:00 PDT
(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 568015 [details] [diff] [review] [diff] [details] [review]
> version 2
> 
> >copy from toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
> >copy to browser/locales/en-US/chrome/browser/devtools/headsUpDisplay.properties
> 
> Can you rename this file to webconsole.properties while you're at it?

good idea.
Comment 26 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-26 09:43:00 PDT
Created attachment 569702 [details] [diff] [review]
upload 3
Comment 27 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-26 10:07:42 PDT
I pushed to try just now, but there appears to be some bustage there.
Comment 28 Rob Campbell [:rc] (:robcee) 2011-10-27 07:30:56 PDT
https://hg.mozilla.org/integration/fx-team/rev/5d250b1cf465

hopefully the bustage mentioned in Comment 27 is of the "try server is busted" variety.
Comment 29 Rob Campbell [:rc] (:robcee) 2011-10-27 13:25:59 PDT
https://hg.mozilla.org/mozilla-central/rev/2541a2e898df
Comment 30 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2011-10-28 10:08:26 PDT
I'm trying to fix my locale after this (painful), but there's something I don't understand.

inspector.properties was last seen here
http://hg.mozilla.org/mozilla-central/file/cd6ed3106521/browser/locales/en-US/chrome/browser/inspector.properties

When moved to /devtools a new string appear (breadcrumbs.siblings)
http://hg.mozilla.org/mozilla-central/file/e43a54b52b06/browser/locales/en-US/chrome/browser/devtools/inspector.properties#l10

I'm trying to understand in which bug this string was introduced (hg logs don't help).
Comment 31 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2011-10-28 10:31:46 PDT
Another thing (nitpicking): webconsole.properties vs webConsole.dtd
Comment 32 Rob Campbell [:rc] (:robcee) 2011-10-31 05:54:30 PDT
flod, breadcrumbs.siblings landed with bug 672006. They ended up in the same changeset together so I can see why that'd be confusing.

Please file bugs if you find anything odd, annoying or painful! thanks.

Note You need to log in before you can comment on or make changes to this bug.