Closed
Bug 960890
Opened 10 years ago
Closed 8 years ago
toolkit devtool's gcli.jsm contains chrome://browser
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ewong, Assigned: ewong)
Details
Attachments
(2 files, 1 obsolete file)
6.11 KB,
patch
|
Pike
:
review-
|
Details | Diff | Splinter Review |
89.71 KB,
patch
|
Pike
:
review-
|
Details | Diff | Splinter Review |
In http://hg.mozilla.org/mozilla-central/file/9bcc52594322/toolkit/devtools/gcli/gcli.jsm#l131, afaik, it should be global/locale/devtools/.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8361521 -
Flags: review?(neil)
Assignee | ||
Updated•10 years ago
|
Attachment #8361521 -
Flags: review?(neil) → review?(l10n)
Comment 2•10 years ago
|
||
I don't understand how this could work with the files we have http://hg.mozilla.org/mozilla-central/file/9bcc52594322/toolkit/locales/en-US/chrome/global/devtools
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #2) > I don't understand how this could work with the files we have > http://hg.mozilla.org/mozilla-central/file/9bcc52594322/toolkit/locales/en- > US/chrome/global/devtools Sorry, I don't understand what you mean. Do you mean that gclicommands.properties shouldn't be in chrome/global/devtools?
Comment 4•10 years ago
|
||
Sorry, I wasn't smart and used the "review" link (doesn't show hg moves)
> rename from browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> rename to toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
Comment 5•10 years ago
|
||
Our plan is to move all of devtools from /browser/devtools and /toolkit/devtools to /devtools, because there isn't any logic to how it's laid out right now (the files would build as if they were part of toolkit) Hence we've generally only been moving things to /toolkit/devtools when they were actually broken, to save 2 moves. I'm guessing this is causing breakage for you. If you have breakage, then lets fix it.
Comment 6•10 years ago
|
||
I'd like to understand what problem we're solving, and how severe it is. Moving files is hard for l10n, as systems like pootle or transifex actually don't work on files, nor constructively work with version control. For many, this is throwing their old work away and starting from scratch, we should have really good reasons for doing so.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #6) > I'd like to understand what problem we're solving, and how severe it is. > > Moving files is hard for l10n, as systems like pootle or transifex actually > don't work on files, nor constructively work with version control. For many, > this is throwing their old work away and starting from scratch, we should > have really good reasons for doing so. FWIW, I think gcli.properties also needs moving. I'm porting devtools toolbox to SeaMonkey and came across an error in the error console: No chrome package registered for chrome://browser/locale/devtools/gcli.properties (Note: In my search for gcli.properties, I got sidetracked and found gclicommands.properties and completely forgot about gcli.properties.) So in toolkit/devtools/gcli/gcli.jsm, line #131: 130 var stringBundle = Services.strings.createBundle( 131 'chrome://browser/locale/devtools/gclicommands.properties'); line #3260: 3259 XPCOMUtils.defineLazyGetter(imports, 'stringBundle', function () { 3260 return Services.strings.createBundle('chrome://browser/locale/devtools/gcli.properties'); This is in toolkit code. It's my understanding that in Toolkit code, there shouldn't be any references to browser/ chrome. Therefore this patch( which I'll update for gcli.properties as well.) will solve this reference issue. Is it severe? Not really. But if jwalker said that gcli will be moving directly to toolkit, then having browser/ chrome references in toolkit code isn't correct (I would think; but, as I'm not a toolkit peer, I'd appreciate being corrected.) If 'moving' files is such a pain for l10n, what if I just basically create new files in toolkit? i.e. a new gcli.properties and gclicommands.properties? I'm open for suggestions.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8361521 -
Attachment is obsolete: true
Attachment #8361521 -
Flags: review?(l10n)
Attachment #8361521 -
Flags: review?(jwalker)
Attachment #8362022 -
Flags: review?(jwalker)
Assignee | ||
Updated•10 years ago
|
Attachment #8362022 -
Flags: review?(l10n)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8362025 -
Flags: review?(jwalker)
Assignee | ||
Updated•10 years ago
|
Attachment #8362025 -
Flags: review?(l10n)
Assignee | ||
Comment 10•10 years ago
|
||
Fwiw, I get a whole bunch of errors in the error console (aside for the chrome error). i.e. Timestamp: 1/18/2014 11:03:24 AM Error: Failed to lookup ,canonDefaultGroupName,[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIStringBundle.GetStringFromName]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource://gre/modules/devtools/gcli.jsm :: exports.lookup :: line 3299" data: no] Source File: resource://gre/modules/devtools/gcli.jsm Line: 3302 Anyway, I've put up two patches. 1) original patch. 2) Adding just the gcli.properties and the gclicommands.properties and leaving the browser/ ones alone.
Comment 11•10 years ago
|
||
Comment on attachment 8362022 [details] [diff] [review] proposed patch (v2) - Option 1 I'm going to cancel my r? because I think Axel should have the call here. I'll fit in with whatever you think best.
Attachment #8362022 -
Flags: review?(jwalker)
Updated•10 years ago
|
Attachment #8362025 -
Flags: review?(jwalker)
Comment 12•10 years ago
|
||
I think something like this should work relativesrcdir browser/locales: locale/@AB_CD@/suite/overrides/devtools/gclicommands.properties (%chrome/browser/devtools/gclicommands.properties) % override chrome://browser/locale/devtools/gclicommands.properties chrome://suite/locale/override/devtools/gclicommands.properties
Updated•10 years ago
|
Attachment #8362025 -
Flags: review?(l10n) → review-
Comment 13•10 years ago
|
||
Comment on attachment 8362022 [details] [diff] [review] proposed patch (v2) - Option 1 Review of attachment 8362022 [details] [diff] [review]: ----------------------------------------------------------------- I don't like either of those patches, and realized in unrelated discussions that we can work around this. Creating overlays for both browser locale files from suite/locales/jar.mn with the relativepathdir trick should work, see my previous comment.
Attachment #8362022 -
Flags: review?(l10n) → review-
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•