Closed Bug 1438375 Opened 6 years ago Closed 6 years ago

Refactor "extensionControlled" Preferences code to use Fluent

Categories

(Firefox :: Settings UI, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(3 files, 1 obsolete file)

extensionControlled code is spanning across chrome and General sections and uses quite a lot of string interpolation.

It makes sense to get it out of the way before I tackle the rest of General JS code.
Assignee: nobody → gandalf
Blocks: 1424681
Status: NEW → ASSIGNED
POC of the patch, just to help my team assess the type of strings that we'll land here.
Priority: -- → P3
Comment on attachment 8951515 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/220816/#review232252

::: browser/components/preferences/in-content/extensionControlled.js:126
(Diff revision 4)
>  
>    return !!addon;
>  }
>  
> -function getControllingExtensionFragment(stringId, addon, ...extraArgs) {
> -  let msg = document.getElementById("bundlePreferences").getString(stringId);
> +function settingNameToL10nID(settingName) {
> +  // 1) We replace NewTabURL to new-tab-uRL

to new-tab-url

More in general: does it make sense to do all this work to generate the l10n-id programmatically, when it's going to break as soon as we need to introduce a new rev for a string?

Why not just use a mapping? It's already used for IDs
https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/extensionControlled.js#
Attachment #8951515 - Attachment is obsolete: true
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

Bob - since you're the last person who worked on this (bug 1429593) I'd like to ask you for feedback on this refactor.

Additionally - it seems that the `showEnableExtensionMessage` codepath is not working correctly. I can verify that when I enable tp-link test extension I see the extension related string in Prefs, and when I disable the extension I can see the right string in the DOM, but it's not visible because the <description> element which should hold the enable extension message is hidden="true".

I can fix it alongside this patch if you can suggest where.
Attachment #8968682 - Flags: feedback?(bob.silverberg)
Attachment #8968682 - Flags: feedback?(bob.silverberg)
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

I'm actually no longer working on WebExtensions, but I think mstriemer is as familiar with the code as me, so I will forward my feedback request to him.
Attachment #8968682 - Flags: feedback?(bob.silverberg) → feedback?(mstriemer)
Passing by note: several comments in the FTL need an update (reference to %S, or <image> in the section one).
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/237358/#review244296

::: browser/components/preferences/in-content/extensionControlled.js:116
(Diff revision 2)
>    }
>  
>    return !!addon;
>  }
>  
> -function getControllingExtensionFragment(stringId, addon, ...extraArgs) {
> +function settingNameToL10nID(settingName) {

I'm not sure I like converting `settingName` to be a string id. Can it get passed in to `handleControllingExtension` instead?

Alternatively, adding a mapping of `settingName` to string id similar to `extensionControlledContentIds` would work too.
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

These changes look good to me. You'll likely need sign off from jaws, too.

Thanks for updating this!
Attachment #8968682 - Flags: feedback?(mstriemer) → feedback+
Thank you Mark! Updated the patch to your feedback and requesting reviews.
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/237358/#review243222

Looks good, thanks!
Attachment #8968682 - Flags: review?(francesco.lodolo) → review+
Attachment #8968682 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/237358/#review245100

These updates from the last patch look good to me. A small nit and a bit of a process question here.

::: browser/components/preferences/in-content/extensionControlled.js:125
(Diff revision 5)
>    }
>  
>    return !!addon;
>  }
>  
> -function getControllingExtensionFragment(stringId, addon, ...extraArgs) {
> +function settingNameToL10nID(settingName) {

nit: the other uses of "id" in this file use "Id". So this should be `settingNameToL10Id` for consistency.

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:111
(Diff revision 5)
>    await waitForMessageShown("browserHomePageExtensionContent");
>  
>    // The homepage has been set by the extension, the user is notified and it isn't editable.
>    let controlledLabel = controlledContent.querySelector("description");
>    is(homepagePref(), extensionHomepage, "homepage is set by extension");
> -  // There are two spaces before "set_homepage" because it's " <image /> set_homepage".
> +  is(doc.l10n.getAttributes(controlledLabel).id,

This used to test that the string was translated and getting the icon and add-on name added properly.

Can they stay the way they were or is this the preferred testing method now?
> Can they stay the way they were or is this the preferred testing method now?

Testing for the particular value out of translation is an antipattern: https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html#testing

But we can test for:
 - l10n-id being correct
 - l10n-attrs being correct
 - icon being present in the element's childNodes

which should basically cover the same with less assumptions about l10n output. I'll update the patch.
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/237358/#review245310

::: browser/components/preferences/in-content/extensionControlled.js:138
(Diff revision 6)
> +function setControllingExtensionDescription(elem, addon, settingName) {
> +  // Remove the old content from the description.
> +  while (elem.firstChild) {
> +    elem.firstChild.remove();
> +  }
> +  let image = document.createElementNS("http://www.w3.org/1999/xhtml", "img");

XUL and HTML images have some subtle differences in how they behave wrt sizing and so on. For our builtin icons it probably won't matter, but I wonder if we should specify width/height for the add-on ones, just to be on the safe side?

::: browser/components/preferences/in-content/extensionControlled.js:145
(Diff revision 6)
> -  addonBit.appendChild(document.createTextNode(" " + addon.name));
> -  return BrowserUtils.getLocalizedFragment(document, msg, addonBit, ...extraArgs);
> +  document.l10n.setAttributes(elem, l10nId, {
> +    name: addon.name
> +  });

My understanding is that the l10n.setAttributes stuff is async. Given that the callers are async anyway, can we just make this wait on the localization having completed?

... though it also seems like almost nobody awaits handleControllingExtension, so maybe there's no point...

::: browser/components/preferences/in-content/extensionControlled.js:181
(Diff revision 6)
>    let elements = getControllingExtensionEls(settingName);
>  
>    elements.button.hidden = true;
>    elements.section.classList.add("extension-controlled-disabled");
> -  let icon = url => {
> -    let img = document.createElement("image");
> +
> +  elements.description.innerHTML = "";

Nit: Using textContent is faster (and yeah, I agree that's counterintuitive)

::: browser/components/preferences/in-content/extensionControlled.js:182
(Diff revision 6)
>  
>    elements.button.hidden = true;
>    elements.section.classList.add("extension-controlled-disabled");
> -  let icon = url => {
> -    let img = document.createElement("image");
> +
> +  elements.description.innerHTML = "";
> +  elements.description.removeAttribute("data-l10n-id");

Can you elaborate on why this is necessary?

::: browser/components/preferences/in-content/main.js:904
(Diff revision 6)
> -      BrowserUtils.getLocalizedFragment(
> -        document,
> -        this._prefsBundle.getString("connectionDesc.label"),
> -        this._brandShortName);
>      let description = document.getElementById("connectionSettingsDescription");
> +    let controllingExtension = await getControllingExtension(PREF_SETTING_TYPE, PROXY_KEY);

Super nit-pickety nit: looks like this could stay above the declaration of `description` to keep blame.

::: browser/components/preferences/in-content/main.js:909
(Diff revision 6)
> -    // Remove the old content from the description.
> +      // Remove the old content from the description.
> -    while (description.firstChild) {
> +      while (description.firstChild) {
> -      description.firstChild.remove();
> +        description.firstChild.remove();
> -    }
> +      }
> -
> +      document.l10n.setAttributes(description, "network-proxy-connection-description");

Would it be possible to just allow `setControllingExtensionDescription` to take `null` for the controllingExtension, and cope with that? That would simplify this code a little.
Attachment #8968682 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8968682 [details]
Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent.

https://reviewboard.mozilla.org/r/237358/#review245310

> XUL and HTML images have some subtle differences in how they behave wrt sizing and so on. For our builtin icons it probably won't matter, but I wonder if we should specify width/height for the add-on ones, just to be on the safe side?

I agree, but I think this is handled by https://searchfox.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#183 . Lmk if that's not enough, and we can fix in a follow up.

> My understanding is that the l10n.setAttributes stuff is async. Given that the callers are async anyway, can we just make this wait on the localization having completed?
> 
> ... though it also seems like almost nobody awaits handleControllingExtension, so maybe there's no point...

The `setAttributes` itself is sync. It just applies the attributes.
What happens then is that MutationObserver is called, and it collects all mutations into a pending Set waiting for the next animation frame.

So making this function wait for localization to apply would basically mean `return new Promise((resolve) => window.requestAnimationFrame(resolve));`. I'm not sure if it's worth it since its rare that any code should wait for the l10n to be applied. Let me know if you think we should and I can add it in a follow up.

> Can you elaborate on why this is necessary?

Added comments. Lmk if that works.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0f3ab5ff7447ff304b9a7f11f06064b37bb021d4 -d de6af6a44e14: rebasing 460401:0f3ab5ff7447 "Bug 1438375 - Refactor "extensionControlled" Preferences code to use Fluent. r=flod,Gijs" (tip)
merging browser/components/preferences/in-content/home.js
merging browser/components/preferences/in-content/main.js
merging browser/locales/en-US/browser/preferences/preferences.ftl
warning: conflicts while merging browser/components/preferences/in-content/home.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb836a045f20
Refactor "extensionControlled" Preferences code to use Fluent. r=flod,Gijs
https://hg.mozilla.org/mozilla-central/rev/fb836a045f20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Backed out for failing browser chrome at browser/components/preferences/in-content/tests/browser_extension_controlled.js

and also browser/base/content/test/static/browser_misused_characters_in_strings.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4da66989752079b16519ad896cdf9267f4f17dac

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=175675361&repo=autoland&lineNumber=3319

https://treeherder.mozilla.org/logviewer.html#?job_id=175676318&repo=autoland&lineNumber=1951

Backout: https://hg.mozilla.org/integration/autoland/rev/8801ee790cc2ae7bbb39613515365ca24f30abaa
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
Target Milestone: Firefox 61 → ---
Backout by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/42c89029f1ad
Backed out changeset fb836a045f20 for browser chrome failures at browser/components/preferences/in-content/tests/browser_extension_controlled.js a=backout
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc389188dc5
Backed out changeset fb836a045f20 for browser chrome failres at browser/components/preferences/in-content/tests/browser_extension_controlled.js
(In reply to Andreea Pavel [:apavel] from comment #33)
> and also
> browser/base/content/test/static/browser_misused_characters_in_strings.js

How can this be, given this patch is only removing strings?
07:23:27    ERROR -  51 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_misused_characters_in_strings.js | jar:file:///Z:/task_1524726398/build/application/firefox/browser/omni.ja!/chrome/en-US/locale/en-US/devtools/client/netmonitor.properties with key=netmonitor.context.importHar has a misused ellipsis. Strings with an ellipsis should use the Unicode … character instead of three periods. -

That failure is clearly from bug 1456129
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/2885667acedb
Fix the broken backout of bug 1438375 to reopen the CLOSED TREE.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ece4329ec73
Refactor "extensionControlled" Preferences code to use Fluent. r=flod,Gijs
https://hg.mozilla.org/mozilla-central/rev/3ece4329ec73
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.