1448221 - Remove platform support for restart-required installs

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1448221: Part 1 - Remove support for operations requiring restart.

Review commit: https://reviewboard.mozilla.org/r/230514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230514/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1448221: Part 2 - Remove support for non-default legacy themes.

Review commit: https://reviewboard.mozilla.org/r/230516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230516/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1448221: Part 3 - Remove startup staging directory scan.

Review commit: https://reviewboard.mozilla.org/r/230518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230518/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8961641 [details]
Bug 1448221: Part 3 - Remove startup staging directory scan.

https://reviewboard.mozilla.org/r/230518/#review236006


Code analysis found 1 defect in this patch:
 - 1 defect 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


::: testing/mozbase/mozprofile/tests/test_addons.py:10
(Diff revision 1)
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import absolute_import
>  
>  import os
>  import shutil

Error: 'shutil' imported but unused [flake8: F401]

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8961639 [details]
Bug 1448221: Part 1 - Remove support for operations requiring restart.

https://reviewboard.mozilla.org/r/230514/#review236672

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4323
(Diff revision 1)
> -    // Inactive add-ons don't require a restart to uninstall
> -    let requiresRestart = this.uninstallRequiresRestart(aAddon);
> -
>      // if makePending is true, we don't actually apply the uninstall,
>      // we just mark the addon as having a pending uninstall
> -    let makePending = aForcePending || requiresRestart;
> +    let makePending = aForcePending;

nit: get rid of `makePending` and just use `aForcePending` below

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8961640 [details]
Bug 1448221: Part 2 - Remove support for non-default legacy themes.

https://reviewboard.mozilla.org/r/230516/#review236686

It looks like this also fixes bug 1394117?  If so can you dup it to this one or is there something remaining on that bug?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3710
(Diff revision 1)
> -        previousTheme = theme;
> -      }
>      }
>  
> -    if (newSkin != this.currentSkin) {
> -      Services.prefs.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, newSkin);
> +    let defaultTheme = XPIDatabase.getVisibleAddonForInternalName(DEFAULT_SKIN);
> +    this.updateAddonDisabledState(defaultTheme, aId && aId != defaultTheme.id);

nit: no need for the first part of the && expression

::: toolkit/mozapps/extensions/test/xpcshell/test_corrupt.js:222
(Diff revision 1)
>      afterCorruption: {
> -      isActive: true,
> -      userDisabled: false,
> +      isActive: false,
> +      userDisabled: true,
>        appDisabled: false,
>        pendingOperations: 0,
>      },
>      afterSecondRestart: {
> -      isActive: true,
> -      userDisabled: false,
> +      isActive: false,
> +      userDisabled: true,
>        appDisabled: false,
>        pendingOperations: 0,
>      },

I think this can be squashed after the changes you made to the test in another bug?

::: toolkit/mozapps/extensions/test/xpcshell/test_corrupt.js:222
(Diff revision 1)
>      afterCorruption: {
> -      isActive: true,
> -      userDisabled: false,
> +      isActive: false,
> +      userDisabled: true,
>        appDisabled: false,
>        pendingOperations: 0,
>      },
>      afterSecondRestart: {
> -      isActive: true,
> -      userDisabled: false,
> +      isActive: false,
> +      userDisabled: true,
>        appDisabled: false,
>        pendingOperations: 0,
>      },

I think this can be squashed after the changes you made to the test in another bug?  and likewise below...

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8961641 [details]
Bug 1448221: Part 3 - Remove startup staging directory scan.

https://reviewboard.mozilla.org/r/230518/#review236696

Excellent, thanks!

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8961640 [details]
Bug 1448221: Part 2 - Remove support for non-default legacy themes.

https://reviewboard.mozilla.org/r/230516/#review236686

Yup.

> I think this can be squashed after the changes you made to the test in another bug?

Yeah, already fixed when I rebased.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8961640 [details]
Bug 1448221: Part 2 - Remove support for non-default legacy themes.

https://reviewboard.mozilla.org/r/230516/#review236686

> nit: no need for the first part of the && expression

Yes there is. If no ID is passed (which happens), we want to set the default theme to non-disabled. Without the first part, we set it to disabled instead.

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8961640 [details]
Bug 1448221: Part 2 - Remove support for non-default legacy themes.

https://reviewboard.mozilla.org/r/230516/#review236686

> Yes there is. If no ID is passed (which happens), we want to set the default theme to non-disabled. Without the first part, we set it to disabled instead.

oh right, i just mis-read it

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51155de79ee3e0260d22a0d6655e444fc1a9d174
Bug 1448221: Part 1 - Remove support for operations requiring restart. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8118a829c47450e90ef32958287db20cb0a3b0
Bug 1448221: Part 2 - Remove support for non-default legacy themes. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/22cd4c5fcb397d80fc7f693737dc1c827ce61d74
Bug 1448221: Part 3 - Remove startup staging directory scan. r=aswan

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/51155de79ee3
https://hg.mozilla.org/mozilla-central/rev/6f8118a829c4
https://hg.mozilla.org/mozilla-central/rev/22cd4c5fcb39

Comment 16

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.