1948364 - Remove Screenshots system addon

Status: RESOLVED FIXED

(Firefox :: Screenshots, task)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This bugzilla issue is going to be tracking the following changes:

• removing the screenshots system addon from mozilla-central
• tweaking BrowserGlue _monitorScreenshotsPref idle task to still ensure that the integrated Screenshots browser feature is disabled through either of the legacy extensions.screenshots.disabled and new screenshots.browser.components.enabled about:config prefs
• tweaks existing tests covering the expectation of the Screenshots feature to be disabled through the extensions.screenshots.disabled pref to verify that the integrated Screenshots browser feature is disabled (and do not expect the deprecated screeenshots system addon to be installed anymore)
• removing non-permalink references to screenshots system addons assets that are going to be removed

On the contrary, the additional followups changes related to completely migrating away from using the extensions.screenshots.disabled pref are going to be tracked by separate bugzilla issue(s), e.g.:

• change Enterprise Policies's DisableFirefoxScreenshots settings to set and lock the new screenshots.browser.components.enabled pref, instead of still using the extensions.screenshots.disabled pref (see here)
• change CustomizableWidgets.sys.mjs to not use the extensions.screenshots.disabled pref to determine if the Screenshots toolbar button should be defined or not (see here)
• completely remove any othe remaining reference of the extension.screenshots.disabled pref from the tree

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1948364 - part 1: Remove screenshots system addon

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1948364 - part 2: Remove references to the removed browser/extensions/screenshots assets

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1948364 - part 3: Adjust BrowserGlue _monitorScreenshotsPref.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1948364 - part 4: adjust Enterprise Policies DisableFirefoxScreenshots setting mochitest.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

There was another reference to screenshots system addons assets here on the Firefox DevTools side that isn't removed by the part 2 patch attached to this bugzilla issue:

// These values are used to truncate the resulting image if the captured area is bigger.
// This is to avoid failing to produce a screenshot at all.
// It is recommended to keep these values in sync with the corresponding screenshots addon
// values in /browser/extensions/screenshots/selector/uicontrol.js
const MAX_IMAGE_WIDTH = 10000;
const MAX_IMAGE_HEIGHT = 10000;

I took a quick look to the internals of the Screenshots integrated components but I didn't find a corresponding const that I could just use to replace the reference to the removed screenshots system addon js script in the capture-screenshot.js DevTools' internal utility module, and so I wanted to at least highlight it here because it looks like it may require its own separate followup.

Comment 6

[Sam Foster [:sfoster] (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

I took a quick look to the internals of the Screenshots integrated components but I didn't find a corresponding const that I could just use to replace the reference

The equivalent dimensions now live at: https://searchfox.org/mozilla-central/rev/08b7628708d8fc8a425ebd1f26bfce7ace1bbbaa/browser/components/screenshots/ScreenshotsUtils.sys.mjs#46-48, so we should be able to update that devtools comment to point there instead.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1948364 - part 5: Remove call to force disable screenshots system addon from initPageActionsTest mochitest helper.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

I took a quick look to the internals of the Screenshots integrated components but I didn't find a corresponding const that I could just use to replace the reference

The equivalent dimensions now live at: https://searchfox.org/mozilla-central/rev/08b7628708d8fc8a425ebd1f26bfce7ace1bbbaa/browser/components/screenshots/ScreenshotsUtils.sys.mjs#46-48, so we should be able to update that devtools comment to point there instead.

The new dimentions are definitely those, but (at least based on a quick glance to what those consts are used for in the capture-screenshot.js DevTools internal utility module) it looks it may also require someone from the DevTools team to double-check if/how we should tweak the related logic on the capture-screenshot.js side to match the new dimentions restrictions as computed from the screenshots integrated components logic.

In the short term, I opted to include in part 2 the changes to replace the referenced screenshots internals in the pre-existing inline comment along with adding a TODO to mention the need to double-check if those consts and logic on the capture-screenshot.js side should also be adjusted to match the changes on the screenshots integrated components logic.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/effcc6926211
part 1: Remove screenshots system addon r=sfoster
https://hg.mozilla.org/integration/autoland/rev/f20c4be77aef
part 2: Remove references to the removed browser/extensions/screenshots assets r=zeid,devtools-reviewers,frontend-codestyle-reviewers,ochameau
https://hg.mozilla.org/integration/autoland/rev/5da1ae561041
part 3: Adjust BrowserGlue _monitorScreenshotsPref. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/4a0df3406fa3
part 4: adjust Enterprise Policies DisableFirefoxScreenshots setting mochitest. r=mkaply
https://hg.mozilla.org/integration/autoland/rev/1e26277915cb
part 5: Remove call to force disable screenshots system addon from initPageActionsTest mochitest helper. r=sfoster

Comment 11

[Mayank Bansal]

Improved the Windows installer size by 38KB

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/effcc6926211
https://hg.mozilla.org/mozilla-central/rev/f20c4be77aef
https://hg.mozilla.org/mozilla-central/rev/5da1ae561041
https://hg.mozilla.org/mozilla-central/rev/4a0df3406fa3
https://hg.mozilla.org/mozilla-central/rev/1e26277915cb

Comment 13

[Florin Bilt]

(In reply to Cosmin Sabou [:CosminS] from comment #12)

https://hg.mozilla.org/mozilla-central/rev/effcc6926211
https://hg.mozilla.org/mozilla-central/rev/f20c4be77aef
https://hg.mozilla.org/mozilla-central/rev/5da1ae561041
https://hg.mozilla.org/mozilla-central/rev/4a0df3406fa3
https://hg.mozilla.org/mozilla-central/rev/1e26277915cb

Perfherder has detected a awsy performance change from push 1e26277915cb0bfa4218c6d4bc1e738fc74470d0.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
1%	JS Tabs closed [+30s, forced GC]	windows11-64-24h2-shippable	fission tp6	46,074,891.67 -> 45,568,058.00

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 44324

For more information on performance sheriffing please see our FAQ.