Closed
Bug 1438375
Opened 6 years ago
Closed 6 years ago
Refactor "extensionControlled" Preferences code to use Fluent
Categories
(Firefox :: Settings UI, enhancement, P3)
Firefox
Settings UI
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 | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
POC of the patch, just to help my team assess the type of strings that we'll land here.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•6 years ago
|
||
Example extension for testing: https://bugzilla.mozilla.org/attachment.cgi?id=8956419
Assignee | ||
Comment 5•6 years ago
|
||
https://bugzilla.mozilla.org/attachment.cgi?id=8942691
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
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#
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8951515 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968682 -
Flags: feedback?(bob.silverberg)
Comment 14•6 years ago
|
||
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)
Comment 15•6 years ago
|
||
Passing by note: several comments in the FTL need an update (reference to %S, or <image> in the section one).
Comment 16•6 years ago
|
||
mozreview-review |
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 17•6 years ago
|
||
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
Thank you Mark! Updated the patch to your feedback and requesting reviews.
Comment 20•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968682 -
Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Comment 23•6 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 24•6 years ago
|
||
> 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 hidden (mozreview-request) |
Comment 26•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
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.
Comment 29•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fb836a045f20 Refactor "extensionControlled" Preferences code to use Fluent. r=flod,Gijs
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb836a045f20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 33•6 years ago
|
||
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 → ---
Updated•6 years ago
|
status-firefox61:
fixed → ---
Target Milestone: Firefox 61 → ---
Comment 34•6 years ago
|
||
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
Comment 35•6 years ago
|
||
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
Comment 36•6 years ago
|
||
(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?
Comment 37•6 years ago
|
||
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
Comment 38•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d85e4da25463ccdcf902ac5a846773b8b642553
Comment 39•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 41•6 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ece4329ec73 Refactor "extensionControlled" Preferences code to use Fluent. r=flod,Gijs
Comment 42•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ece4329ec73
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gandalf)
You need to log in
before you can comment on or make changes to this bug.
Description
•