Closed Bug 1386616 Opened 7 years ago Closed 7 years ago

Move devtools key-shortcuts.properties out of the devtools addon

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1386616 - dt-addon: move key-shortcuts.properties to devtools/shim/locales;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1386616 - dt-addon: create new devtools/shim locale;
59 bytes, text/x-review-board-request
Pike
: review+
Details 
devtools-startup should be moved outside of devtools/client in prevision of devtools moving to an addon.

Since devtools-startup is supposed to work without the devtools addon installed, its dependency on key-shortcuts.properties should also move out of devtools/client.

An option is to move it to devtools/shim and create a new locale path that will remain available even if Firefox is built without DevTools.
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Francesco: we plan to move http://searchfox.org/mozilla-central/source/devtools/client/locales/en-US/key-shortcuts.properties to a different folder (no content change though). 

What is the best way to handle that for localization teams?
Flags: needinfo?(francesco.lodolo)
Where would the new folder be? Also, what's the timing?

Since we expose a (delayed) clone of mozilla-central to localization tools, we could move the file in the l10n repositories via script right before merging this changeset.

This is going to work fine for locales working on Poontoon, might not go so smoothly for locales working on Pootle. We're moving all remaining locales away from the latter at the end of the month, hence the question about timing.

It's also a relatively small file, so it wouldn't be the end of the world if it works only for most locales.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #3)
> Where would the new folder be?

New folder is `devtools/shim/locales/en-US` which introduces a new locale path "devtools-shim"

> Also, what's the timing?

I can't say for sure at the moment, but this might block another bug (Bug 1369801). If that's confirmed I would like to land it as early as possible. 

> 
> Since we expose a (delayed) clone of mozilla-central to localization tools,
> we could move the file in the l10n repositories via script right before
> merging this changeset.
> 

In this case the script seems trivial to write, but do you have one available already, or do we need to provide something?
(In reply to Julian Descottes [:jdescottes] from comment #4)
> (In reply to Francesco Lodolo [:flod] from comment #3)
> > Where would the new folder be?
> 
> New folder is `devtools/shim/locales/en-US` which introduces a new locale
> path "devtools-shim"

Adding the file itself is not going to expose it for localization. You need to have an l10n.ini and l10n.toml
https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/locales/l10n.ini
https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/locales/l10n.toml

:pike is going to be your best choice to review such a patch.


> I can't say for sure at the moment, but this might block another bug (Bug
> 1369801). If that's confirmed I would like to land it as early as possible. 

That's OK.

> In this case the script seems trivial to write, but do you have one
> available already, or do we need to provide something?

I probably have something around to reuse. If not, I still consider this on me to fix.
(In reply to Francesco Lodolo [:flod] from comment #5)
> (In reply to Julian Descottes [:jdescottes] from comment #4)
> > (In reply to Francesco Lodolo [:flod] from comment #3)
> > > Where would the new folder be?
> > 
> > New folder is `devtools/shim/locales/en-US` which introduces a new locale
> > path "devtools-shim"
> 
> Adding the file itself is not going to expose it for localization. You need
> to have an l10n.ini and l10n.toml
> https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/locales/l10n.
> ini
> https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/locales/l10n.
> toml
> 
> :pike is going to be your best choice to review such a patch.
> 

Thanks for the info! I already added the toml file in my current WIP patch, but I skipped the l10n.ini because I didn't see any for our devtools-shared locale (http://searchfox.org/mozilla-central/source/devtools/shared/locales). Do you think we should add one?
Comment on attachment 8893445 [details]
Bug 1386616 - dt-addon: create new devtools/shim locale;

https://reviewboard.mozilla.org/r/164534/#review170200

::: browser/locales/l10n.toml:141
(Diff revision 1)
>  
>  [[includes]]
>      path = "devtools/client/locales/l10n.toml"
>  
> +[[includes]]
> +    path = "devtools/shim/locales/l10n.toml"

You'll need to do this for both the l10n.toml and the l10n.ini file.

We'll convert from the ini to the toml files with or shortly after bug 1382005.

Btw, if you rebase onto m-c once more, I significantly simplified doing local l10n repacks yesterday, docs are at http://gecko.readthedocs.io/en/latest/build/buildsystem/locales.html. You might need to run configure, but then it's a one-liner.
Attachment #8893445 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] from comment #12)
> Comment on attachment 8893445 [details]
> Bug 1386616 - dt-addon: create new devtools/shim locale;
> 
> https://reviewboard.mozilla.org/r/164534/#review170200

Thanks for the review.

> 
> ::: browser/locales/l10n.toml:141
> (Diff revision 1)
> >  
> >  [[includes]]
> >      path = "devtools/client/locales/l10n.toml"
> >  
> > +[[includes]]
> > +    path = "devtools/shim/locales/l10n.toml"
> 
> You'll need to do this for both the l10n.toml and the l10n.ini file.

So do I need to add an entry with path = "devtools/shim/locales/l10n.ini" ? 
Why isn't this done for devtools/client/locales?

> 
> We'll convert from the ini to the toml files with or shortly after bug
> 1382005.
> 
> Btw, if you rebase onto m-c once more, I significantly simplified doing
> local l10n repacks yesterday, docs are at
> http://gecko.readthedocs.io/en/latest/build/buildsystem/locales.html. You
> might need to run configure, but then it's a one-liner.

I'm probably missing a prerequisite, but on the latest mc, after doing build and package, I tried running `./mach build installers-en-US` but it failed with:
> /bin/sh: ../../dist/xpi-stage/locale-en-US/update.locale: No such file or directory

Do I need a special kind of build in the first place?
Flags: needinfo?(l10n)
Per our conversation on IRC, we should localize these strings into all firefox locales, and not just devtools locales.

Thus, let's not actually create separate toml files for this. Just treat it like browser/extensions/onboarding.

(If you're following along on teh installers-fr thread, en-US shouldn't work, and we don't support artifact builds)
Flags: needinfo?(l10n)
Comment on attachment 8893445 [details]
Bug 1386616 - dt-addon: create new devtools/shim locale;

Clearing review request while I wait fro try results
Attachment #8893445 - Flags: review?(l10n)
After doing a regular non-artifact build I was able to do a l10n repack successfully. 

However I can't get the same error as the one I see on the L10n on try:
https://treeherder.mozilla.org/logviewer.html#?job_id=120736822&repo=try

The error on try was: 
> Error: Missing file: ../../dist/xpi-stage/locale-ast/browser/chrome/ast/locale/ast/devtools/client/key-shortcuts.properties

But client/key-shortcuts.properties is precisely the file that moved to devtools/shim, so I don't know why this is complaining here. We'll see if the issue is still present with the new approach suggested by Pike.
While testing another patch I found a weird behavior related to moving key-shortcuts.properties from one locale to the other.
[Note that all this is using artifact builds, I'll test with regular build and will report back when I have the results]

Following the STRs below:
- checkout current mozilla-central
- ./mach clobber && ./mach build && ./mach run -P
- on the profile picker, create a new profile
- try to open devtools with keyboard shortcuts, quit Firefox
- apply the two patches attached to this Bug 
- ./mach clobber && ./mach build && ./mach run -P
- select the profile created previously
- try to open devtools with keyboard shortcuts -> nothing happens

The following errors are logged:
> JavaScript error: 
> file:///Users/jdescottes/Development/hg/fx-team/obj-x86_64-apple-darwin15.6.0/dist/Nightly.app/Contents/Resources/browser/components/devtools-startup.js, 
> line 65: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIStringBundle.GetStringFromName]

The line in question triggers the following getter:

> XPCOMUtils.defineLazyGetter(this, "Bundle", function () {
>   const kUrl = "chrome://devtools-shim/locale/key-shortcuts.properties";
>   return Services.strings.createBundle(kUrl);
> });

The key parts here are to:
- clobber before each build
- keep the same profile

Restarting with the same profile just gives the same result. Restarting with a clean profile works. 

Axel: Does this ring a bell? I'm really surprised by the fact that the issue seems linked to the user profile being used. Any idea how this would impact which locales are available?
Flags: needinfo?(l10n)
No idea. Maybe .... the startup cache? On the mac, ~/Library/Caches/Firefox/Profiles/*.$PROF/startupCache. Could you check by just blowing that away?
Flags: needinfo?(l10n)
(In reply to Axel Hecht [:Pike] from comment #20)
> No idea. Maybe .... the startup cache? On the mac,
> ~/Library/Caches/Firefox/Profiles/*.$PROF/startupCache. Could you check by
> just blowing that away?

Indeed, deleting this folder fixes the issue!
So I was looking into this in more details, and the issue is not really related to localization/properties, but to startup script caching (as Pike suggested!).

Going through the STRs:
1 - checkout current mozilla-central
2 - ./mach clobber && ./mach build && ./mach run -P
3 - on the profile picker, create a new profile
===> here devtools-startup.js is put in the startupCache
4 - try to open devtools with keyboard shortcuts, quit Firefox
5 - apply the two patches attached to this Bug 
6 - ./mach clobber && ./mach build && ./mach run -P
7 - select the profile created previously
8 - try to open devtools with keyboard shortcuts -> nothing happens 
===> instead of using the updated devtools-startup, the cached (and outdated) version will be used here

I will log a separate bug about that, I don't think it's worth blocking this bug on it.
See Also: → 1389502
Comment on attachment 8893445 [details]
Bug 1386616 - dt-addon: create new devtools/shim locale;

https://reviewboard.mozilla.org/r/164534/#review170200

> You'll need to do this for both the l10n.toml and the l10n.ini file.
> 
> We'll convert from the ini to the toml files with or shortly after bug 1382005.
> 
> Btw, if you rebase onto m-c once more, I significantly simplified doing local l10n repacks yesterday, docs are at http://gecko.readthedocs.io/en/latest/build/buildsystem/locales.html. You might need to run configure, but then it's a one-liner.

Applied suggestions from comment 14
Comment on attachment 8893445 [details]
Bug 1386616 - dt-addon: create new devtools/shim locale;

https://reviewboard.mozilla.org/r/164534/#review173402

lgtm.
Attachment #8893445 - Flags: review?(l10n) → review+
Comment on attachment 8892922 [details]
Bug 1386616 - dt-addon: move key-shortcuts.properties to devtools/shim/locales;

https://reviewboard.mozilla.org/r/163944/#review174968

Sorry for the review delay, I was waiting for things to settle on the l10n side to be sure it was worth looking at.
And it looks good to me, thanks!

::: devtools/shared/l10n.js:62
(Diff revision 5)
>      if (/^toolkit/.test(url)) {
>        reqFn = reqGlobal;
>      } else if (/^devtools\/shared/.test(url)) {
>        reqFn = reqShared;
> +    } else if (/^devtools\/shim/.test(url)) {
> +      reqFn = reqShim;

I'm starting to wonder if all that dance into Loader.jsm and these regexp and non-naive path/url manipulation are really worth.
Wouldn't it be better to pass the full absolute URL for the couple of properties coming from firefox (toolkit and shim)?
Like here, pass "chrome://devtools-shim/locale/key-shortcuts.properties" to l10n helper modules?

(that's just a thought, not something to address in this patch)
Attachment #8892922 - Flags: review?(poirot.alex) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bae931220733
dt-addon: create new devtools/shim locale;r=Pike
https://hg.mozilla.org/integration/autoland/rev/bb1ea1b46dad
dt-addon: move key-shortcuts.properties to devtools/shim/locales;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/bae931220733
https://hg.mozilla.org/mozilla-central/rev/bb1ea1b46dad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: