1460717 - Cleanup XPIDatabaseReconcile.processStartupChanges

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Kris Maglione [:kmag]]

There's probably a lot of simplification we can do here, but at the moment, the main issue is that this code is so hairy and convoluted that it's hard for anyone to actually understand well enough to change.

We should do some basic refactoring as a starting point to a larger simplification.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1460717: Make processFileChanges somewhat maintainable.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8974825 [details]
Bug 1460717: Make processFileChanges somewhat maintainable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243204/diff/1-2/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8974825 [details]
Bug 1460717: Make processFileChanges somewhat maintainable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243204/diff/2-3/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8974825 [details]
Bug 1460717: Make processFileChanges somewhat maintainable.

https://reviewboard.mozilla.org/r/243204/#review249358

Whew, thanks.

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:2460
(Diff revision 3)
>      // corrupt database and we don't have migration data for this add-on then
>      // this must be a new install.

Trying to make this comment clear may be a lost cause but I think you drop the "and we don't have migration data..." bit

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:2706
(Diff revision 3)
> +    // Also reload the metadata for add-ons in the application directory
> +    // when the application version has changed.

I don't understand this

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8974825 [details]
Bug 1460717: Make processFileChanges somewhat maintainable.

https://reviewboard.mozilla.org/r/243204/#review249358

> I don't understand this

I don't understand it either.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8974825 [details]
Bug 1460717: Make processFileChanges somewhat maintainable.

https://reviewboard.mozilla.org/r/243204/#review249358

> I don't understand it either.

so just remove it?  it pretty clearly doesn't apply to this code

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c010e63474e69583574d1d6a72fe6e57c272239
Bug 1460717: Make processFileChanges somewhat maintainable. r=aswan

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1c010e63474e

Comment 9

[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.