This is the second part of the bug 1424681. We're going to migrate Preferences::General JS code to use FTL.

The main part of the patch here will be a refactor of the actions menu which uses a lot of synchronous l10n calls including for sorting - https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/components/preferences/in-content/main.js#1886 The other item is going to be refactoring handler descriptions which are also sync - https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/components/preferences/in-content/main.js#3164 The rest is fairly small and trivial. I'm going to take the extensionController code to a separate bug/patch since it's substantial.

Comment on attachment 8951090 [details] Bug 1435915 - Migrate Preferences::General JS part to the new Localization API. https://reviewboard.mozilla.org/r/220342/#review226294 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/preferences/in-content/main.js:2690 (Diff revision 1) > + downloadFolder.removeAttribute("data-l10n-id"); > downloadFolder.label = this._getDisplayNameOfFile(currentDirPref.value); > iconUrlSpec = fph.getURLSpecFromFile(currentDirPref.value); > } else if (folderIndex == 1) { > // 'Downloads' > - downloadFolder.label = bundlePreferences.getString("downloadsFolderName"); > + document.l10n.setAttributes(downloadFolder, "download-folder-name-downloads") Error: Missing semicolon. [eslint: semi] ::: browser/components/preferences/in-content/main.js:2694 (Diff revision 1) > // 'Downloads' > - downloadFolder.label = bundlePreferences.getString("downloadsFolderName"); > + document.l10n.setAttributes(downloadFolder, "download-folder-name-downloads") > iconUrlSpec = fph.getURLSpecFromFile(await this._indexToFolder(1)); > } else { > // 'Desktop' > - downloadFolder.label = bundlePreferences.getString("desktopFolderName"); > + document.l10n.setAttributes(downloadFolder, "download-folder-name-desktop") Error: Missing semicolon. [eslint: semi]

Comment on attachment 8951090 [details] Bug 1435915 - Migrate Preferences::General JS part to the new Localization API. https://reviewboard.mozilla.org/r/220342/#review232260 ::: browser/locales/en-US/browser/preferences/preferences.ftl:308 (Diff revision 2) > + .typeDescription = Podcast > +applications-type-pdf = > + .typeDescription = Portable Document Format (PDF) > + > +applications-type-web-feed-with-type = > + .typeDescription = Web Feed ({ $type }) What is `{ type }`? Can we use `applications-type-description-with-type` for these 4 strings?

Paolo's work unblocked most of this patch, but I'm still hitting the wall on one weird bug (XBL related around menupopups), and one functional issue. Namely, the code we're refactoring builds this fragment: ``` <richlistitem> <hbox flex="1" equalsize="always"> <hbox class="typeContainer" flex="1" align="center"> <image class="typeIcon" width="16" height="16" src="moz-icon://goat?size=16"/> <label class="typeDescription" flex="1" crop="end"/> </hbox> <hbox class="actionContainer" flex="1" align="center"> <image class="actionIcon" width="16" height="16"/> <label class="actionDescription" flex="1" crop="end"/> </hbox> <hbox class="actionsMenuContainer" flex="1"> <menulist class="actionsMenu" flex="1" crop="end" selectedIndex="1"> <menupopup/> </menulist> </hbox> </hbox> </richlistitem> ``` The `actionContainer` and `actionMenuContainer` are displayed interchangely. When the line is active the menu is displayed, otherwise the label. The behavior is to copy the currently selected menuitem as the `.actionContainer > label`, which in the Fluent world means that we'd like to set the l10n-id of the selected menuitem on that label, but unfortunately the shapes of the elements differ. menuitem requires an attribute "label", while label requires either textContent or an attribute "value". This means that I can either do some trickery like localize menuitems and then fetch their translations and apply onto the label (plus some mutation observer?), or I should wait for Dynamic References in Fluent [0] and use them for the label. I think I'd prefer to do the latter. [0] https://github.com/projectfluent/fluent/issues/80

Interesting use-case. In the future I'm hoping that it will be possible to solve it by using a web component which encapsulates the behavior you described and normalizes the shape of the Fluent message for both scenarios. Can you explain how Fluent dynamic reference would help solve this? Since there's already JS logic involved, would it be possible to get the data-l10n-id of the menuitem, s/-menuitem/-label/ it and set the result of the replacement (now with -label) on the label element? The FTL would look like the following: applications-action-always-ask-label = Always ask applications-action-always-ask-menuitem = .label = {applications-action-always-ask-label} Or: applications-action-always-ask-label = .value = Always ask applications-action-always-ask-menuitem = .label = {applications-action-always-ask-label.value}

Comment on attachment 8982068 [details] Bug 1435915 - Migrate Preferences::General JS to Fluent. I got something that doesn't not work, but damn, this is a very aged code it seems. The patch moves from .properties to .ftl, but also does several other things to make the Fluent integration possible: - I removed the whole duality of actionDescription being calculated separately. Instead I just set a MutationObserver which reacts to label changes in actionsMenu and project them onto actionDescription. - I moved the description/menu switching to CSS - I reorganized filtering/sorting to operate on the DOM elements (which are localized) allowing us to not carry localization in our HandlerInfo objects. The patch seems to work, but I feel like I'm digging in a complicated codebase that I may not fully understand, so I'd like someone who knows this code more to take a look and verify if: - this is a good refactor - should we do this now? (vs. wait for some bigger modularization refactor to get this code out of a huge file?) - am I missing something? - anything else? Jared - if there's someone better suited for this feedback round, feel free to reassign :)

Comment on attachment 8982068 [details] Bug 1435915 - Migrate Preferences::General JS to Fluent. https://reviewboard.mozilla.org/r/248104/#review256708 ::: browser/components/preferences/in-content/main.js:1479 (Diff revision 1) > - this.rebuildActionsMenu(); > - handlerListItem.showActionsMenu = true; > + //XXX: I think this is not needed anymore? > + // if (handlerListItem) { > + // this.rebuildActionsMenu(); > + // handlerListItem.showActionsMenu = true; > + // } Why do you think this is not needed? This was added by bug 1459556 which might give you more background. ::: browser/components/preferences/in-content/main.js:1579 (Diff revision 1) > - _rebuildView() { > - let lastSelectedType = this.selectedHandlerListItem && > - this.selectedHandlerListItem.handlerInfoWrapper.type; > - this.selectedHandlerListItem = null; > + async _rebuildView() { > + //XXX: Here, instead of removing all entries and rebuilding from scratch, > + // it would be cool to only remove entries that are no longer in visibleTypes, > + // and only add entries that are in visibleTypes but are missing in this._list. > + // > + // I'm not sure how to link from on to another tho. You can get the HandlerListItem for a given node by calling the static method `HandlerListItem.forNode(node);`. Instead of clearing all the nodes first, you could gather all the HandlerListItems for the currently displayed childNodes. From a handlerListItem, you can get the visibleType from the handlerInfoWrapper property. You can then remove the all HandlerListItem nodes whose handlerInfoWrapper isn't in visibleTypes, while removing items from visibleTypes when their HandlerListItem node was already present. You will then be left with a visibleTypes that only contains types that need to be added to the DOM. `this._sortView(this._list)` will take care of fixing any items that are out of order. ::: browser/components/preferences/in-content/main.js:2676 (Diff revision 1) > * to help users distinguish between those types. > */ > get typeDescription() { > if (this.disambiguateDescription) { > - return gMainPane._prefsBundle.getFormattedString( > - "typeDescriptionWithType", [this.description, this.type]); > + const desc = this.description; > + // Once Fluent supports dynamic references, this should be switched Is there a bug number you can link for this? ::: browser/themes/shared/incontentprefs/preferences.inc.css:276 (Diff revision 1) > +#handlersView > richlistitem > hbox > .actionContainer { > + display: -moz-box; > +} > + > +#handlersView > richlistitem > hbox > .actionsMenuContainer { > + display: none; > +} > + > +#handlersView > richlistitem[selected="true"] > hbox > .actionContainer { > + display: none; > +} > + > +#handlersView > richlistitem[selected="true"] > hbox > .actionsMenuContainer { > + display: -moz-box; > +} I think you can delete the first two rules, and then for the last rule you can change that to: ```css #handlersView > richlistitem:not([selected="true"]) > hbox > .actionsMenuContainer { display: none; } ``` Then you can combine the two selectors in to one ruleset since they set the same property-value pair.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/33b58a35cd4e Remove preferences.properties. r=fluent-reviewers,Gijs,flod

Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5daa02293ca8 Remove preferences.properties. r=fluent-reviewers,Gijs,flod