Closed Bug 1448221 Opened 6 years ago Closed 6 years ago

Remove platform support for restart-required installs

Categories

(Toolkit :: Add-ons Manager, enhancement)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

Bug 1448221: Part 1 - Remove support for operations requiring restart.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1448221: Part 2 - Remove support for non-default legacy themes.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1448221: Part 3 - Remove startup staging directory scan.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
      No description provided.
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 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
Attachment #8961639 - Flags: review?(aswan) → review+
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...
Attachment #8961640 - Flags: review?(aswan) → review+
Comment on attachment 8961641 [details]
Bug 1448221: Part 3 - Remove startup staging directory scan.

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

Excellent, thanks!
Attachment #8961641 - Flags: review?(aswan) → review+
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 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 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
Blocks: 1372694
Depends on: 1449149
See Also: → 1453496
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.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.