687851 - Create a locales directory in the devtools module, move all devtools l10n strings to it

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

Most of our strings will be optional, and we would like to be able to review them ourselves

Comment 1

[Axel Hecht [:Pike]]

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]]

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]]

(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]]

(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]]

Sure, that works then. Subfolders it is.

Comment 6

[Joe Walker [:jwalker]]

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]]

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: [e-mail]]]

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]]

(In reply to Gavin Sharp (use [e-mail] 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)]

I prefer less nesting as a rule. ~30 files doesn't seem like too many to me, really.

Comment 11

[Joe Walker [:jwalker]]

OK, Ok, ok. Clearly outvoted. We'll go flatter.
Joe.

Comment 12

[Joe Walker [:jwalker]]

Attachment: 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?)]

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]]

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]]

(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)]

(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)]

Comment on attachment 566522 [details] [diff] [review]
upload 1

looks good.

Comment 18

[Rob Campbell [:rc] (:robcee)]

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]]

Attachment: 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]]

I'm a bit lost, I'm afraid. What's this review request about in detail?

Comment 21

[Joe Walker [:jwalker]]

(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?)]

Comment on attachment 568015 [details] [diff] [review]
version 2

OK. This will keep Fennec working. Thanks!

Comment 23

[Axel Hecht [:Pike]]

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]]

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)]

(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]]

Attachment: upload 3

Comment 27

[Joe Walker [:jwalker]]

I pushed to try just now, but there appears to be some bustage there.

Comment 28

[Rob Campbell [:rc] (:robcee)]

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)]

https://hg.mozilla.org/mozilla-central/rev/2541a2e898df

Comment 30

[Francesco Lodolo [:flod]]

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]]

Another thing (nitpicking): webconsole.properties vs webConsole.dtd

Comment 32

[Rob Campbell [:rc] (:robcee)]

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.