Closed Bug 1402069 Opened 7 years ago Closed 7 years ago

Add a test string using new localization API

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1402069 - Add a test string for the new localization API.
59 bytes, text/x-review-board-request
Pike
: review+
mossop
: feedback+
Details 
In order to prove the new system, we're going to land a single string localized using the new localization API and let it ride trains.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

I moved from about:healthreport to about:accounts but kept the idea of shadowing an already existing string.

Axel, do you think that this is a good test for the first milestone?
Attachment #8910893 - Flags: feedback?(l10n)
While waiting for Pike's feedback:
* We shouldn't put a temp string in a path exposed to localization tools. Either we put it somewhere else, or drop the [FTL] part from the string.
* I'm not completely sure it's a good pick. You're taking a "brand" string, and replacing it with a distinct string, used only for the title of this page. If this is temporary, it might make sense.
https://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/syncBrand.dtd
We'll definitely drop the `[FTL]` part from the string before I request a review :)

wrt. choice of the string - I'm ok picking a different string, but I don't see a problem with replacing a brand string with a distinct string for the title. With L20n we are going to reduce reuse of strings and we do want to have a unique l10n-id per binding (l10n <--> widget).

But as I said, I'm fine going for a different string. My criteria are:

1) Stable UI - no recent or planned activity around
2) Relatively hidden - if things go wrong, minimize the impact either visual or UX
3) If possible in browser, not toolkit
4) Single DTD entity, prefer with a value over an attribute

Any candidates?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> We'll definitely drop the `[FTL]` part from the string before I request a
> review :)

OK, that makes more sense.

> wrt. choice of the string - I'm ok picking a different string, but I don't
> see a problem with replacing a brand string with a distinct string for the
> title. With L20n we are going to reduce reuse of strings and we do want to
> have a unique l10n-id per binding (l10n <--> widget).

Are we going to reduce reuse of branding strings (i.e. 'Firefox' as brandShortName? We really shouldn't, but that's a different discussion unrelated to this bug.

> 1) Stable UI - no recent or planned activity around
> 2) Relatively hidden - if things go wrong, minimize the impact either visual
> or UX
> 3) If possible in browser, not toolkit
> 4) Single DTD entity, prefer with a value over an attribute
> 
> Any candidates?

I'm not aware of any, the closes thing is aboutAbout.dtd, but it's 2 strings and in toolkit.
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review193496

f- for the comments flod made.

For fluent, we should have a message that's unique for the title, which references the brandname message. Sounds overly complex for this phase.

Also, we shouldn't keep the original DTD entity in the document, that's just going to hide bugs we'd find otherwise.
Attachment #8910893 - Flags: review-
> Are we going to reduce reuse of branding strings (i.e. 'Firefox' as brandShortName? We really shouldn't, but that's a different discussion unrelated to this bug.

As Pike said - we want to reference brand message from the title message, rather than directly reuse it. I don't think we should be super-strict about it, since we can often assume that brand is in the right form for a title, but that's a guess.

> Also, we shouldn't keep the original DTD entity in the document, that's just going to hide bugs we'd find otherwise.

You mean, we shouldn't do:

```
<title data-l10n-id="msg-id">&msg.id;</title>
```

?

That was my whole idea to keep us fallback on DTD if anything doesn't work with FTL. My take is that we're going to just manually monitor how this single message work, and if it ends up in the right places in all builds.

> f- for the comments flod made.

Flod made two comments - one about `[FTL]` in the value, and the other about title-specific string. I responded to both. Is there anything else that causes f- ?
Flags: needinfo?(l10n)
There's no point in doing this if it doesn't actually expose problems if it's not working.

The fallback plan should be that we back out the patch that introduced ftl, and that we do so without any worries about localizer impact, or conflicts with other on-going development of a feature.

So, remove the usage of the old string, but not remove the strings. Also, the back-out should be simple enough to not bother relman when we ask for it to be taken. We could go as far as make that switchable by a pref.
Flags: needinfo?(l10n)
Attachment #8910893 - Flags: feedback?(l10n)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

I updated the patch to use a single, simple string in an already async UI.

Pike - does it look like the right test for the scope of this milestone?
Attachment #8910893 - Flags: feedback?(l10n)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

I think this is a good string and UI to start with.

Do we expect the way this is coded to become a best practice? Should Localization objects be held locally?

I don't like the idea of complaining in the browser console if there's a problem, I don't think that's discoverable enough. Also, I'd re-introduce the idea to make this test pref-controlled. That gives us a lot of tools to switch things on and off, even through shield instead of a dot release, if something's funky in the release channel.
Attachment #8910893 - Flags: feedback?(l10n)
Great!

I updated the patch and I believe it's review ready!

I added the pref and moved us to use fluent-web.js rather than manual Localization.jsm.
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review195790

::: browser/components/preferences/in-content/main.js:2523
(Diff revision 4)
>    },
>    async chooseFolderTask() {
>      let bundlePreferences = document.getElementById("bundlePreferences");
> -    let title = bundlePreferences.getString("chooseDownloadFolderTitle");
> +    let title;
> +
> +    if (Services.prefs.getBoolPref("intl.l10n.fluent.disabled", false)) {

Wouldn't it make more sense to call the pref "intl.l10n.fluent.enabled"?

::: browser/components/preferences/in-content/main.js:2526
(Diff revision 4)
> -    let title = bundlePreferences.getString("chooseDownloadFolderTitle");
> +    let title;
> +
> +    if (Services.prefs.getBoolPref("intl.l10n.fluent.disabled", false)) {
> +      title = bundlePreferences.getString("chooseDownloadFolderTitle");
> +    } else {
> +      // If `intl.l10n.fluent-disabled` is false, we'll attempt to use

In general, I find double negatives to be harded to read. "fluent.enabled is true" reads much clearer to me.  Also note that you have a dash here and a dot in the name passed to getBoolPref.

::: browser/components/preferences/in-content/main.js:2531
(Diff revision 4)
> +      // If `intl.l10n.fluent-disabled` is false, we'll attempt to use
> +      // the new localization API for this string.
> +      try {
> +        title = await getExperimentalFluentString("choose-download-folder-title");
> +      } catch (e) {
> +        title = bundlePreferences.getString("chooseDownloadFolderTitle");

I agree with Axel that we shouldn't fallback here again. If it fails, let it fail and let us learn about it. The pref should make that an opt-in experiment. (Which is also why I'd prefer to call it fluent.enabled.)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review195806

::: browser/components/preferences/in-content/main.js:116
(Diff revision 4)
> +// localization API. It's meant to remain isolated and well guarded against
> +// any potential errors with a fallback on the stable API.
> +//
> +// See bug 1402061 for details.
> +async function getExperimentalFluentString(name) {
> +  return await document.l10n.formatValue(name);

Nit: you don't need await here. It's fine to simply return the promise returned by document.l10n.formatValue.  Skipping the await might actually avoid creating an additional promise here. That said, if you think it improves the readability of this code, let's keep it.
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review195836

What stas said, and some more.

Also, how did the tests for l10n-merge and this patch go? I'm thinking of adding a 

choose-download-folder-title
    .title = We should skip this.
    
as a localized entry, and ensure that we're having an empty main.ftl in the built package?

::: browser/components/preferences/in-content/main.js:115
(Diff revision 4)
> +// This function is temporary and serves to test the new
> +// localization API. It's meant to remain isolated and well guarded against
> +// any potential errors with a fallback on the stable API.
> +//
> +// See bug 1402061 for details.
> +async function getExperimentalFluentString(name) {

Now that you actually added the DOM API and use it, does it still make sense to have this wrapper?

::: browser/components/preferences/in-content/main.js:2523
(Diff revision 4)
>    },
>    async chooseFolderTask() {
>      let bundlePreferences = document.getElementById("bundlePreferences");
> -    let title = bundlePreferences.getString("chooseDownloadFolderTitle");
> +    let title;
> +
> +    if (Services.prefs.getBoolPref("intl.l10n.fluent.disabled", false)) {

I guess gandalf is trying to avoid to have to explicitly add the pref to all.js.

I wonder if that's actually good or bad. Gandalf, can you get a toolkit or prefs owner/peer to comment on that?
Attachment #8910893 - Flags: review?(l10n) → review-
> I guess gandalf is trying to avoid to have to explicitly add the pref to all.js.

Yeah, since it's a temporary pref, I wanted to avoid adding it for everyone into all.js which I believe would make it stay there even after we remove it.

> I wonder if that's actually good or bad. Gandalf, can you get a toolkit or prefs owner/peer to comment on that?

Yep. My plan was to get a round of review from you and then get :mossop. Will update the patch to your review feedback and add him for the next one :)

> The pref should make that an opt-in experiment. (Which is also why I'd prefer to call it fluent.enabled.)

Are you suggesting we start with the this code off (so, not triggering any FTL code) and make people who want to test it manually switch it "ftl.enabled"? That's a major change in approach - so far we planned to land it with FTL enabled and only add the pref to opt-out (or turn off via shield) if we discover any problem.
Flags: needinfo?(stas)
Updated the patch.

I'm operating under the assumption that I did misunderstood stas and he does not actually advocate switching opt-out to opt-in here.
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

Dave, as part of the careful rollout of L20n, we're planning to land this single string in our UI to test the whole localization ecosystem handling a single Fluent string.

Per :pike's request I added a pref here to enable us to flip it to turn off this codepath in case we discover any problem down the road.

My plan would be to land this, and then in 59 land more code and remove the if/else here and the pref.

Two requests:

 - can you take a look at the patch and confirm that this looks like a sensible thing to do?
 - can you help us decide if we should put this pref in the all.js, or is ok not to since it's temporary?

Thanks.
Attachment #8910893 - Flags: feedback?(dtownsend)
(In reply to Axel Hecht [:Pike] from comment #17)

> Also, how did the tests for l10n-merge and this patch go? I'm thinking of
> adding a 
> 
> choose-download-folder-title
>     .title = We should skip this.
>     
> as a localized entry, and ensure that we're having an empty main.ftl in the
> built package?

Yep! Verified that such Message is removed from the ftl file. Also if it contains any syntax error :)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review196036

::: browser/components/preferences/in-content/main.js:2522
(Diff revision 5)
> +    if (Services.prefs.getBoolPref("intl.l10n.fluent.disabled", false)) {
> +      title = bundlePreferences.getString("chooseDownloadFolderTitle");
> +    } else {
> +      title = await document.l10n.formatValue("choose-download-folder-title");
> +    }

Do we have any automated tests that verify that this string is set correctly?
> Do we have any automated tests that verify that this string is set correctly?

We have tests that verify that `DOMLocalization.prototype.formatValue` returns the right string based on contexts generated via L10nRegistry (which is also tested).

In the ideal world we'd love not to have tests that test the value being set in the UI, because in the Intl domain you should never test against a particular output value. (for example, that would limit our ability to launch tests in other locales among other things).

So, the way we'd like to test l10n in production is:

 - test for the UI code ensures that `data-l10n-id` is set.
 - test for L20n ensures that once something sets `data-l10n-id` we localize the element.

I recognize that for this experiment, we can't do this because we don't localize a XUL/HTML element, but rather load a string.

Do you recommend that we add a test for this particular string being returned?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> > Do we have any automated tests that verify that this string is set correctly?
> 
> We have tests that verify that `DOMLocalization.prototype.formatValue`
> returns the right string based on contexts generated via L10nRegistry (which
> is also tested).
> 
> In the ideal world we'd love not to have tests that test the value being set
> in the UI, because in the Intl domain you should never test against a
> particular output value. (for example, that would limit our ability to
> launch tests in other locales among other things).
> 
> So, the way we'd like to test l10n in production is:
> 
>  - test for the UI code ensures that `data-l10n-id` is set.
>  - test for L20n ensures that once something sets `data-l10n-id` we localize
> the element.

I agree with this in the general case.

> I recognize that for this experiment, we can't do this because we don't
> localize a XUL/HTML element, but rather load a string.
> 
> Do you recommend that we add a test for this particular string being
> returned?

I think it depends what we're looking to gain from this experiment. If we want verify that the localisation system as a whole is working in all release channels then we either have to direct QA to verify the strings manually for all channels etc. or we have a test that does as much of that as we can. I'd only suggest it for this first string and we can remove it once we're satisfied with the results but I think it would be useful.
> I'd only suggest it for this first string and we can remove it once we're satisfied with the results but I think it would be useful.

Is it ok if we only test against en-US locale?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> > I'd only suggest it for this first string and we can remove it once we're satisfied with the results but I think it would be useful.
> 
> Is it ok if we only test against en-US locale?

Yes
Added! Thank you Dave!
Attachment #8910893 - Flags: feedback?(dtownsend)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> I'm operating under the assumption that I did misunderstood stas and he does
> not actually advocate switching opt-out to opt-in here.

You understood well. I thought opt-in would be a better solution but I see the benefits of going for opt-out.
Flags: needinfo?(stas)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review196228

::: browser/components/preferences/in-content/main.js:2519
(Diff revision 7)
> +    // This is an experimental use of the new localization API.
> +    // We temporarily keep it behind a flag to let ourselves turn it off
> +    // if we discover any problems.
> +    //
> +    // If the `intl.l10n.fluent.disabled` is not set or is false, we'll use
> +    // the new API, otherwise we'll use the old API.

This comments repeats exactly what's written in code below it. Perhaps a better comment would be:

    New localization API experiment (October 2017).
    See bug 1402061 for details.

    The `intl.l10n.fluent.disabled` pref can be used
    to opt-out of the experiment in case of any
    unforseen problems. The legacy API will then
    be used.
Thanks! updated :)
Ok. I'm a bit lost. The test passes on mac, windows and linux locally and fails on try server.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #35)
> Ok. I'm a bit lost. The test passes on mac, windows and linux locally and
> fails on try server.

Did you run a packaged build? That is normally the problem when try fails since it always tests packaged builds.
It seems that I may have just made a mistake in the `ok()` command. I did `ok(title, value);` instead `ok(title === value);`.

I fired a try on updated patch. Not sure why it passes locally and fails on try tho. Result is a confusing red herring.
It wasn't the ok() part.

Mossop was right - packaged builds don't contain the `/localization/` directory.
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

Boom! Green :)

Readding f? on Dave.
Attachment #8910893 - Flags: feedback?(dtownsend)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review196538

So, I have details to argue about for this patch, but moreover, I've inspected the langpack from try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbce8b9d9127&group_state=expanded&selectedJob=138338374, and that ships the en-US file as Italian. It's bewildering to me that it doesn't on the tarball, which might indicated that there's two bugs.

One way to check is to create a localization with the localized ftl file on a user repo, and specify that /users/foo/ parent in testing/mozharness/configs/single_locale/mozilla-central.py instead of l10n-central, and create repack-N jobs on try for that to look at.

::: browser/components/preferences/in-content/tests/browser.ini:35
(Diff revision 10)
>  [browser_bug795764_cachedisabled.js]
>  [browser_bug1018066_resetScrollPosition.js]
>  [browser_bug1020245_openPreferences_to_paneContent.js]
>  [browser_bug1184989_prevent_scrolling_when_preferences_flipped.js]
>  [browser_engines.js]
> +[browser_fluent.js]

That support-files seems to be moving even further away from the test that it seems to belong to bothers me. Mossop?

::: browser/components/preferences/in-content/tests/browser_fluent.js:10
(Diff revision 10)
> +
> +  let doc = gBrowser.contentDocument;
> +
> +  let title = await doc.l10n.formatValue("choose-download-folder-title");
> +
> +  ok(title === "Choose Download Folder:", "Title should be picked up from en-US locale.");

I'm a bit concerned that we're creating a test that fails on localized builds to test localization.

Part of the problem is that we're using a string that's not accessible to tests where it's used, right?

Maybe just test that we're not triggering an error scenario?
Attachment #8910893 - Flags: review?(l10n) → review-
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

I can confirm that we're packaging en-US .ftl file in case localized file is missing when building a langpack.

STR:

1) apply the patch from this bug on -mc
2) ./mach build langpack-it
3) look into the produced .xpi file

Current Result:

In browser/localization/it/browser/preferences/ there's `main.ftl` file with en-US content.

Expected Result:

No file in that directory.
Attachment #8910893 - Flags: feedback?(dtownsend)
Depends on: 1410451
Thanks Axel!

Rebased on top of the patch from bug 1410451. Let's see what the try says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9798b22b1c47c644313ae0d9144d99cbb90e3b57 (will add L10n jobs that you did)

> Maybe just test that we're not triggering an error scenario?

Did that, but that raises a question - long time ago we talked about returning l10nId as the last resort fallback for missing entries.
Now I see that `Localization.formatValue` returns `undefined` instead.

:stas - is that intentional? Should we aim to change that to l10nId? I don't think it needs to block soft-launch, but it would be nice to use the opportunity to iron that out :)
Flags: needinfo?(stas)
No longer depends on: 1410451
Update here. I started drafting a test plan in bug 1410734 to ensure we behave according to our vision. I also filled small bugs that we need to iron out to get the right behavior for already known misbehaviors: bug 1410451, bug 1410731 and bug 1410733.

With those three patches in, I'm getting correct runtime fallback for repackaged version with a missing entry.
Here's a try run with the 3 fixes that gets the repackaged build fallback properly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f5324151af215fe4106a2d7e9361b816ef1bfe8 :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #45)

> :stas - is that intentional? Should we aim to change that to l10nId? I don't
> think it needs to block soft-launch, but it would be nice to use the
> opportunity to iron that out :)

No, it's a bug. I filed bug 1410849.
Flags: needinfo?(stas)
Attachment #8910893 - Flags: review?(l10n)
Attachment #8910893 - Flags: review?(l10n)
:mossop - with the test matrix completed in bug 1410734 (I'm taking a final look at Windows now, but things look good), we're ready to land this.

Do you think it's ok to land this experimental string in 58 and benefit from seeing how it rides trains before we start moving any more code, or do you think it's too late and would prefer us to land it once 59 opens?

Also, there's a pending question for you from Axel in comment 42 :
Flags: needinfo?(dtownsend)
(In reply to Axel Hecht [:Pike] from comment #42)
> I'm a bit concerned that we're creating a test that fails on localized
> builds to test localization.
> 
> Part of the problem is that we're using a string that's not accessible to
> tests where it's used, right?

It should be accessible shouldn't it?

> Maybe just test that we're not triggering an error scenario?

I guess I'd be ok with that. Maybe put some existing string that we expect to see replaced and compare that the new string differs?


(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #51)
> :mossop - with the test matrix completed in bug 1410734 (I'm taking a final
> look at Windows now, but things look good), we're ready to land this.
> 
> Do you think it's ok to land this experimental string in 58 and benefit from
> seeing how it rides trains before we start moving any more code, or do you
> think it's too late and would prefer us to land it once 59 opens?

I don't see any additional risk to this landing now as long as we do a visual inspection on the nightly that the string is present.
Flags: needinfo?(dtownsend)
Re ...

[browser_bug1020245_openPreferences_to_paneContent.js]
[browser_bug1184989_prevent_scrolling_when_preferences_flipped.js]
[browser_engines.js]
[browser_fluent.js]
support-files =
  browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul
[browser_change_app_handler.js]

...

The support-files seems to belong to [browser_bug1184989_prevent_scrolling_when_preferences_flipped.js], right? Should gandalf move the `support-files` back up?

Rewording my other question, the string we're getting from document.l10n is used in an external dialog, where we can't inspect it. All the test does is replicating a part of the code path of the original code flow, but not testing the actual code flow.
Flags: needinfo?(dtownsend)
(In reply to Axel Hecht [:Pike] from comment #53)
> Re ...
> 
> [browser_bug1020245_openPreferences_to_paneContent.js]
> [browser_bug1184989_prevent_scrolling_when_preferences_flipped.js]
> [browser_engines.js]
> [browser_fluent.js]
> support-files =
>   browser_bug1184989_prevent_scrolling_when_preferences_flipped.xul
> [browser_change_app_handler.js]
> 
> ...
> 
> The support-files seems to belong to
> [browser_bug1184989_prevent_scrolling_when_preferences_flipped.js], right?
> Should gandalf move the `support-files` back up?

Yes.

> Rewording my other question, the string we're getting from document.l10n is
> used in an external dialog, where we can't inspect it. All the test does is
> replicating a part of the code path of the original code flow, but not
> testing the actual code flow.

Ugh sorry I should have looked at the patch more than thinking in the abstract. Yeah that doesn't seem like much of a worthwhile test does it. I think this is a poor string to do this first test on because as you say we can't actually test that it is being set even to anything. Something in the DOM would be a far more valuable test.
Flags: needinfo?(dtownsend)
> Something in the DOM would be a far more valuable test.

Something in the DOM would not be easily put behind a pref :(

I see three ways:

1) We could land a string in FTL and keep the DTD as a backup as such:

```
<elem data-l10n-id="new-string">&old.string;</elem>
```

So that if anything goes wrong the old string will work.

2) I can take a string in FTL and apply it on top of DTD from JS behind a pref:

```
<elem>&old.string;</elem>

if (pref) {
  document.l10n.setAttributes(elem, "new-string");
}
```

3) I can move a DTD string to Properties and add FTL:

```
<elem></elem>

if (pref) {
  document.l10n.setAttributes(elem, "new-string");
} else {
  elem.textContent = stringBundle.getString("old-string");
}
```

but messing with moving DTD to properties feels off.

4) We can give up on the backup:

```
<elem data-l10n-id="new-string"></elem>
```


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


Which one would you prefer?
Flags: needinfo?(dtownsend)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

Does it look better? :)
Attachment #8910893 - Flags: feedback?(dtownsend)
Attachment #8910893 - Flags: feedback?(dtownsend)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review203388

::: browser/components/preferences/in-content/tests/browser_fluent.js:20
(Diff revision 15)
> +  ]);
> +
> +  let elem = doc.querySelector(
> +    `#contentProcessCount > menupopup > menuitem[value="${defaultProcessCount}"]`);
> +
> +  ok(elem.getAttribute("label"), msg);

Is this meant to be an is test rather than ok?
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

Yeppp! Thank you for catching it! Fixed :)
Attachment #8910893 - Flags: feedback?(dtownsend)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review203402
Attachment #8910893 - Flags: review+
Attachment #8910893 - Flags: feedback?(dtownsend)
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

So, we have a test machine that seems to be running without e10s - Windows 7 debug.

I had to add an early exit from the test that matches what we do in code: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/main.js#1185

Dave, can you confirm that it's an ok workaround?
Attachment #8910893 - Flags: feedback?(dtownsend)
Attachment #8910893 - Flags: feedback?(dtownsend) → feedback+
Comment on attachment 8910893 [details]
Bug 1402069 - Add a test string for the new localization API.

https://reviewboard.mozilla.org/r/182376/#review203738
Attachment #8910893 - Flags: review?(l10n) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/417011993e0c
Add a test string for the new localization API. r=mossop,Pike
https://hg.mozilla.org/mozilla-central/rev/417011993e0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1416449
@zibi
The localization file landed in browser/preferences/main.ftl, I would expect it in browser/chrome/browser/preferences/main.ftl

In l10n repos:
* expected: browser/chrome/browser/preferences/main.ftl
* actual: browser/browser/preferences/main.ftl 

I assume that's an error in the patch?
Flags: needinfo?(gandalf)
The lack of chrome is intentional, after all, we're not referencing it via the chrome:// protocol anymore.
Flags: needinfo?(gandalf)
(In reply to Axel Hecht [:Pike] from comment #75)
> The lack of chrome is intentional, after all, we're not referencing it via
> the chrome:// protocol anymore.

Is the double browser expected too? This is where the l10n file needs to live, which looks quite confusing
https://hg.mozilla.org/l10n-central/it/file/49a7aaa3fd4d/browser/browser/preferences/main.ftl
> Is the double browser expected too?

Yes. The first "browser" in this case is "browser source" (vs. "toolkit source" or "devtools source"), the second is "browser component".

This will make much more sense when we'll have more files.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: