Closed Bug 1435912 Opened 6 years ago Closed 6 years ago

Migrate Preferences::General XUL to .ftl

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.
59 bytes, text/x-review-board-request
jaws
: review+
flod
: review+
Details 
This is the first part of the bug 1424681. We're going to migrate as much of the main.xul and related to use FTL.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

This patch is ready for the initial feedback.

I was able to build it and it runs quite well for such an early attempt and the scope of the patch.

I left two classes of DTD entities in main.xul:

 - there's a class of DTD entities shared with browser/base/content/aboutDialog.xul in `aboutDialog.dtd`. I'm not sure if we want to split them into preferences.ftl and aboutDialog.ftl or load aboutDialog.ftl from preferences.xul
 - there are searchkeywords still in DTD. I can move them to FTL but I expect a lot of bikeshedding around it because they are collected from tons of different .DTD files from all around. It may be easier to tackle them in a separate bug.

There are two new dependencies that I know of:
 - syncBrand.ftl needs to be added
 - PLURAL built-in is used here so we need to support it well

Going to add dependencies.
Attachment #8948584 - Flags: feedback?(francesco.lodolo)
Depends on: 1432338
Depends on: 1415844
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review223792

::: browser/components/preferences/in-content/main.js:494
(Diff revision 1)
>      // Show a release notes link if we have a URL.
>      let relNotesLink = document.getElementById("releasenotes");
>      let relNotesPrefType = Services.prefs.getPrefType("app.releaseNotesURL");
>      if (relNotesPrefType != Services.prefs.PREF_INVALID) {
>        let relNotesURL = Services.urlFormatter.formatURLPref("app.releaseNotesURL");
> +      Services.console.logStringMessage(relNotesURL);

leftover - I'll remove in the next iteration.
The performance still looks good. I once again used :myk's mochitest and got:

mozilla-central:
TEST-PASS | browser/components/preferences/in-content/tests/browser_startup.js | Initial time to initialize prefs page: 398.08.
TEST-PASS | browser/components/preferences/in-content/tests/browser_startup.js | Mean time to initialize prefs page: 197.005.

with the patch:
TEST-PASS | browser/components/preferences/in-content/tests/browser_startup.js | Initial time to initialize prefs page: 355.04.
TEST-PASS | browser/components/preferences/in-content/tests/browser_startup.js | Mean time to initialize prefs page: 195.7026.


I tested multiple times and while the variance is there, the numbers for initial are consistently lower for Fluent. Not sure if that's significant, but at least it's reasonable to assume that we're not regressing in this test.
Depends on: 1435464
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

Now, with 100% more feedback from Flod! :)
Attachment #8948584 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review223936

::: browser/locales/en-US/browser/preferences/preferences.ftl:337
(Diff revision 2)
> +    .label = Show a touch keyboard when necessary
> +    .accesskey = k
> +
> +browsing-search-on-start-typing =
> +    .label = Search for text when you start typing
> +    .accesskey = x

We should likely pick a different accesskey, since we gave up on having unique accesskeys in preferences, and 'x' is not available in the string.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:9
(Diff revision 2)
>  
>  <!-- Note: each tab panel must contain unique accesskeys -->
>  
>  <!ENTITY generalTab.label                "General">
>  
>  <!ENTITY useCursorNavigation.label       "Always use the cursor keys to navigate within pages">

I think we can remove this label+ak

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:33
(Diff revision 2)
>  <!ENTITY alwaysSubmitCrashReports1.accesskey "c">
>  <!ENTITY crashReporterLearnMore.label    "Learn more">
>  
>  <!ENTITY networkTab.label                "Network">
>  
>  <!ENTITY networkProxy.label              "Network Proxy">

I think we can remove this label+ak

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:35
(Diff revision 2)
>  <!ENTITY networkTab.label                "Network">
>  
>  <!ENTITY networkProxy.label              "Network Proxy">
>  
> -<!ENTITY connectionSettingsLearnMore.label "Learn more">
>  <!ENTITY connectionSettings.label        "Settings…">

I think we can remove this label+ak
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

The FTL looks very good to me, thanks.

Something else might pop up when reviewing the actual migration, but I don't expect a lot of issues.

I wonder if there's a smart way to identify unused strings (I think Pike had stuff for DTDs, not sure how reusable it is).
Attachment #8948584 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review224472

::: browser/components/preferences/in-content/main.xul:23
(Diff revision 2)
>  
>  <hbox id="generalCategory"
>        class="subcategory"
>        hidden="true"
>        data-category="paneGeneral">
> -  <label class="header-name" flex="1">&paneGeneral.title;</label>
> +      <label class="header-name" flex="1" data-l10n-id="pane-general-title"></label>

nit: <label/> rather than <label></label>
Priority: -- → P3
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review226784

I know that you didn't ask for review yet, but I figured I'd do one anyway.

::: browser/components/preferences/in-content/main.js:890
(Diff revision 3)
>  
> -    if (tabs.length > 1)
> -      useCurrent.label = useCurrent.getAttribute("label2");
> -    else
> -      useCurrent.label = useCurrent.getAttribute("label1");
> +    // tabs.length can be set to `0` if no tabs can be used for home page.
> +    // In that case, we show a singular case, rather than plural
> +    document.l10n.setAttributes(useCurrent, "use-current-page", {
> +      tabCount: tabs.length < 2 ? 1 : tabs.length
> +    });

This shouldn't be using plurals, but two distinct strings, I think.

In Russian etc this would need to say "Use 1 current page" or something. Even in the case where it's 0.

I also wonder if we should just disable the button in the case of 0, as it's not doing anything in the case where there are no active tabs.

::: browser/components/preferences/in-content/main.xul:488
(Diff revision 3)
>  
> -  <label>&updateApplicationDescription.label;</label>
> +  <label data-l10n-id="update-application-description"></label>
>    <hbox align="center">
>      <vbox flex="1">
> -      <description>
> -        &updateApplication.version.pre;<label id="version" class="tail-with-learn-more" />&updateApplication.version.post;
> +      <description id="updateAppInfo" data-l10n-id="update-application-info">
> +        <html:a id="releasenotes" class="learnMore text-link" hidden="true"></html:a>

Were you able to test this? In particular the hidden part, and just our current impl of DOM overlays.

::: browser/components/preferences/in-content/preferences.xul
(Diff revision 3)
>        disablefastfind="true"
>        data-l10n-id="pref-page"
>        data-l10n-attrs="title">
>  
>    <link rel="localization" href="branding/brand.ftl"/>
> -  <link rel="localization" href="browser/preferences/main.ftl"/>

We're gonna migrate this from DTD/properties again?

::: browser/locales/en-US/browser/preferences/preferences.ftl:65
(Diff revision 3)
> +
> +## General Section
> +
> +startup-label = Startup
> +
> +# { -brand-short-name } will be 'Firefox Developer Edition',

We'll need to revisit this comment. Bug 1373244 is filed to create distinct profiles per channel.

::: browser/locales/en-US/browser/preferences/preferences.ftl:103
(Diff revision 3)
> +
> +use-current-page =
> +    .label =
> +        { $tabCount ->
> +            [one] Use Current Page
> +           *[other] Use Current Pages

Definitely not plurals here.

We could do [0] and [1], or just use distinct strings for 1 and 2....

::: browser/locales/en-US/browser/preferences/preferences.ftl:118
(Diff revision 3)
> +    .accesskey = R
> +
> +tabs-group-label = Tabs
> +
> +ctrl-tab-recently-used-order =
> +    .label = Ctrl+Tab cycles through tabs in recently used order

I find the description missing the point. I know, existing ever since bug 1116787 landed, but really, should we carry this forward?

(This switches on tab previews, the order in which they're shown is kinda unhelpful detail, imho)

::: browser/locales/en-US/browser/preferences/preferences.ftl:154
(Diff revision 3)
> +    .accesskey = i
> +
> +containers-disable-alert-title = Close All Container Tabs?
> +containers-disable-alert-desc =
> +    { $tabCount ->
> +        [one] If you disable Container Tabs now, { $tabCount } container tab will be closed. Are you sure you want to disable Container Tabs?

should we hard-code `one` instead of `1` in the string values here and below?

If we do, probably worth adding a comment? We should establish rules on that.

::: browser/locales/en-US/browser/preferences/preferences.ftl:198
(Diff revision 3)
> +    .label = Translate web content
> +    .accesskey = T
> +
> +# The <img/> will be replaced by the translation provider's logo
> +translation-options-attribution = Translations by <img/>
> +

No newline here to make the comment actually be a message comment and not a standalone comment.

::: browser/locales/en-US/browser/preferences/preferences.ftl:308
(Diff revision 3)
> +
> +performance-limit-content-process-option = Content process limit
> +    .accesskey = l
> +
> +performance-limit-content-process-enabled-desc = Additional content processes can improve performance when using multiple tabs, but will also use more memory.
> +performance-limit-content-process-disabled-desc = Modifying the number of content processes is only possible with multiprocess { -brand-short-name }. <label>Learn how to check if multiprocess is enabled</label>

<label> is XUL and shouldn't survive DOM overlays.

::: browser/locales/en-US/chrome/browser/preferences/tabs.dtd:3
(Diff revision 3)
>  <!-- 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/. -->

Remove the whole file if we stop using it?
Attachment #8948584 - Flags: review-
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review226798

::: browser/components/preferences/in-content/preferences.xul
(Diff revision 3)
>        disablefastfind="true"
>        data-l10n-id="pref-page"
>        data-l10n-attrs="title">
>  
>    <link rel="localization" href="branding/brand.ftl"/>
> -  <link rel="localization" href="browser/preferences/main.ftl"/>

Yes, that was the plan when we decided to go for a single file.

We want to get rid of main.ftl, and we can't migrate FTL to FTL.

::: browser/locales/en-US/browser/preferences/preferences.ftl:65
(Diff revision 3)
> +
> +## General Section
> +
> +startup-label = Startup
> +
> +# { -brand-short-name } will be 'Firefox Developer Edition',

I'm not sure that affects this comment. If it is, the string is likely going to be changed or dropped: 'Firefox' was called out explicitly (hard coded) because this is only used in DevEdition.

::: browser/locales/en-US/browser/preferences/preferences.ftl:103
(Diff revision 3)
> +
> +use-current-page =
> +    .label =
> +        { $tabCount ->
> +            [one] Use Current Page
> +           *[other] Use Current Pages

Good point, this should be two separate strings, not a plural string (see also bug 1437583).

::: browser/locales/en-US/browser/preferences/preferences.ftl:118
(Diff revision 3)
> +    .accesskey = R
> +
> +tabs-group-label = Tabs
> +
> +ctrl-tab-recently-used-order =
> +    .label = Ctrl+Tab cycles through tabs in recently used order

I don't think it's going to help us questioning existing strings in migrations, unless we really have to, e.g. to solve localization issues.

From that bug 1116787 (comment 2), it looks like switching browser.ctrlTab.previews gives you the tab switching in recent order, so the label seems correct.

::: browser/locales/en-US/browser/preferences/preferences.ftl:154
(Diff revision 3)
> +    .accesskey = i
> +
> +containers-disable-alert-title = Close All Container Tabs?
> +containers-disable-alert-desc =
> +    { $tabCount ->
> +        [one] If you disable Container Tabs now, { $tabCount } container tab will be closed. Are you sure you want to disable Container Tabs?

Could that affect migrations in any way?

As for the comments, I think we shouldn't add them, and it should be covered in documentation (I even started adding a note about it at some point). Unlike now, it's implicit that you can add the placeable for the number, and how it's called from the syntax, no need to search for the piece of code.
Bug 1417155 will be moving some new window / tab functionality to a separate pane. Neither are directly blocking each other but will likely have conflicts.

Similarly bug 1440219 will be changing the startup radio buttons to be a checkbox, but that bug is likely to land after this one.
See Also: → 1417155, 1440219
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review223936

> We should likely pick a different accesskey, since we gave up on having unique accesskeys in preferences, and 'x' is not available in the string.

What do you mean "not available"? It's in the word "text". If you want to change it, can you suggest a new one?
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review223936
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review226784

> This shouldn't be using plurals, but two distinct strings, I think.
> 
> In Russian etc this would need to say "Use 1 current page" or something. Even in the case where it's 0.
> 
> I also wonder if we should just disable the button in the case of 0, as it's not doing anything in the case where there are no active tabs.

I settled on using a `[1]` case explicitly after asking several people from ECMA402 about this. It seems that while in most cases 1/other is indeed the correct approach, there are cases like Japanese where this will either not use any selector or will have to use plural rules. (see https://linguistics.stackexchange.com/a/16907 for an example)

> Were you able to test this? In particular the hidden part, and just our current impl of DOM overlays.

Yup, confirmed to work.
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

The patch is completed.
I tested the runtime and migration against Italian and it looks good.

Time for a round of feedback.
Attachment #8948584 - Flags: feedback?(stas)
Attachment #8948584 - Flags: feedback?(l10n)
Attachment #8948584 - Flags: feedback?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> Comment on attachment 8948584 [details]
> Bug 1435912 - Migrate Preferences::General XUL part to the new Localization
> API.
> 
> https://reviewboard.mozilla.org/r/218006/#review223936
> 
> > We should likely pick a different accesskey, since we gave up on having unique accesskeys in preferences, and 'x' is not available in the string.
> 
> What do you mean "not available"? It's in the word "text". If you want to
> change it, can you suggest a new one?

MozReview ate my comment. Apparently I was looking at the wrong string when I made this comment, dropped the issue.

(I should be able to take a look at the patch on Monday, when I'm back from pto+travel)
No longer depends on: 1435464
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

Jared, I think it's also ready for your review.

This patch requires a patch from bug 1432338 to work.

The changes are fairly simple with two exceptions:
 - I moved the "disabled" state control over "Use current page(s)" fully to `setInputDisabledStates` and left `_updateUseCurrentButton` fully 
 - The `checkBrowserContainers` function has been refactored

I'd like to aim to land this in Fx 60 which means to get it in tree by March 1st.
Attachment #8948584 - Flags: review?(jaws)
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review228956

One general note: you start expressions (e.g. plurals) on a new line, while the migration serializer doesn't. For example

containers-disable-alert-ok-button = { $tabCount ->
        [one] Chiudi #S scheda contenitore
       *[other] Chiudi #S schede contenitore
    }

vs

containers-disable-alert-ok-button =
    { $tabCount ->
        [one] Close { $tabCount } Container Tab
       *[other] Close { $tabCount } Container Tabs
    }
    
I think it would be a good idea to use the same format the serializer produces.

::: browser/locales/en-US/browser/preferences/preferences.ftl:101
(Diff revision 6)
> +
> +home-page-label = Home page
> +
> +# Since the label of this button does not use the actual number,
> +# in most cases you just need to provide a separate variant for the singular case.
> +use-current-pages =

I see two issues here:
* I have no idea how Pontoon is going to expose this string for migrated locales, given the plural on the label and an additional attribute.
* What happens for a new locale? Are they going to see 1/other or a standard plural form, e.g. 6 forms for Arabic?

::: browser/locales/en-US/browser/preferences/preferences.ftl:219
(Diff revision 6)
> +    .accesskey = v
> +
> +download-choose-folder =
> +    .label =
> +        { PLATFORM() ->
> +            [macosx] Choose…

macos

::: browser/locales/en-US/browser/preferences/preferences.ftl:224
(Diff revision 6)
> +            [macosx] Choose…
> +           *[other] Browse…
> +        }
> +    .accesskey =
> +        { PLATFORM() ->
> +            [macosx] e

macos

::: browser/locales/en-US/browser/preferences/preferences.ftl:327
(Diff revision 6)
> +browsing-use-smooth-scrolling =
> +    .label = Use smooth scrolling
> +    .accesskey = m
> +
> +browsing-use-onscreen-keyboard =
> +    .label = Always use the cursor keys to navigate within pages

Show a touch keyboard when necessary

::: browser/locales/en-US/browser/preferences/preferences.ftl:331
(Diff revision 6)
> +browsing-use-onscreen-keyboard =
> +    .label = Always use the cursor keys to navigate within pages
> +    .accesskey = c
> +
> +browsing-use-cursor-navigation =
> +    .label = Show a touch keyboard when necessary

Always use the cursor keys to navigate within pages

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:36
(Diff revision 6)
> +                        FTL.Identifier('label'),
> +                        REPLACE(
> +                            'browser/chrome/browser/preferences/main.dtd',
> +                            'separateProfileMode.label',
> +                            {
> +                                '&brandShortName;' : MESSAGE_REFERENCE('-brand-short-name')

nit: space before :

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:466
(Diff revision 6)
> +                )
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('containers-disable-alert-desc'),
> +                value=PLURALS(
> +                    'browser/chrome/browser/preferences/preferences.properties',

This migration fails, because the string doesn't use the standard placeholder for plurals (#S vs #1). So we end up without the variable in FTL.

That's an error in the original Containers strings that was never fixed.

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:472
(Diff revision 6)
> +                    'disableContainersMsg',
> +                    EXTERNAL_ARGUMENT('tabCount'),
> +                )
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('containers-disable-alert-ok-button'),

Same issue here with #S

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:707
(Diff revision 6)
> +                                        expression=FTL.CallExpression(
> +                                            callee=FTL.Identifier('PLATFORM')
> +                                        ),
> +                                        variants=[
> +                                            FTL.Variant(
> +                                                key=FTL.VariantName('macosx'),

macos

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:739
(Diff revision 6)
> +                                        expression=FTL.CallExpression(
> +                                            callee=FTL.Identifier('PLATFORM')
> +                                        ),
> +                                        variants=[
> +                                            FTL.Variant(
> +                                                key=FTL.VariantName('macosx'),

macos

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:902
(Diff revision 6)
> +                        '&brandShortName;': MESSAGE_REFERENCE('-brand-short-name')
> +                    },
> +                )
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('update-application-info'),

This migration is missing the `<a>What’s new</a>` part (releaseNotes.link)
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review228972
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

There are a few strings to fix, the main questions are the "Use current page(s)" string in Pontoon, and the Containers plurals.
Attachment #8948584 - Flags: feedback?(francesco.lodolo) → feedback-
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review228956

Let's fix the serializer. Zibi has been using the preferred formatting here and I'd like to keep it that way.
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

My flu-brain isn't going to catch more than flod, clearing the feedback request.
Attachment #8948584 - Flags: feedback?(l10n)
Depends on: 1441105
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review228956

> This migration fails, because the string doesn't use the standard placeholder for plurals (#S vs #1). So we end up without the variable in FTL.
> 
> That's an error in the original Containers strings that was never fixed.

I stand corrected (thanks to Stas): PLURALS() doesn't know anything about the placeholder format.
https://github.com/projectfluent/python-fluent/blob/master/tests/migrate/test_context_real_examples.py#L145

This migration is missing a replacement (you also need to import REPLACE_IN_TEXT)

```
lambda text: REPLACE_IN_TEXT(
    text,
    {
        '#S': EXTERNAL_ARGUMENT('tabCount')
    })
```

> This migration is missing the `<a>What’s new</a>` part (releaseNotes.link)

I guess this should work

```
FTL.TextElement(' <a>'),
COPY(
    'browser/chrome/browser/aboutDialog.dtd',
    'releaseNotes.link'
),
FTL.TextElement('</a>')
```
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229048

::: browser/locales/en-US/browser/preferences/preferences.ftl:13
(Diff revision 6)
>      .label = Only when using Tracking Protection
>  do-not-track-option-always =
>      .label = Always
>  
>  pref-page =
>      .title = { PLATFORM() ->

Since we've updated the serializer in python-fluent, we should update this to

```
pref-page =
    .title = 
        { PLATFORM() ->
            [windows] Options
           *[other] Preferences
        }
```
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229074

::: browser/locales/en-US/browser/preferences/preferences.ftl:63
(Diff revision 6)
>  revert-no-restart-button = Revert
>  restart-later = Restart Later
> +
> +## General Section
> +
> +startup-label = Startup

I noticed that you usually drop the word "label" if the original ID used to be "foo.label". Why keep it here? Perhaps rename this to `startup-header`?
Attachment #8948584 - Flags: feedback?(stas)
(Removed the f? request on me because of flod's f-.)
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

Flod - applied your feedback leaving open the question about Pontoon support. Can you take another look to verify if everything else looks good?
Attachment #8948584 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229144

Not sure if we want to distinguish -title and -header (I choose it based on how they appear in prefs)

::: browser/locales/en-US/browser/preferences/preferences.ftl:98
(Diff revision 7)
> +    .label = Show your windows and tabs from last time
> +
> +disable-extension =
> +    .label = Disable Extension
> +
> +home-page-label = Home page

Following stas' advice, let's avoid using -label where possible: home-page-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:118
(Diff revision 7)
> +
> +restore-default =
> +    .label = Restore to Default
> +    .accesskey = R
> +
> +tabs-group-label = Tabs

tabs-group-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:170
(Diff revision 7)
> +    }
> +containers-disable-alert-cancel-button = Keep enabled
> +
> +## General Section - Language & Appearance
> +
> +language-and-appearance-label = Language and Appearance

language-and-appearance-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:172
(Diff revision 7)
> +
> +## General Section - Language & Appearance
> +
> +language-and-appearance-label = Language and Appearance
> +
> +fonts-and-colors-label = Fonts & Colors

fonts-and-colors-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:174
(Diff revision 7)
> +
> +language-and-appearance-label = Language and Appearance
> +
> +fonts-and-colors-label = Fonts & Colors
> +
> +default-font-label = Default font

default-font

::: browser/locales/en-US/browser/preferences/preferences.ftl:176
(Diff revision 7)
> +
> +fonts-and-colors-label = Fonts & Colors
> +
> +default-font-label = Default font
> +    .accesskey = D
> +default-font-size-label = Size

default-font-size

::: browser/locales/en-US/browser/preferences/preferences.ftl:187
(Diff revision 7)
> +
> +colors-settings =
> +    .label = Colors…
> +    .accesskey = C
> +
> +language-label = Language

language-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:189
(Diff revision 7)
> +    .label = Colors…
> +    .accesskey = C
> +
> +language-label = Language
> +
> +choose-language-label = Choose your preferred language for displaying pages

choose-language-description

::: browser/locales/en-US/browser/preferences/preferences.ftl:209
(Diff revision 7)
> +    .label = Check your spelling as you type
> +    .accesskey = t
> +
> +## General Section - Files and Applications
> +
> +files-and-applications-label = Files and Applications

files-and-applications-title

::: browser/locales/en-US/browser/preferences/preferences.ftl:211
(Diff revision 7)
> +
> +## General Section - Files and Applications
> +
> +files-and-applications-label = Files and Applications
> +
> +download-label = Downloads

download-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:233
(Diff revision 7)
> +
> +download-always-ask-where =
> +    .label = Always ask you where to save files
> +    .accesskey = A
> +
> +applications-label = Applications

applications-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:248
(Diff revision 7)
> +
> +applications-action-column =
> +    .label = Action
> +    .accesskey = A
> +
> +drm-content-label = Digital Rights Management (DRM) Content

drm-content-header

::: browser/locales/en-US/browser/preferences/preferences.ftl:256
(Diff revision 7)
> +    .label = Play DRM-controlled content
> +    .accesskey = P
> +
> +play-drm-content-learn-more = Learn more
> +
> +update-application-label = { -brand-short-name } Updates

update-application-title

::: browser/locales/en-US/browser/preferences/preferences.ftl:290
(Diff revision 7)
> +    .label = Automatically update search engines
> +    .accesskey = e
> +
> +## General Section - Performance
> +
> +performance-label = Performance

performance-title

::: browser/locales/en-US/browser/preferences/preferences.ftl:317
(Diff revision 7)
> +performance-default-content-process-count =
> +    .label = { $num } (default)
> +
> +## General Section - Browsing
> +
> +browsing-label = Browsing

browsing-title

::: browser/locales/en-US/browser/preferences/preferences.ftl:341
(Diff revision 7)
> +    .label = Search for text when you start typing
> +    .accesskey = x
> +
> +## General Section - Proxy
> +
> +network-proxy-label = Network Proxy

network-proxy-title
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229192

Still waiting for Pontoon to see if we can support the "Use current page(s)" string.

::: browser/locales/en-US/browser/preferences/preferences.ftl:260
(Diff revision 8)
> +
> +update-application-title = { -brand-short-name } Updates
> +
> +update-application-description = Keep { -brand-short-name } up to date for the best performance, stability, and security.
> +
> +update-application-info = Version { $version }. <a>What's new</a>

Just noticed while checking the migration: you added a period after the version that it's not in the source string.

This should be 

```
update-application-info = Version { $version } <a>What's new</a>
```

::: python/l10n/fluent_migrations/bug_1435912_preferences_general_xul.py:927
(Diff revision 8)
> +                        'browser/chrome/browser/preferences/advanced.dtd',
> +                        'updateApplication.version.post'
> +                    ),
> +                    FTL.TextElement(' <a>'),
> +                    COPY(
> +                        'browser/locales/en-US/chrome/browser/aboutDialog.dtd',

Not sure why you changed the snippet, but this doesn't work ;-)

It should be `browser/chrome/browser/aboutDialog.dtd`
Attachment #8948584 - Flags: review?(francesco.lodolo)
Thank you flod! Applied the feedback.

As far as I'm aware we're waiting for only one question to hash out.

Jared - The patch is ready for review
(In reply to Francesco Lodolo [:flod] from comment #24)
> ::: browser/locales/en-US/browser/preferences/preferences.ftl:101
> (Diff revision 6)
> > +
> > +home-page-label = Home page
> > +
> > +# Since the label of this button does not use the actual number,
> > +# in most cases you just need to provide a separate variant for the singular case.
> > +use-current-pages =
> 
> I see two issues here:
> * I have no idea how Pontoon is going to expose this string for migrated
> locales, given the plural on the label and an additional attribute.
> * What happens for a new locale? Are they going to see 1/other or a standard
> plural form, e.g. 6 forms for Arabic?
> 

To summarize testing:
* Pontoon seems to not be importing a plural string with special case, asked Matjaz to investigate on IRC
https://github.com/mozilla-l10n/pontoon-ftl/commit/1fe90aa969c9ee5821db58c8b11c140cd4f84e54
* A new locale is going to get all plural forms (6 for Arabic). We could special case something like "has 1/other" AND "all forms don't use the variables" to only show 1 and other in Pontoon, but that's not there now, and probably need a larger discussion
(In reply to Francesco Lodolo [:flod] from comment #40)
> (In reply to Francesco Lodolo [:flod] from comment #24)
> > ::: browser/locales/en-US/browser/preferences/preferences.ftl:101
> > (Diff revision 6)
> > > +
> > > +home-page-label = Home page
> > > +
> > > +# Since the label of this button does not use the actual number,
> > > +# in most cases you just need to provide a separate variant for the singular case.
> > > +use-current-pages =
> > 
> > I see two issues here:
> > * I have no idea how Pontoon is going to expose this string for migrated
> > locales, given the plural on the label and an additional attribute.
> > * What happens for a new locale? Are they going to see 1/other or a standard
> > plural form, e.g. 6 forms for Arabic?
> > 
> 
> To summarize testing:
> * Pontoon seems to not be importing a plural string with special case, asked
> Matjaz to investigate on IRC
> https://github.com/mozilla-l10n/pontoon-ftl/commit/
> 1fe90aa969c9ee5821db58c8b11c140cd4f84e54

There was a syntax error (closing curly brace not indented). Fixed that, the import worked correctly.

> * A new locale is going to get all plural forms (6 for Arabic). We could
> special case something like "has 1/other" AND "all forms don't use the
> variables" to only show 1 and other in Pontoon, but that's not there now,
> and probably need a larger discussion
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229482

Strings and migration look good. There is still an open question about the "Use current page(s)" string (if we should drop it while we fix Pontoon), but I don't think this should block review here.
Attachment #8948584 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8948584 [details]
Bug 1435912 - Migrate Preferences::General XUL part to the new Localization API.

https://reviewboard.mozilla.org/r/218006/#review229644

r+ with the following changes:

Can you please file a bug to update the test at /browser/base/content/test/static/browser_misused_characters_in_strings.js to check .ftl files?

::: browser/components/preferences/in-content/main.js:775
(Diff revision 9)
> +          // Special case for current_page to disable it if tabCount is 0
> +          let isDisabled = pref == "pref.browser.homepage.disable_button.current_page" ?
> +            tabCount < 1 || Preferences.get(pref).locked || isControlled :
> +            Preferences.get(pref).locked || isControlled;

Can you factor out the current_page part so that we don't have to duplicate the rest of each expression?

```js
let isDisabled = Preferences.get(pref).locked || isControlled;

if (pref == "pref.browser.disable_button.current_page") {
  isDisabled = isDisabled || tabCount < 1;
}
```

::: browser/components/preferences/in-content/main.js:921
(Diff revision 9)
>        .getAttribute("windowtype") == "navigator:browser") {
>        // We should only include visible & non-pinned tabs
>  
>        tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
>        tabs = tabs.filter(this.isNotAboutPreferences);
> +      // XXX: Fix tabbrowser to report tab.closing before it blurs it

Can you file a bug for this?

::: browser/components/preferences/in-content/main.xul:366
(Diff revision 9)
>      <hbox align="center" flex="1">
>        <checkbox id="translate" preference="browser.translation.detectLanguage"
> -                label="&translateWebPages.label;." accesskey="&translateWebPages.accesskey;"
> +                data-l10n-id="translate-web-pages"
>                  onsyncfrompreference="return gMainPane.updateButtons('translateButton',
>                                                'browser.translation.detectLanguage');"/>
> -      <hbox id="bingAttribution" hidden="true" align="center">
> +      <hbox id="bingAttribution" hidden="true">

Please put the align=center back, as well as the <separator>

You are regressing bug 1437967
Attachment #8948584 - Flags: review?(jaws) → review+
> Can you please file a bug to update the test at /browser/base/content/test/static/browser_misused_characters_in_strings.js to check .ftl files?

Filed bug 1441636

> 
Can you file a bug for this?

Filed bug 1441637
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ba30e000664
Migrate Preferences::General XUL part to the new Localization API. r=flod,jaws
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37d0425705d1
Backed out 2 changesets (bug 1435912, bug 1441733) for for Fluent in Preferences::General::XUL bc failures. a=backout on a CLOSED TREE
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b1228043619
Migrate Preferences::General XUL part to the new Localization API. r=flod,jaws
https://hg.mozilla.org/mozilla-central/rev/9b1228043619
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1444564
Depends on: 1444969
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: