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)

x86
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ewong, Assigned: ewong)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch proposed patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8361521 - Flags: review?(jwalker)
Attachment #8361521 - Flags: review?(neil)
Attachment #8361521 - Flags: review?(neil) → review?(l10n)
(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?
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
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.
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.
(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.
Attachment #8361521 - Attachment is obsolete: true
Attachment #8361521 - Flags: review?(l10n)
Attachment #8361521 - Flags: review?(jwalker)
Attachment #8362022 - Flags: review?(jwalker)
Attachment #8362022 - Flags: review?(l10n)
Attachment #8362025 - Flags: review?(jwalker)
Attachment #8362025 - Flags: review?(l10n)
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 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)
Attachment #8362025 - Flags: review?(jwalker)
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
Attachment #8362025 - Flags: review?(l10n) → review-
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-
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: