Closed Bug 1306858 Opened 3 years ago Closed 3 years ago

Localizations for packed embedded webextensions searched for in extension root

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox51 fixed, firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: freaktechnik, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

When using localized strings in order for the manifest to be valid, a folder named after the default_locale has to exist in _locales/.

That test checks _locales/ in the XPI root for embedded WebExtensions instead of _locales/ in webextension/.
Component: WebExtensions: General → WebExtensions: Untriaged
Assignee: nobody → kmaglione+bmo
Component: WebExtensions: Untriaged → WebExtensions: General
Summary: Localizations for embedded webextensions searched in extension root → Localizations for packed embedded webextensions searched for in extension root
Attached file i18n-test.zip
The attached ZIP contains an SDK extension that embeds a very minimal WebExtension with just an en messages.json. To try it, it has to be packed to an XPI by JPM.

In the zip I already copied _locales to the root, but if deleted the manifest.json is no longer valid. However the _locales folder in webextensions/ is required too, since the strings are correctly loaded from there.
Duplicate of this bug: 1306968
Priority: -- → P2
Whiteboard: triaged
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

https://reviewboard.mozilla.org/r/82584/#review81434

::: toolkit/components/extensions/test/xpcshell/test_locale_data.js:12
(Diff revision 1)
>  const uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
>  function* generateAddon(data) {
>    let id = uuidGenerator.generateUUID().number;
>  
> -  data = Object.assign({applications: {gecko: {id}}}, data);
> +  data = Object.assign({embedded: true}, data);

Did you mean to move this test entirely over to hybrid addons?  (as opposed to testing both standalone and embedded?)
Attachment #8796863 - Flags: review?(aswan) → review+
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

https://reviewboard.mozilla.org/r/82584/#review81434

> Did you mean to move this test entirely over to hybrid addons?  (as opposed to testing both standalone and embedded?)

We're not actually testing either hybrid or standalone add-ons here, just the locale processing code. And this change just tests that it works if we're looking at a jar: root URL with a sub-directory rather than the root of the JAR.
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

https://reviewboard.mozilla.org/r/82584/#review81434

> We're not actually testing either hybrid or standalone add-ons here, just the locale processing code. And this change just tests that it works if we're looking at a jar: root URL with a sub-directory rather than the root of the JAR.

I understand that, the question was if you meant to move entirely over to the scenario that looks like a hybrid addon (i.e., a sub-directory in the jar) rather than explicitly testing both that and the scenario that looks like a regular webextension (i.e., the root of the jar)
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

https://reviewboard.mozilla.org/r/82584/#review81434

> I understand that, the question was if you meant to move entirely over to the scenario that looks like a hybrid addon (i.e., a sub-directory in the jar) rather than explicitly testing both that and the scenario that looks like a regular webextension (i.e., the root of the jar)

I did, yes. As long as we handle arbitrary paths, it shouldn't matter what that arbitrary path is. We have other tests that test locales with actual extensions, with locales in the root. This is just a unit test for the locale processing code.
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

https://reviewboard.mozilla.org/r/82584/#review81434

> I did, yes. As long as we handle arbitrary paths, it shouldn't matter what that arbitrary path is. We have other tests that test locales with actual extensions, with locales in the root. This is just a unit test for the locale processing code.

Okay, I'm pedantic and like covering all the cases down in the targeted unit tests but I agree with you that we do get full coverage elsewhere.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a359909d4490460a2cfd301003f48172c0f2add7
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions. r=aswan
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

Approval Request Comment
[Feature/regressing bug #]: Bug 1269342
[User impact if declined]: This issue prevents developers from localizing their embedded WebExtensions in affected Firefox versions.
[Describe test coverage new/current, TreeHerder]: The related features are well-covered by existing tests, and new tests were added for this change.
[Risks and why]: Low. The change is relatively simple, and the affected use cases are covered extensively by automated tests.
[String/UUID change made/needed]: None.
Attachment #8796863 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a359909d4490
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8796863 [details]
Bug 1306858: Fix locale discovery in packed, embedded WebExtensions.

Fix a locale issue related to WebExtensions. Take it in 51 aurora.
Attachment #8796863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to uplift like:

grafting 367478:a359909d4490 "Bug 1306858: Fix locale discovery in packed, embedded WebExtensions. r=aswan"
merging toolkit/components/extensions/Extension.jsm
warning: conflicts while merging toolkit/components/extensions/Extension.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.