toolkit devtool's gcli.jsm contains chrome://browser

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: ewong, Assigned: ewong)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Comment 1

5 years ago
Created attachment 8361521 [details] [diff] [review]
proposed patch (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8361521 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #8361521 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Attachment #8361521 - Flags: review?(neil) → review?(l10n)
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

5 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?
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.

Comment 6

5 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

5 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

5 years ago
Created attachment 8362022 [details] [diff] [review]
proposed patch (v2) - Option 1
Attachment #8361521 - Attachment is obsolete: true
Attachment #8361521 - Flags: review?(l10n)
Attachment #8361521 - Flags: review?(jwalker)
Attachment #8362022 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #8362022 - Flags: review?(l10n)
(Assignee)

Comment 9

5 years ago
Created attachment 8362025 [details] [diff] [review]
proposed patch - Option 2
Attachment #8362025 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #8362025 - Flags: review?(l10n)
(Assignee)

Comment 10

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

Comment 12

5 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

5 years ago
Attachment #8362025 - Flags: review?(l10n) → review-

Comment 13

5 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-
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.