Remove unnecessary content manifest for global-platform

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
7 years ago
8 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

There are no content files shipped for global-platform and global-region, only locale files.
Comment on attachment 649206 [details] [diff] [review]
Remove unnecessary content manifest for global-platform and global-region

It used to be that you had to register a content package before you could register a locale package. You've run this through tryserver?
Attachment #649206 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #2)
> Comment on attachment 649206 [details] [diff] [review]
> Remove unnecessary content manifest for global-platform and global-region
> 
> It used to be that you had to register a content package before you could
> register a locale package. You've run this through tryserver?

I only tested locally, but I sure can push to try.
So, in fact, the problem is that the "platform" keyword is only recognized for content. So while global-region is okay without a content package, global-platform is a different story. OTOH, now that there is the os flag, is there a reason to keep "platform" (especially considering global-platform is apparently the last place to use it)?
(In reply to Mike Hommey [:glandium] from comment #4)
> OTOH, now that there is the os flag,
> is there a reason to keep "platform" (especially considering global-platform
> is apparently the last place to use it)?

Mmmm one issue is that it's not possible to do AND rules with flags...
global-region was removed in bug 1277905.
Summary: Remove unnecessary content manifest for global-platform and global-region → Remove unnecessary content manifest for global-platform
Attachment #649206 - Attachment is obsolete: true
Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review134038

Bumping review to Axel. The serious problem that caused us not to do this in the past is that locale packs are not platform-dependent, and this effectively changes that. So I suspect that this is not ok, but the new world of l10n may have changed that.
Attachment #8858493 - Flags: review?(l10n)
Attachment #8858493 - Flags: review?(benjamin)
AFAIK, the lang packs are now per platform.
If that doesn't work, we can still replace "platform" with the relevant "os" tests.

i.e.

locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/unix/
locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/mac/ os=Darwin
locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/win/ os=WINNT
Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review134238

Yeah, we don't support platform dependent language packs. We introduced bugs in that already with conditional packaging of files, I know, but we shouldn't break this further. Looking at the allowed-dupes changes, this might already be fixed, didn't look hard.

At some point we'll want to get rid of the chrome registry for localization files alltogether, so we'll want to solve this in some way.

One way to do it would be to do the platform detection at the callsites, and reference platform specific strings, by file or by id. That's what we do for Preferences vs Settings, for example.
The other way would be to fast-path l20n with platform selectors for this on desktop, but I don't know what we need to do on the callsites to get this going. CCing gandalf, maybe he can spend some thoughts on this.

r- on the patch and the follow-ups, we'll need to solve this differently such that we don't introduce platform dependencies in the langpack distributable.
Attachment #8858493 - Flags: review?(l10n) → review-
My immediate goal is to get rid of the "platform" flag for chrome, such that bug 1158018 can finally be fixed without working around it.  Comment 13 would work just fine for that goal and wouldn't change the status quo wrt chrome registration in langpacks. Would you be okay with that for now? (also, you didn't CC gandalf)
Flags: needinfo?(l10n)
Maybe if you add a platform-flagged chrome override? (AFAICT, comment 13 would still lead to different chrome paths at runtime?)

AKA, I'm fine with chrome registry being runtime dependent, as long as the language pack xpi works as is on all platforms.

(I CCed gandalf before I submitted my review, knowing I couldn't do that through mozreview ;-) , probably w/out bugmail)
Flags: needinfo?(l10n)
I'm working on moving l10n resources away from chrome registry all together (bug 1333980).

It's one of the items on the path to the new l10n API which will allow us to get string variants per-platform, so one could write:

```
key = { PLATFORM() ->
  [win] Settings
  [mac] Peferences
 *[other] Options
}
```

On top of that, I start to suspect that intl.properties may have a better home in form of a .toml file instead of a .properties file combined into a complex pref.

So, there are some changes that I'll be working on over the next quarter.

If you want to fix it sooner, I'm with Pike. Do whatever you need to, but don't make us create per-platform langpacks.

I tried to read bug 1158018 but it feels like you guys make a lot of mental shortcuts when talking between each other and it's a bit hard to grasp the scope of the problem, at least in how it affects the platform specific l10n resources.
Could you give me a tl;dr and I'll try to help you with the solution? :)
Flags: needinfo?(mh+mozilla)
Attachment #8858494 - Flags: review?(benjamin)
Attachment #8858495 - Flags: review?(benjamin)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Could you give me a tl;dr and I'll try to help you with the solution? :)

The tl;dr is that we want the packaging steps to just use the chrome manifests to determine what to package, and we make it error out when things referenced in chrome manifests are missing. Which the empty content entry for global-platform triggers. We could add a one-off exception for that, but we might as well get rid of the "platform" thing entirely, considering it's possible to do the same without it.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review141358
Attachment #8858493 - Flags: review?(gps) → review+
Comment on attachment 8858494 [details]
Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

https://reviewboard.mozilla.org/r/130460/#review141362

::: commit-message-115ad:15
(Diff revision 2)
> +Unfortunately, for determinism reasons, the chrome manifest entries from
> +jar.mn are sorted (per bug 982075), so keeping global-platform/unix
> +would leave it appearing after /mac, and would override it on mac
> +because of the lack of "os" flag on the /unix entry (we can't put "os"
> +flags on that entry because we can't do something like os!=Darwin &&
> +os!=WINNT). So we move it to /gtk such that it always comes before /mac.

Do you feel this potential footgun requires an inline comment? Or even better, can the parser verify that flag-less entries appear before entries with flags?

(Or since this pattern is only used once, perhaps we'll just take a risk that the footgun never fires.)
Comment on attachment 8858494 [details]
Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

https://reviewboard.mozilla.org/r/130460/#review143084

Boy that's quite the footgun. Perhaps it would make sense to implement a specific manifest marker for all the unix-like OSes?
Attachment #8858494 - Flags: review?(benjamin) → review+
Comment on attachment 8858495 [details]
Bug 780562 - Remove support for the chrome "platform" flag.

https://reviewboard.mozilla.org/r/130462/#review143094
Attachment #8858495 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg [:bsmedberg] from comment #24)
> Comment on attachment 8858494 [details]
> Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.
> 
> https://reviewboard.mozilla.org/r/130460/#review143084
> 
> Boy that's quite the footgun. Perhaps it would make sense to implement a
> specific manifest marker for all the unix-like OSes?

os=Unix?
Blocks: 1365419
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/47661994bdb1
Take locale chrome manifest flags into account when repacking l10n. r=gps
https://hg.mozilla.org/integration/autoland/rev/c94e87a18096
Stop relying on the "platform" chrome flag for global-platform. r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/82edbfe72c47
Remove support for the chrome "platform" flag. r=bsmedberg
Depends on: 1366169
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.