1156046 - nsUpdateProcessor::ProcessUpdate should use the directory service instead of nsXREDirProvider (also removes app update gonk support)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Now that the app update xpcshell tests have a custom directory service the special casing in nsUpdateDriver.cpp for nsUpdateProcessor::ProcessUpdate is no longer necessary.

Comment 1

[Robert Strong (they/them - no direct email)]

Pushed to try along with the patch from bug 1155896
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93146a44f3e0

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 8594558 [details] [diff] [review]
patch rev1

Found a problem with this approach :(

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Pushed to oak

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Slightly cleaner

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

This cleans up ProcessUpdates and combines SwitchToUpdatedApp to ApplyUpdate... SwitchToUpdatedApp was only added to allow staging to land.

Comment 10

[Robert Strong (they/them - no direct email)]

Note: I've run this through try with success but the tests that could verify this code on gonk have been disabled since we added gonk due to other issues. I'm trying to figure out if I can get at least some of these tests to run on gonk and will post a try run at some point later.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8603575 [details] [diff] [review]
patch rev1

By fixing bug 821866 I was able to find a bug in the gonk changes so cancelling review until after I fix that.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 13

[Robert Strong (they/them - no direct email)]

This also removes gonk

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8857303 [details] [diff] [review]
patch rev1

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17dc9fbac3aa73220786ae3af40717c23955448

Comment 15

[Robert Strong (they/them - no direct email)]

Though the ApplyUpdate function could be further cleaned up I would prefer just going with combining the existing functionality from ApplyUpdate and SwitchToUpdatedApp into ApplyUpdate and doing any additional cleanup in another bug.

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 8857303 [details] [diff] [review]
patch rev1

Try run is looking good.

Comment 17

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8857303 [details] [diff] [review]
patch rev1

Review of attachment 8857303 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsUpdateDriver.cpp
@@ +712,2 @@
>    } else {
> +    // Launch updater.exe to either stage or apply an update

nit: the name "updater.exe" got copied into the Mac and Linux sections here.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch for landing with comments fixed

Comment 19

[Robert Strong (they/them - no direct email)]

Before landing I am double checking that the paths are correct for all 3 cases

Before the patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ac8aa602d0609613d095d85fc088909dca677bd

After the patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abbace5b34ade7510bfdb930515ffe02c320ab10

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #19)
> Before landing I am double checking that the paths are correct for all 3
> cases
> 
> Before the patch
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7ac8aa602d0609613d095d85fc088909dca677bd
> 
> After the patch
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=abbace5b34ade7510bfdb930515ffe02c320ab10
All of the paths are correct

Comment 21

[Robert Strong (they/them - no direct email)]

Landed on mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc65a6d51e749e7b605e67b1f621c779b2de9a36

Comment 22

[Robert Strong (they/them - no direct email)]

Accidentally landed with the wrong bug number.
https://hg.mozilla.org/mozilla-central/rev/cc65a6d51e74

I was told that since this made it to m-c it is best to leave it as is.