Closed Bug 1377911 Opened 2 years ago Closed 2 years ago

Remove the override chrome entries from language manifests

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files)

As per bug 1365709 comment 12 we have three overrides when we generate a langpack for desktop:


    "override chrome://global/locale/appstrings.properties chrome://browser/locale/appstrings.properties",
    "override chrome://global/locale/netError.dtd chrome://browser/locale/netError.dtd",
    "override chrome://mozapps/locale/downloads/settingsChange.dtd chrome://browser/locale/downloads/settingsChange.dtd",

I'd like to understand if there's any reason for those three overrides to exist and if so, if they have to be in language resources (since they don't differ between languages).

I'm not sure how to approach it - I'd like to try to NI several people who might have an idea on why are they there and consider trying removing them in this cycle.

The overrides are located in http://searchfox.org/mozilla-central/source/browser/locales/jar.mn#122

and were added in bug 411037 and bug 486826.

I'm not sure if I understand the logic and the rationale to add those overrides to /browser/locales/jar.mn instead of browser/content/jar.mn.
:Pike - any idea why this happens, what can we do to move it away from locales, and who else to NI?
Blocks: 1365031
Flags: needinfo?(l10n)
Since the bugs that added it were in Firefox::Build Config, I'm moving it to Core::Build Config.
Component: Internationalization → Build Config
No idea. Best bet is to try, and to write a test, I guess.
Flags: needinfo?(l10n)
NI'ing more people that I found through hg log on the overriden files.

Patrick, Masatoshi, Myk - you showed up in the log for those files. Can you help us asses the situation? Are those overrides still needed? If yes, can we move them out of locale manifests? Anyone else who might know? (for example, if there's a value in keeping netError strings separate between products and separate for toolkit?)
Flags: needinfo?(myk)
Flags: needinfo?(mcmanus)
Flags: needinfo?(VYV03354)
It seems that the reason we have those files are as follows:

1) appstrings.properties - we want to use `Firefox` name and there's no easy way to reference brandShortName from .properties file. 
But there are also a content differences:

* dom/ and mobile/ versions have `weakCryptoUsed` entity. browser/ doesn't.

2) netError.dtd is so completely different between the three products that I'm not sure if it should even be the same file anymore

3) settingsChange.dtd has only version for browser/ and toolkit/ and the browser/ one has a small string difference in both of it's strings.

Browser: `Settings can be changed using the Applications tab in &brandShortName;'s Preferences`
Toolkit: `Settings can be changed in &brandShortName;'s Preferences.`

=================

My *guess* is that (3) is not relevant anymore and we could just merge it back and remove the override.
The other two should probably remain and we could move them to browser/base/jar.mn but it would be a bit awkward as the entry for /locale/ would be in /base/.

My understanding is that since the override is product based, not locale based, that should be ok, but would like to run it via Pike.
Pike, would you be ok with moving those three overrides to browser/base/jar.mn and mobile/android/chrome/jar.mn so that we don't have them popping up in per-locale manifests?
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> 3) settingsChange.dtd has only version for browser/ and toolkit/ and the
> browser/ one has a small string difference in both of it's strings.
> 
> Browser: `Settings can be changed using the Applications tab in
> &brandShortName;'s Preferences`
> Toolkit: `Settings can be changed in &brandShortName;'s Preferences.`
> 
> =================
> 
> My *guess* is that (3) is not relevant anymore and we could just merge it
> back and remove the override.

I agree.

I added the settingsChange.dtd override in bug 395317 via <https://github.com/mozilla/gecko/commit/047aefc6b363705f697e409445d55cfe84ac81d1> back in 2007 after a redesign of the Applications preferences that put them in a different location than other toolkit/-based apps.

They're still in a different location than other toolkit/-based apps, but Thunderbird also overrides the strings, per <https://hg.mozilla.org/comm-central/file/081f746eb2e8/mail/locales/en-US/chrome/overrides/settingsChange.dtd>, so merging the change back to toolkit/ wouldn't affect that application.  And other toolkit/-based apps could do the same.

Note that the current Preferences redesign has renamed Applications to Files & Applications, and documentation like <https://support.mozilla.org/en-US/kb/applications-panel-set-how-firefox-handles-files> calls the sections of the Preferences interface "panels" rather than "tabs".

So the string should now read `Settings can be changed using the Files & Applications panel in &brandShortName;'s Preferences`.
Flags: needinfo?(myk)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> Pike, would you be ok with moving those three overrides to
> browser/base/jar.mn and mobile/android/chrome/jar.mn so that we don't have
> them popping up in per-locale manifests?

I wouldn't mind, with good comments.

That was basically what I had in mind when I said "try it out, and write some tests". Sorry for not saying so explicitly.
Flags: needinfo?(l10n)
... and to be explicit, I'm confident that we need chrome overlays for the existing infrastructure.

And possibly overloading paths in chrome registry with app and platform registration locations.
beyond part 2 of comment 5 I don't have any recollection. sorry.
Flags: needinfo?(mcmanus)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8893649 [details]
Bug 1377911 - Do not strip localized override entries when repackaging.

https://reviewboard.mozilla.org/r/164748/#review170204

It's not like I dislike the idea to move the override definitions into content, but this won't work.

If we can't do something better for settingsChange.dtd, I'd let it live as is. We'll remove them all at some point later in life.

::: mobile/android/chrome/jar.mn:72
(Diff revision 1)
>  % override chrome://global/content/netError.xhtml chrome://browser/content/netError.xhtml
>  % override chrome://mozapps/content/extensions/extensions.xul chrome://browser/content/aboutAddons.xhtml
> +
> +# L10n resource overrides.
> +% override chrome://global/locale/appstrings.properties chrome://browser/locale/appstrings.properties 
> +% override chrome://global/locale/netError.dtd chrome://browser/locale/netError.dtd 

probably copy-n-paste, but trailing whitespace ;-)

::: toolkit/locales/en-US/chrome/mozapps/downloads/settingsChange.dtd:6
(Diff revision 1)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> -<!ENTITY  settingsChangePreferences.label  "Settings can be changed in &brandShortName;'s Preferences.">
> -<!ENTITY  settingsChangeOptions.label      "Settings can be changed in &brandShortName;'s Options.">
> +<!ENTITY  settingsChangePreferences.label2  "Settings can be changed using the Files & Applications panel in &brandShortName;'s Preferences.">
> +<!ENTITY  settingsChangeOptions.label2      "Settings can be changed using the Files & Applications panel in &brandShortName;'s Options.">

This won't work.

I wonder if flod has a better idea. How rational would it be to rm the toolkit file, and move the browser one here? awsy?
Attachment #8893649 - Flags: review?(l10n) → review-
Flags: needinfo?(francesco.lodolo)
(In reply to Axel Hecht [:Pike] from comment #11)
> This won't work.

In what sense?

> I wonder if flod has a better idea. How rational would it be to rm the
> toolkit file, and move the browser one here? awsy?

The string in /browser is obsolete on many levels (bug 421466, now prefs reorg).

The one in this patch is incorrect too: there's a "Files and Applications" section in the "General" panel, there's no "Files & Applications" panel (note the "and" too).

So, copying from browser doesn't seem to make a lot of sense to me.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #12)
> The one in this patch is incorrect too: there's a "Files and Applications"
> section in the "General" panel, there's no "Files & Applications" panel
> (note the "and" too).

I should have added that this is an assumption on my side, based on the Italian build and a look at the code, since sadly DTD files have both.
https://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/preferences/preferences.dtd#l25
Ok, updated the patch, a'ka - "Don't try to be the smarts sandwich in the drawer, Zibi. Just do your job and do it well."
Comment on attachment 8893649 [details]
Bug 1377911 - Do not strip localized override entries when repackaging.

https://reviewboard.mozilla.org/r/164748/#review172322

Can you add a comment to browser/locales/jar.mn to put overrides into browser/base?

With that, r=me.

Also, I think we should see if we can do something similar for mobile/android, it'd be good to be consistent here.

(And TB/SM)
Attachment #8893649 - Flags: review?(l10n) → review+
Filed bug 1389397 for Fennec. Will work on it now.

Jorg, there's no rush with that for TB/SM, but wanted to put it on your radar.
Flags: needinfo?(jorgk)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7581c0c154de
Move the override chrome entries from language manifests to product manifests. r=Pike
So you're saying we should move
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mail/locales/jar.mn#10
settingsChange.dtd and netError.dtd somewhere else? We don't seem to have appstrings.properties.
Flags: needinfo?(jorgk)
Yeah! 

https://dxr.mozilla.org/comm-central/source/mail/base/jar.mn

looks like a good place. :)
Flags: needinfo?(VYV03354)
Blocks: 1389407
https://hg.mozilla.org/mozilla-central/rev/7581c0c154de
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
thank you Sebastian!
Flags: needinfo?(gandalf)
Reopening because this was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yeah, so Axel, I have to say, I completely don't understand what's going on.

I tested the patch manually before landing on all platforms, in several languages and it all works. You were of course correct that it needed tests, but my assumption was that tests can be added later since they're purpose is to prevent us from regressing later, not to catch a regression this patch introduces now.

Once the regressions popped up, I can only reproduce it locally on repackaged builds. It doesn't show up just after build, it doesn't show up after merge-% + chrome-%. It only shows up if I do:

 - ./mach build
 - ./mach package
 - ./mach installers-%

and then unpack the result package and run it.

Not sure if it'll help you isolate the cause, but my biggest problem is that it doesn't show up in en-US at all. In en-US the path to "chrome://global/locale/netError.dtd" picks up the right override (so the netError.dtd from browser). In l10n build it does not override, so the "chrome:/global/locale/netError.dtd" shows the toolkit netError, while the "chrome://browser/locale/netError.dtd" picks up the browser one.

How is it possible that depending on locale the override is registered or not, if what this patch does should, by my naive logic, make the override independent of the locale logic?
Flags: needinfo?(l10n)
I verified that chrome/chrome.manifest differs between the en-US and de linux builds from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=80ff3f300e05f38f96c385b03d1973a966a2bd35&selectedJob=122738484.

My suspicion is that something in https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/packager/l10n.py does this, but I don't find anything that's obvious to me.

Redirecting the needinfo to glandium, hopefully he can help.
Flags: needinfo?(l10n) → needinfo?(mh+mozilla)
:glandium was able to help me on IRC. We were stripping override entries if they contained `locales` path in them.
I added a patch to remove this limitation.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8893649 [details]
Bug 1377911 - Do not strip localized override entries when repackaging.

https://reviewboard.mozilla.org/r/164748/#review177690
Attachment #8893649 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8900978 [details]
Bug 1377911 - Move the override chrome entries from language manifests to product manifests.

carry-over r+ - the patch didn't change, I just added the dependency on the packager patch.
Attachment #8900978 - Flags: review?(l10n) → review+
Comment on attachment 8893649 [details]
Bug 1377911 - Do not strip localized override entries when repackaging.

https://reviewboard.mozilla.org/r/164748/#review177832

Doesn't this need to fix the call-sites to .localized?

http://searchfox.org/mozilla-central/search?q=.localized&case=false&regexp=false&path=packager%2Fl10n.py probably?
Attachment #8893649 - Flags: review+
Attachment #8900978 - Flags: review+ → review?(l10n)
Comment on attachment 8900978 [details]
Bug 1377911 - Move the override chrome entries from language manifests to product manifests.

https://reviewboard.mozilla.org/r/172428/#review178008
Attachment #8900978 - Flags: review?(l10n) → review+
(In reply to Axel Hecht [:Pike] from comment #38)
> Doesn't this need to fix the call-sites to .localized?
> 
> http://searchfox.org/mozilla-central/search?q=.
> localized&case=false&regexp=false&path=packager%2Fl10n.py probably?

Got confused by the lack of context, .localized is there on the baseclass in http://searchfox.org/mozilla-central/source/python/mozbuild/mozpack/chrome/manifest.py#30.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c0bbead5d73
Do not strip localized override entries when repackaging. r=glandium,Pike
https://hg.mozilla.org/integration/autoland/rev/8396a848ae25
Move the override chrome entries from language manifests to product manifests. r=Pike
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/e8b80dec78b4
Do not strip localized override entries when repackaging. r=glandium,Pike
https://hg.mozilla.org/mozilla-central/rev/31465a03c03d
Move the override chrome entries from language manifests to product manifests. r=Pike a=merge
https://hg.mozilla.org/mozilla-central/rev/e8b80dec78b4
https://hg.mozilla.org/mozilla-central/rev/31465a03c03d
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
I'm monitoring for any regressions here and it seems like mac, linux and windows got an update with the patch and there are no regressions reported today in bugzilla.
I also manually tested local repacks for the regressions and it seems clear.
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf) → qe-verify-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.