Details
- Reviewers
jmendez - Commits
- rFIREFOXAUTOLAND73815c686dca: Bug 2048048 - Fix policy issues with new tab preferences. r=jmendez
rFIREFOXAUTOLANDba7f1e692798: Bug 2048048 - Fix policy issues with new tab preferences. r=jmendez
rFIREFOXAUTOLANDcf46b7382184: Bug 2048048 - Fix policy issues with new tab preferences. r=jmendez - Bugzilla Bug ID
- 2048048
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
This revision makes changes to files both inside browser/extensions/newtab and in files outside of it. The New Tab page for Firefox Desktop supports train-hopping — a mechanism that allows New Tab code to be deployed to Beta and Release channels independently of the standard release train. Cross-layer changes can break train-hop compatibility, which may block New Tab deployments until fixes reach Release through normal channels.
If your change modifies platform code that browser/extensions/newtab depends on (such as module URIs, DOM APIs, or Nimbus feature definitions), please check whether a compatibility shim is needed. Shims should be marked with a @backward-compat { version NNN } comment indicating the Firefox version that must reach Release before the shim can be removed.
If your change is unrelated to the New Tab page's train-hop compatibility (for example, it only touches tests or documentation), you can possibly disregard this message - but if you are unsure whether your change affects train-hop compatibility, please ensure that someone from the home-newtab-reviewers-rotation review group has a chance to review before landing.
@mkaply, could you please add manual test steps for this in the context of enterprise policies and outline what the expected behaviour is for supportFirefox, Stories, and Weather after the changes?
| browser/components/enterprisepolicies/Policies.sys.mjs | ||
|---|---|---|
| 1783–1787 |
| |
| browser/components/enterprisepolicies/tests/browser/browser_policy_firefoxhome.js | ||
| 198–202 | I would drop this as this setting has been on by default since 152. Alternatively, I'd make sure the pref is true, otherwise it's possible to run this test in an environment with SRD off and test nothing because of an early return. SpecialPowers.pushPrefEnv({ set: [["browser.settings-redesign.enabled", true]] }) | |
| browser/extensions/newtab/lib/AboutPreferences.sys.mjs | ||
| 1432–1437 ↗ | (On Diff #1302638) |
Also, what's the intention behind this change? The bug mentions "I added some custom code that does the right thing based on the checkboxes" but it would help to have the intended behaviour spelled out. |
| browser/components/enterprisepolicies/Policies.sys.mjs | ||
|---|---|---|
| 1783–1787 | Yes. In the new design, setting those preferences to false makes Stories disappear completely (that's not what happened before). All we were trying to do is turn them off. | |
| browser/extensions/newtab/lib/AboutPreferences.sys.mjs | ||
| 1432–1437 ↗ | (On Diff #1302638) |
It should be disabled on (which I think would match the UI in the the non disabled case as well) The intention behind this change is that currently if you lock the two preferences, the checkboxes are disabled, but you can still use the "Support Firefox" toggle to visually turn the checkboxes on and off even though it doesn't actually do anything. If the checkboxes are disabled by policy, the Support Firefox should be disabled as well. |
| browser/extensions/newtab/lib/AboutPreferences.sys.mjs | ||
|---|---|---|
| 1432–1437 ↗ | (On Diff #1302638) | I have a wariness from historically running into trouble disconnecting a UI's displayed value from its internal value, as the get implementation appears to do here (although I don't have a practical known issue to raise here). I wonder if this might not be better solved in Policies.sys.mjs by setting and locking "browser.newtabpage.activity-stream.showSponsoredCheckboxes" to the appropriate value if both policies are locked. |
| browser/extensions/newtab/lib/AboutPreferences.sys.mjs | ||
|---|---|---|
| 1432–1437 ↗ | (On Diff #1302638) | That's an interesting idea. I think I like that. Gets me out of the new tab business completely. |
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
