Closed
Bug 1365709
Opened 7 years ago
Closed 7 years ago
Consume new webextension-based langpacks
Categories
(Toolkit :: Add-ons Manager, enhancement)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
(Whiteboard: triaged)
Attachments
(2 files, 3 obsolete files)
Based on the conversation from bug 1365031 we want to start producing langpacks as webextensions. In bug 1365440 we're planning how to build the new langpacks and in this bug I'd like to focus on consuming them. The plan is to: 1) Recognize the special kind of extension based on some entry in manifest.json 2) Register those langpacks early (including from the profile dir) to finalize the negotiated languages list early 3) For the selected language, register it's chrome entries from manifest.json in ChromeRegistry 4) Register it as a FileSource in L10nRegistry. 5) Register the selected langpacks as "resources" in ChromeRegistry
Assignee | ||
Comment 1•7 years ago
|
||
:kmag, :aswan - can you confirm that approach and help me get started with where and how to write it? :)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
Comment 2•7 years ago
|
||
To clarify, the idea is that langpacks should be packaged in a format that is like webextensions (ie, metadata about the langpack comes from manifest.json), but we won't actually load/execute webextension code when installing/enabling a langpack. Related to that, I'm moving the Component for this bug over to the Add-ons Manager. For step 1, I would start here: http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1073 This is the code that is executed when the add-ons manager is asked to install an xpi that contains a manifest.json file. I'm not sure whether it makes sense to extend the ExtensionData class or just do your own recognition/parsing here. It depends how much of the langpack manifest contents overlaps between extensions and langpacks. It might make sense to split ExtensionData into a base class for common manifest elements with separate subclasses for extensions and langpacks. For step 2, early startup work is handled here: http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/mozapps/extensions/AddonManagerStartup.cpp#549 There are two halves to this: 1. identify the information from the extension that you're going to need at browser startup and arrange for it to be stored in addonStartup.json at the time the langpack is installed 2. Execute steps 3-5 from comment 0 during startup using the information stored in addonStartup.json Hopefully that makes sense from a high level, I'm happy to help with the details as much as I can, Kris is the original author of pretty much all the code you'll be touching and he has considerably more background here...
Component: WebExtensions: General → Add-ons Manager
Flags: needinfo?(aswan)
Comment 3•7 years ago
|
||
I don't want to pile onto the scope of this bug, but I'd also like us to strongly consider signing language packs which is bug 1197876 and ran out of steam a while ago. I wasn't able to figure out what was actually meant to be done around bug 1197876 comment 7 for example.
Assignee | ||
Comment 4•7 years ago
|
||
:andym - I assume that signing is more of a build config task, and we tackle the new langpacks build config in bug 1365440. If any of the steps I proposed there has to be changed to get the signing, please, let me know.
Comment 5•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4) > :andym - I assume that signing is more of a build config task, and we tackle > the new langpacks build config in bug 1365440. > Sounds good, although we'll also need to change add-ons manager to enforce the signing too (with a suitable grace period as build processes catch up).
Comment 6•7 years ago
|
||
I agree with comment 0 and comment 2. We'd also need to add some code to AddonManagerStartup.cpp to register chrome packages based on, and extract locale lists from, the addonStartup JSON data. The chrome registration will probably also require some component manager changes: http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/xpcom/components/nsComponentManager.cpp#1979 since we currently only allow registering static manifest files, and we'd probably want these entries to be generated on the fly.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #6) > The chrome registration will probably also require some component manager > changes: > > http://searchfox.org/mozilla-central/rev/ > f55349994fdac101d121b11dac769f3f17fbec4b/xpcom/components/nsComponentManager. > cpp#1979 > > since we currently only allow registering static manifest files, and we'd > probably want these entries to be generated on the fly. For the chrome registration I thought we'll be doing something with http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/chrome/nsChromeRegistryChrome.cpp#696 Is that not the right place to hook AddonsManager to register chrome registry entries?
Comment 8•7 years ago
|
||
Yes, but that's only called by the manifest parser, and the manifest parser can currently only be invoked for static manifest files. We'd need to add an external API to invoke that logic without a static manifest file.
Flags: needinfo?(kmaglione+bmo)
Comment 9•7 years ago
|
||
(In reply to Andy McKay [:andym] from comment #3) > I don't want to pile onto the scope of this bug, but I'd also like us to > strongly consider signing language packs which is bug 1197876 and ran out of > steam a while ago. I wasn't able to figure out what was actually meant to be > done around bug 1197876 comment 7 for example. I expect signing to come up once all our l10n builds are in TC, and then we'll sign them. I don't think that it matters much what we put in the xpi exactly, tbh. So I'd like to move signing out of the scope of the work here. As far as linux distro builds go, they should either do a proper multi-locale build, or look into putting the langpacks in a location where they're distro integration add-ons are. I hope that we have a solution for that stuff already? As for chrome registry, given that we need three features of chrome registration (chrome, resource, override), maybe it's not worth it to define a special code path, and maybe we just ship langpacks as web extensions with an embedded mostly-standard langpack addon?
Assignee | ||
Comment 10•7 years ago
|
||
> As far as linux distro builds go, they should either do a proper multi-locale build, I'm investigating the ability to build multi-lingual Firefox in bug 1358824. > As for chrome registry, given that we need three features of chrome registration (chrome, resource, override), maybe it's not worth it to define a special code path, and maybe we just ship langpacks as web extensions with an embedded mostly-standard langpack addon? Are you suggesting we keep a chrome.manifest in the webextension and make ChromeRegistry read it?
Flags: needinfo?(l10n)
Comment 11•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > > As for chrome registry, given that we need three features of chrome registration (chrome, resource, override), maybe it's not worth it to define a special code path, and maybe we just ship langpacks as web extensions with an embedded mostly-standard langpack addon? > > Are you suggesting we keep a chrome.manifest in the webextension and make > ChromeRegistry read it? We could do that, but I'd prefer not to if we can avoid it. The resource packages should be registered separately form the chrome.manifest either way. They're really an entirely separate thing from chrome registration, and the way that we handle them for restartless add-ons is a bit of a hack. What do we use override for in langpacks?
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #11) > What do we use override for in langpacks? We currently have three: "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'm not sure why are the part of the langpack (I'd expect them to be the same for each langpack?). Maybe we can keep those in some non-locale browser manifest if that's the case? Pike?
Comment 13•7 years ago
|
||
We use overrides ............ OK, so ..... There's bugs 1255404, 1255382 where we use overrides and shouldn't. But then for desktop, we actually really have some overrides that are useful, to customize toolkit strings for Firefox. And then there's http://searchfox.org/mozilla-central/source/mobile/android/locales/jar.mn#39-101 to enable to ship sparse localizations for toolkit on fennec. I don't know enough about the actual interactions of those overrides, and if we could move them from the localized chrome.manifest into the application chrome.manifest, and then we wouldn't have to replicate that logic in each installable bundle?
Flags: needinfo?(l10n)
Assignee | ||
Comment 14•7 years ago
|
||
Ok. :kmag - do you think it's better for me to investigate now an attempt to remove the overrides from langpacks, or keep them and register them together with "locale" and "resource" entries via manifest.json and AddonsManager logic?
Flags: needinfo?(kmaglione+bmo)
Comment 15•7 years ago
|
||
OK. Well, if those are always the same for all language packs for a given app, in the worst case, we should just be able to define a static manifest for those overrides, and load it whenever we load a language pack. So all we should really need to worry about are the locale entries, which I think should be easy enough to handle.
Comment 16•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14) > Ok. :kmag - do you think it's better for me to investigate now an attempt to > remove the overrides from langpacks, or keep them and register them together > with "locale" and "resource" entries via manifest.json and AddonsManager > logic? I think things will be easier if we can just remove them. But, either way, I'm happy as long as we don't have to read a chrome.manifest file from the extension.
Flags: needinfo?(kmaglione+bmo)
Comment 17•7 years ago
|
||
hi :gandalf, since you're working on this do you want to assign it to yourself and set your own priority?
Flags: needinfo?(gandalf)
Whiteboard: triaged
Assignee | ||
Comment 18•7 years ago
|
||
yeah, totally. Taking :)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
Comment 19•7 years ago
|
||
:) thanks :gandalf!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. :aswan, can you take a look at this? I tried to read through the XPIInstall and AddonManagerStartup (as per comment 2), but I'm totally lost in details. I don't know if I need a new schema, and if so how to write one that matches the manifest that I'm providing with the langpack.
Attachment #8872650 -
Flags: feedback?(aswan)
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. https://reviewboard.mozilla.org/r/144184/#review147950 Sorry, I should have alerted you earlier that I think the way webextension themes were done is sloppy and I'd like to avoid copying that pattern if possible. I was hoping we could do some bigger refactoring around manifest parsing, ExtensionData, etc. The basic idea would be to have a single definition of the common bits of any manifest and separate definitions that logically inherit from the base for extensions, langpacks, themes, etc. So for langpacks, the only bits from toolkit/components/extensions that we would re-use are the manifest parsing and validation, we wouldn't even instantiate ExtensionData. Looking more closely at this though, there's a non-trivial chunk of work that would have to be done to make this practical. What's your timeline for this project?
Assignee | ||
Comment 23•7 years ago
|
||
I don't have the exact ETA when it has to be done, but I'm trying to get as much work done ahead of the switch as possible. getting new langpack model to be handled by Gecko would help me with that and allow me to test the multi-locale Firefox codeflow that I'm working on right now.
Assignee | ||
Comment 24•7 years ago
|
||
This is an example webextensions compatible langpack generated using the patch from bug 1365440. We met with :aswan and decided to use the "langpack-id" field to recognize that it's a langpack. Now, the next steps: 1) Refactor the Addons code to handle langpack type of the web manifest 2) Create hooks for adding/updating/removing langpacks 3) Make those hooks properly fire ChromeRegistry API to add/update/remove "locale" entries 4) Make the hook properly add/remove ChromeRegistry's "resource" entry for use by L10nRegistry 5) Add XPCOM/C++ entry point for L10nRegistry so that Addons code can call it (since it's in C++) 6) File a bug to consider removing the "override" entries from langpacks Details: For (3): All the entries to be added are in the manifest in "chrome_entries" field. The overrides will either have to be added as well, or may be removed by (6). For (4): This should happen only on add/remove, no action required on "update". It should do a programmatic equivalent of: "resource langpack-${langpack-id} path/to/langpack" For (5): The API calls will look sth like this: On "add": ``` let langpackId = 'langpack-pl'; let locales = ['pl']; let basePath = `resource://${langpackId}`; const langpackSource = new FileSource(langpackId, locales, basePath); L10nRegistry.registerSource(langpackSource); ``` On "update": ``` let langpackId = 'langpack-pl'; let locales = ['pl']; let basePath = `resource://${langpackId}`; const langpackSource = new FileSource(langpackId, locales, basePath); L10nRegistry.updateSource(langpackSource); ``` On "remove": ``` L10nRegistry.removeSource(langpackId); ``` :aswan - hope that's a good start for you!
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
Here's a quick-and-dirty first cut that should hopefully get us to being able to load a new-style langpack in the current session. Uninstall/disable still need to be handled as does the startup code for an installed langpack. As mentioned in the commit message in the attached patch, there's also the open task of adding a new api to add chrome registry entries without using a chrome.manifest file. But first things first, :gandalf can you check that this is basically working so we're on the right track? And Kris, if you want to look the patch over and give some feedback that would be good too.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gandalf)
Comment 28•7 years ago
|
||
Oh, one other thing: the attached sample xpi has the Firefox application uuid in the manifest.json, it needs to be changed to something else (e.g., "langpack-pl@mozilla.org") or it won't load.
Updated•7 years ago
|
Attachment #8872650 -
Flags: feedback?(aswan)
Assignee | ||
Updated•7 years ago
|
Attachment #8872650 -
Attachment is obsolete: true
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8885481 [details] Bug 1365709 Crude strawman for webextension langpacks https://reviewboard.mozilla.org/r/156332/#review166912 It works! I built it on top of L10nRegistry and it properly registered a langpack and added it to L10nRegistry! Wohooo! :) The two things that are needed to make it MVP: 1) The ChromeRegistry entries should be added 2) The langpack should show in Addons Language Packs page (so that they can be removed) :) ::: toolkit/mozapps/extensions/internal/WebExtensionLangpackBootstrap.js:15 (Diff revision 1) > +const Cc = Components.classes; > +const Cu = Components.utils; > +const Cr = Components.results; > + > +Cu.import("resource://gre/modules/Extension.jsm"); > +Cu.import("resource://gre/modules/L10nRegistry.jsm"); You need to use named imports here: const { L10nRegistry, FileSource } = Cu.import("resource://gre/modules/L10nRegistry.jsm", {});
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gandalf)
Assignee | ||
Comment 30•7 years ago
|
||
1) L10nRegistry landed and is now available in intl/l10n/L10nRegistry.jsm 2) build steps to generate new langpacks landed. Steps to build a langpack: a) cd objdir/browser/locales b) make langpack-webext-fr c) the result langpack is in objdir/dist/{win64|linux|mac}/xpi/ I think you should be unblocked with this now! :)
Flags: needinfo?(kmaglione+bmo) → needinfo?(aswan)
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
I sat down to write some notes about this and realized it wasn't a lot of work and would be more clearly expressed in code. So, I've pushed an updated version of the old patch that uses the interface from bug 1384689. With some very quick&dirty testing I was able to load the sample langpack. :gandalf, if you could try the patch out and let me know what is and isn't working, that would be a huge help and then we can talk next steps.
Flags: needinfo?(aswan) → needinfo?(gandalf)
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8885481 [details] Bug 1365709 Crude strawman for webextension langpacks https://reviewboard.mozilla.org/r/156332/#review176476 ::: toolkit/components/extensions/schemas/manifest.json:294 (Diff revision 2) > + "languages": { > + "type": "array", > + "items": { > + "type": "string", > + "pattern": "^[-A-Za-z]+$" > + } > + }, > + > + "chrome_entries": { > + "type": "array", > + "items": { "type": "string" } > + } We discussed this on IRC and decided on a different format: [16:47:16] <John-Galt> Also, will we ever need any langpacks to include more than one locale? [16:47:39] <John-Galt> Your example manifests had multiple, but it would be easier if we could assume they only included one, and then just listed resources. [16:47:43] <gandalf> John-Galt: yes [16:48:16] <gandalf> actually, quite soon we'll want to start shipping langpacks with partial locales and their fallbacks (say, ["es-MX", "es"]) [16:48:16] <John-Galt> Failing that, we could do something like "languages": {"pl": { ... }} [16:52:19] <John-Galt> gandalf: So, going with your example: "languages": {"pl": {"resources": ["/localization/pl/browser/aboutDialog.ftl"], "chrome_resources": {"alerts": "pl/locale/pl/alerts/", ...}}} [16:52:26] <John-Galt> And then we can just eventually phase out the latter. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1126 (Diff revision 2) > + async startup(data, reason) { > + let extension = new ExtensionData(data.resourceURI); > + let manifest = await extension.loadManifest(); > + > + this.langpackId = manifest["langpack-id"]; > + this.handle = aomStartup.registerChrome(data.resourceURI, manifest.entries); This needs to be generated form manifest data rather than pulled directly from the manifest.
Assignee | ||
Comment 34•7 years ago
|
||
I'm updating the langpack generation code to handle the updated scheme of the manifest in bug 1390461. > This needs to be generated form manifest data rather than pulled directly from the manifest. I'd like to keep the entries in the manifest as: "chrome_resources": [ "locale global pl path/", ... ] and only `entries.map(entry => entry.split(' '))` in the consuming code. Is that ok with you? > :gandalf, if you could try the patch out and let me know what is and isn't working, that would be a huge help and then we can talk next steps. :aswan - the code seems to work except of a few changes I have to make. The open questions for me are: 1) What is missing 2) The chrome registration does register, but then ChromeRegistry fails to pick up the resources properly. (something about paths to those resources doesn't point to the langpack basepath?) Can you help me with that? I feel like we're close :)
Depends on: 1390461
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
I pushed another patch with a few little cleanups. I'm not sure what the issue is with the chrome registration, I suspect Kris could probably figure it out much more quickly then me (plus he's in your time zone...) Kris, I realize the schema and other Extension.jsm bits are pretty hacky, I see a ton of opportunity to clean stuff up but I'd like to do that in a follow-up given many other pressing short-term bugs.
Comment 37•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #34) > I'm updating the langpack generation code to handle the updated scheme of > the manifest in bug 1390461. > > > This needs to be generated form manifest data rather than pulled directly from the manifest. > > I'd like to keep the entries in the manifest as: > > "chrome_resources": [ > "locale global pl path/", > ... > ] > > and only `entries.map(entry => entry.split(' '))` in the consuming code. Is > that ok with you? Is there a particular reason you want to do that? I'm pretty strongly opposed to it. Chrome resources registered by language packs need to be validated. We need to make sure they're only registering locale resources, and that the locale resources they're registering are only for the locales they claim to provide. Aside from making that easier to verify, the format I suggested is also considerably more compact.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 38•7 years ago
|
||
I filed bug 1393147 to update the scheme and put r? on you there, let's discuss it there :)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•7 years ago
|
||
An updated .xpi file with latest schema changes
Attachment #8881838 -
Attachment is obsolete: true
Assignee | ||
Comment 41•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. This patch with the attached example .xpi gets to LangpackBootstrapScope.startup and then data.manifest is undefined. Can you help me get through this hurdle?
Attachment #8872650 -
Flags: review?(kmaglione+bmo) → feedback?(aswan)
Comment 42•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. Looks like I was overzealous in an earlier "clean up". The fix is to apply: diff --git a/toolkit/mozapps/extensions/internal/XPIProvider.jsm b/toolkit/mozapps/extensions/internal/XPIProvider.jsm --- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm +++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ -1113,17 +1113,18 @@ function recordAddonTelemetry(aAddon) { } class LangpackBootstrapScope { install(data, reason) {} uninstall(data, reason) {} async startup(data, reason) { Services.console.logStringMessage(`manifest: ${JSON.stringify(data.manifest)}`); - let manifest = data.manifest; + let extension = new ExtensionData(data.resourceURI); + let manifest = extension.loadManifest(); this.langpackId = manifest.langpack_id; this.handles = []; this.languages = Object.keys(manifest.languages); Services.console.logStringMessage(`Languages: ${this.languages}`); for (const [language, entries] of Object.entries(manifest.languages)) { const chromeEntries = Object.entries(entries).map(([alias, path]) => {
Attachment #8872650 -
Flags: feedback?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8885481 -
Attachment is obsolete: true
Flags: needinfo?(gandalf)
Assignee | ||
Comment 44•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. Together with bug 1393147 this patch works! I'm able to produce a langpack, install it, it registers in Chrome and L10n registries and I can access the files, switch language on fly and so on :) I'm asking for the feedback first, because I'm not sure if my schema is correct and if the naming scheme for the langpack-id and protocol alias are acceptable. I'm also not sure where should we distinguish between langpacks for different products (Firefox, Fennec, etc.) and how to decide which FileSources to register for which product. But as far as I know, this fully works in Firefox, so it's complete. :)
Attachment #8872650 -
Flags: review?(kmaglione+bmo)
Attachment #8872650 -
Flags: feedback?(l10n)
Attachment #8872650 -
Flags: feedback?(kmaglione+bmo)
Comment 45•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. sry, but the code here is out of my leage, can't get myself to have an opinion on the changes.
Attachment #8872650 -
Flags: feedback?(l10n)
Assignee | ||
Updated•7 years ago
|
Assignee: aswan → gandalf
Assignee | ||
Comment 46•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. Formally recognize we're in a review zone :)
Attachment #8872650 -
Flags: feedback?(kmaglione+bmo) → review?(kmaglione+bmo)
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. https://reviewboard.mozilla.org/r/144184/#review180596 This mostly looks good. I'm just worried about the fact that this does computation at every startup rather than storing computed data in the startup cache. ::: toolkit/components/extensions/schemas/manifest.json:224 (Diff revision 3) > + > + "minimum_chrome_version":{ > + "type": "string", > + "optional": true > + }, Seems unnecessary. ::: toolkit/components/extensions/schemas/manifest.json:291 (Diff revision 3) > + "preprocess": "localize" > + }, > + > + "langpack_id": { > + "type": "string", > + "pattern": "^[-A-Za-z]+$" Nit: Maybe something like `"^[a-zA-Z][a-zA-Z-]+$"`? ::: toolkit/components/extensions/schemas/manifest.json:297 (Diff revision 3) > + }, > + > + "languages": { > + "type": "object", > + "patternProperties": { > + "^[a-zA-Z-]+$": { Can we maybe be a bit stricter about this? Maybe `/^[a-z]{2}[a-zA-Z-]+$/`? ::: toolkit/components/extensions/schemas/manifest.json:308 (Diff revision 3) > + "optional": true > + }, > + "chrome_resources": { > + "type": "object", > + "patternProperties": { > + "^[a-zA-Z-\\.]+$": { Nit: No need for the `\\` ::: toolkit/components/extensions/schemas/manifest.json:311 (Diff revision 3) > + "type": "object", > + "patternProperties": { > + "^[a-zA-Z-\\.]+$": { > + "choices": [ > + { > + "type": "string" Nit: `"$ref": "ExtensionURL"` ::: toolkit/components/extensions/schemas/manifest.json:317 (Diff revision 3) > + }, > + { > + "type": "object", > + "patternProperties": { > + "^[a-z]+$": { > + "type": "string" Nit: `"$ref": "ExtensionURL"` ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1121 (Diff revision 3) > + install(data, reason) {} > + uninstall(data, reason) {} > + > + async startup(data, reason) { > + const extension = new ExtensionData(data.resourceURI); > + const manifest = await extension.loadManifest(); This is a fairly expensive operation without the startup cache. Please create a separate `Langpack` class in Extension.jsm that stores these computed values in the startup cache the same way the Extension class dos. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1144 (Diff revision 3) > + for (const [language, entries] of Object.entries(manifest.languages)) { > + if (entries.chrome_resources) { > + const chromeEntries = Object.entries(entries.chrome_resources).map( > + ([alias, path]) => { > + return ["locale", alias, language, path]; > + } > + ); > + this.chromeRegistryHandles.push( > + aomStartup.registerChrome(manifestURI, chromeEntries) > + ); > + } > + } Same here. This is more expensive than I'd like it to be for something that happens at every startup. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1151 (Diff revision 3) > + this.chromeRegistryHandles.push( > + aomStartup.registerChrome(manifestURI, chromeEntries) > + ); Nit: Please use a single entry for registerChrome call for all locales.
Attachment #8872650 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. I applied all the feedback except of the Langpack class for startup cache. Building it based on Extension class in Extension.jsm seems like a non-trivial task. It may take me a bit.
Attachment #8872650 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 50•7 years ago
|
||
:aswan, when you're back from PTO, would you have time to help me get the POC of the Localization class Kris is asking for? The Extension class is over 600 lines of code and it doesn't seem easy to just take what's needed for Langpack startup cache.
Flags: needinfo?(aswan)
Comment 51•7 years ago
|
||
It should be simple. It just needs to be something like: this.Langpack = class extends ExtensionData { static getBootstrapScope(id, file) { return new LangpackBootstrapScope(); } async promiseLocales(locale) { let locales = await StartupCache.locales .get([this.id, "@@all_locales"], () => this._promiseLocaleMap()); return this._setupLocaleData(locales); } readLocaleFile(locale) { return StartupCache.locales.get([this.id, this.version, locale], () => super.readLocaleFile(locale)) .then(result => { this.localeData.messages.set(locale, result); }); } get manifestCacheKey() { return [this.id, this.version, Services.locale.getAppLocaleAsLangTag()]; } async _parseManifest() { let data = await super.parseManfest(); data.chromeResources = this.getChromeResources(); return data; } parseManifest() { return StartupCache.manifests.get(this.manifestCacheKey, () => this._parseManifest()); } async startup(reason) { ...; } async shutdown(reason) { ...; } };
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•7 years ago
|
||
I updated the patch to your feedback and it seems to work locally. I tested installing, uninstalling and starting with an installed langpack.
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. https://reviewboard.mozilla.org/r/144184/#review180732 r=me with issues addressed. This could also use some tests, but that can be done in a follow-up, as long as it happens before we switch to these langpacks by default. ::: toolkit/components/extensions/Extension.jsm:431 (Diff revision 5) > return results; > } > > readJSON(path) { > return new Promise((resolve, reject) => { > - let uri = this.rootURI.resolve(`./${path}`); > + let uri = this.resourceURL.resolve(`./${path}`); This will throw, since `resourceURL` is a string, and has no `resolve` method. ::: toolkit/components/extensions/Extension.jsm:460 (Diff revision 5) > // capabilities this extension has access to, as derived from the > // manifest. The current implementation just returns the contents > // of the permissions attribute, if we add things like url_overrides, > // they should also be added here. > get userPermissions() { > + if (this.type != "extension") { Nit: WebExtensions code prefers strict equality/inequality (`===`/`!==`) operators. ::: toolkit/components/extensions/Extension.jsm:631 (Diff revision 5) > - .map(path => path.replace(/^\/*/, "/")); > + .map(path => path.replace(/^\/*/, "/")); > + } > + } > > return {apiNames, dependencies, originPermissions, id, manifest, permissions, > webAccessibleResources}; We'll also need to return the `type` here, and copy it to the Extension object in `loadManifest`, or it won't be on most extension objects at runtime (since this method is only called when processed manifest data is not already cached) ::: toolkit/components/extensions/Extension.jsm:844 (Diff revision 5) > + this.langpack = new Langpack(data); > + return this.langpack.startup(); > + } > + > + shutdown(data, reason) { > + this.langpack.shutdown(); Nit: Please set `this.langpack = null` here. ::: toolkit/components/extensions/Extension.jsm:1506 (Diff revision 5) > + const manifestURI = Services.io.newURI( > + `${this.resourceURL}manifest.json` > + ); Nit: `Services.io.newURI("manifest.json", null, this.rootURI)` ::: toolkit/components/extensions/Extension.jsm:1511 (Diff revision 5) > + > + Nit: NO need for blank lines. ::: toolkit/components/extensions/Extension.jsm:1513 (Diff revision 5) > + ); > + > + this.chromeRegistryHandle = null; > + > + > + let chromeEntries = data.chromeResources; Nit: No need for the temporary. ::: toolkit/components/extensions/Extension.jsm:1514 (Diff revision 5) > + > + this.chromeRegistryHandle = null; > + > + > + let chromeEntries = data.chromeResources; > + Services.console.logStringMessage(JSON.stringify(chromeEntries, null, 2)); Please remove :) ::: toolkit/components/extensions/Extension.jsm:1520 (Diff revision 5) > + const resourceProtocol = Services.io.getProtocolHandler("resource") > + .QueryInterface(Ci.nsIResProtocolHandler); Nit: Please add a top-level lazy getter for this. ::: toolkit/components/extensions/Extension.jsm:1528 (Diff revision 5) > + > + // We'll register different sources depending on the product. > + // For Firefox we'll register 'browser' and 'toolkit' > + // For Fennec, we may register a different 'browser' and the same 'toolkit' > + // > + // XXX: We may want to check if the directory exist to decide if Nit: "exists" Also, I'm fine with checking whether a directory exists, but if we do, it needs to be done from `_parseManifest` and the result stored in the startup cache. Please add something about that to the comment. ::: toolkit/components/extensions/Extension.jsm:1543 (Diff revision 5) > + `resource://${data.langpackId}/localization` > + )); > + } > + > + async shutdown(reason) { > + const data = await this.parseManifest(); We don't want to call this twice in a session. Please set `this.langpackId` after the first call and use that instead. ::: toolkit/components/extensions/Extension.jsm:1545 (Diff revision 5) > + } > + > + async shutdown(reason) { > + const data = await this.parseManifest(); > + L10nRegistry.removeSource(data.langpackId); > + this.chromeRegistryHandle.destruct(); This needs to be null checked. Also, please set to `null` after destructing. ::: toolkit/components/extensions/Extension.jsm:1547 (Diff revision 5) > + const resourceProtocol = Services.io.getProtocolHandler("resource") > + .QueryInterface(Ci.nsIResProtocolHandler); Nit: Please use a global lazy getter for this. ::: toolkit/components/extensions/Extension.jsm:1554 (Diff revision 5) > + resourceProtocol.setSubstitution(data.langpackId, null); > + } > + > + getChromeResources(manifest) { > + const chromeEntries = []; > + for (const [language, entries] of Object.entries(manifest.languages)) { Nit: s/entries/entry/ ::: toolkit/components/extensions/Extension.jsm:1555 (Diff revision 5) > + if (entries.chrome_resources) { > + for (const [alias, path] of Object.entries(entries.chrome_resources)) { Nit: We'd generally avoid the `if` here by dong something like `Object.entries(entry.chrome_resources || {})` ::: toolkit/components/extensions/Extension.jsm:1557 (Diff revision 5) > + getChromeResources(manifest) { > + const chromeEntries = []; > + for (const [language, entries] of Object.entries(manifest.languages)) { > + if (entries.chrome_resources) { > + for (const [alias, path] of Object.entries(entries.chrome_resources)) { > + if (typeof path == "string") { Nit: `===` ::: toolkit/components/extensions/Extension.jsm:1559 (Diff revision 5) > + } > + > + // If the path is not a string, it's an object with path per platform > + // where the keys are taken from AppConstants.platform > + const platform = AppConstants.platform; > + if (path.hasOwnProperty(platform)) { > + chromeEntries.push(["locale", alias, language, path[platform]]); > + } This should be an else block. ::: toolkit/components/extensions/Extension.jsm:1564 (Diff revision 5) > + } > + > + // If the path is not a string, it's an object with path per platform > + // where the keys are taken from AppConstants.platform > + const platform = AppConstants.platform; > + if (path.hasOwnProperty(platform)) { Nit: Please don't use `hasOwnProperty` as a method. It only works on objects that don't have their own `hasOwnProperty` property. If you need to use it, define a helper like: const hasOwnProperty = Function.call.bind({}.hasOwnProperty); And then do `hasOwnProperty(object, property)` ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4209 (Diff revision 5) > } > > if (isWebExtension(aType)) { > activeAddon.bootstrapScope = Extension.getBootstrapScope(aId, aFile); > + } else if (aType == "webextension-langpack") { > + activeAddon.bootstrapScope = new LangpackBootstrapScope(); Please export `Langpack` rather than `LangpackBootstrapScope` and call `Langpack.getBootstrapScope(aId, aFile)` instead.
Attachment #8872650 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 58•7 years ago
|
||
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. I think I got it all working, thanks Kris! I decided to bite the bullet and get the FileSource registration correctly, and ended up realizing that it shouldn't be a hardcoded list of source combinations in the Extension.jsm, but rather a list of sources should be in the manifest.json. I filed bug 1396334 and set it as a dependency for this one. Basically, there's one toolkit which we'll "probe" for - the main "toolkit" one. And on top of that Firefox happens to have one more FileSource, which now will be listed in the manifest with it's base_path and potentially list of available paths. I'm setting an r? again since I changed more code. You can see the inter-diff (plus one unrelated change) here: https://reviewboard.mozilla.org/r/144184/diff/5-7/
Attachment #8872650 -
Flags: review+ → review?(kmaglione+bmo)
Assignee | ||
Comment 59•7 years ago
|
||
Kris, I filed bug 1395363 for tests. If you have any particular things that you'd like to have in tests, please add them to that bug.
Comment 60•7 years ago
|
||
mozreview-review |
Comment on attachment 8872650 [details] Bug 1365709 - Fix linter bustage. https://reviewboard.mozilla.org/r/144184/#review181094 r=me with nits addressed ::: toolkit/components/extensions/Extension.jsm:583 (Diff revision 7) > + > + let type = classifyPermission(perm); > + if (type.origin) { > + let matcher = new MatchPattern(perm, {ignorePath: true}); > + > + whitelist.push(matcher); > + perm = matcher.pattern; > + } else if (type.api) { > + this.apiNames.add(type.api); > + } > + > + this.permissions.add(perm); Pretty sure you didn't mean to duplicate this. ::: toolkit/components/extensions/Extension.jsm:1497 (Diff revision 7) > + } > + > + async _parseManifest() { > + let data = await super.parseManifest(); > + > + const productCodeName = AppConstants.MOZ_BUILD_APP.replace('/', '-'); Nit: Please use double quotes. ::: toolkit/components/extensions/Extension.jsm:1511 (Diff revision 7) > + > + // Check if there's a root directory `/localization` in the langpack. > + // If there is one, add it with the name `toolkit` as a FileSource. > + const sourceURI = Services.io.newURI( > + `/localization`, null, this.rootURI); > + await fetch(sourceURI).then( I'm pretty sure `fetch` will throw if you pass it an `nsIURI` object. You probably want something like `this.rootURI.resolve("./localization")`, which will return a string. But, `fetch` is probably not the best choice for this. Something like `await this.readDirectory("localization")` might be better. ::: toolkit/components/extensions/Extension.jsm:1557 (Diff revision 7) > + )); > + } > + } > + > + async shutdown(reason) { > + const data = await this.parseManifest(); Again, please don't call `parseManifest` again at shutdown. Just store `data.langpackId` as `this.langpackId` (and similar for other properties you need to preserve) the first time you parse it. ::: toolkit/components/extensions/schemas/manifest.json:330 (Diff revision 7) > + "patternProperties": { > + "^[a-z]+$": { > + "type": "object", > + "properties": { > + "base_path": { > + "type": "string" Nit: `"$ref": "ExtensionURL"` ::: toolkit/components/extensions/schemas/manifest.json:334 (Diff revision 7) > + "base_path": { > + "type": "string" > + }, > + "paths": { > + "type": "array", > + "items": { "type": "string" }, Nit: `"format": "strictRelativeURL"`
Attachment #8872650 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 63•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/960c3df4ced4 Consume new webextension based language packs. r=kmag
Comment hidden (mozreview-request) |
Comment 65•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f6fb12ee2f5a Fix linter bustage. r=kmag
Comment 66•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/960c3df4ced4 https://hg.mozilla.org/mozilla-central/rev/f6fb12ee2f5a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 67•7 years ago
|
||
Attachment #8901926 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•