Remove platform support for restart-required installs

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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

a year ago
mozreview-review
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+
Duplicate of this bug: 1372694

Comment 7

a year ago
mozreview-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 8

a year ago
mozreview-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+
(Assignee)

Comment 9

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1394117
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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

a year ago
mozreview-review-reply
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 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51155de79ee3
https://hg.mozilla.org/mozilla-central/rev/6f8118a829c4
https://hg.mozilla.org/mozilla-central/rev/22cd4c5fcb39
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Updated

a year ago
Blocks: 1372694

Updated

a year ago
Depends on: 1449149
Duplicate of this bug: 1372695
See Also: → 1453496

Comment 16

a year ago
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)
(Assignee)

Updated

7 months ago
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

7 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.