Bug 2031290 - Enterprise: PackageKitUpdateChecker - #948
Conversation
19f9444 to
15a4520
Compare
4340818 to
a9ef312
Compare
349e310 to
5b7125b
Compare
5b7125b to
65d3d66
Compare
684366e to
440d0f8
Compare
| // on Windows at least, during testing, make sure we let time for the UI to show up | ||
| // before we start those actions. For some reason, not doing this ends up | ||
| // in Marionette not starting the test code while the update applies. | ||
| await new Promise((resolve, _) => { |
There was a problem hiding this comment.
The delay here is OK for getting the first version checked in, but I recommend opening a bug to figure out how to control this more precisely.
One thing that might help with preventing the update from happening automatically is the pref app.update.disabledForTesting
| case "update-downloaded": | ||
| // states from toolkit/mozapps/update/nsIUpdateService.idl#189-191 | ||
| switch (state) { | ||
| case "pending-elevate": |
There was a problem hiding this comment.
pending-elevate is a macos-specific state. pending-service is a Window-specific state. pending is a general-purpose state.
All 3 of these should probably be covered here.
See https://firefox-source-docs.mozilla.org/toolkit/mozapps/update/docs/Concepts.html#update-state
| case "update-staged": | ||
| case "update-downloaded": | ||
| // states from toolkit/mozapps/update/nsIUpdateService.idl#189-191 | ||
| switch (state) { |
There was a problem hiding this comment.
Instead of putting a switch directly inside another switch, can you please move this inner switch statement to a separate function?
| break; | ||
| // https://searchfox.org/enterprise-main/rev/a038f49228d707c6675ef20ce640034a64307d2e/toolkit/mozapps/update/UpdateListener.sys.mjs#366 | ||
| case "update-error": | ||
| switch (state) { |
There was a problem hiding this comment.
This is another switch to extract to a separate function
| // operation, and lets HandleErrorCodeSignal / HandleFinishedSignal be shared. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| template <typename PromisePrivateType, typename ResultType> |
There was a problem hiding this comment.
This entire file should have unit testing at the function level.
There was a problem hiding this comment.
Yes, but gtest were not a thing back then on our builds so there was no way to do it at all ... I've enabled them and if the code moves from a PoC to something we land, I'll add coverage
| "@mozilla.org/updates/update-manager;1", | ||
| Ci.nsIUpdateManager | ||
| ); | ||
| if (Services.prefs.getBoolPref("app.update.use_package_kit", false)) { |
There was a problem hiding this comment.
Is there a way to distinguish that this build is an enterprise build, or that this installation uses felt, or something that would be a clear differentiator from an consumer build?
I'd rather use something like that to control switching between these implementations.
There was a problem hiding this comment.
The limit here is that the enterprise build for non debian and debian package is the same (it's a repack, and we want to keep it this way), so we cannot rely only on AppConstants.MOZ_ENTERPRISE to switch implementation. We could check the pref only when AppCoinstants.MOZ_ENTERPRISE is true though.
| "@mozilla.org/updates/update-checker;1", | ||
| Ci.nsIUpdateChecker | ||
| ); | ||
| if (Services.prefs.getBoolPref("app.update.use_package_kit", false)) { |
There was a problem hiding this comment.
Assuming we're aiming for the separation of implementations we discussed, this file should not have any checks for app.update.use_package_kit.
| let updateCheckerContract = "@mozilla.org/updates/update-checker;1"; | ||
| let updateServiceContract = "@mozilla.org/updates/update-service;1"; | ||
| let updateManagerContract = "@mozilla.org/updates/update-manager;1"; | ||
| if (Services.prefs.getBoolPref("app.update.use_package_kit", false)) { |
There was a problem hiding this comment.
I think there should be a way to put all of these service lookups into one place and use that everywhere.
Something like:
class UpdateServiceFactory {
defineLazyGetter(lazy, name, service_nickname, type) {
const contract = #getContractByImpl(service_nickname);
XPCOMUtils.defineLazyServiceGetter(lazy, name, contract, type);
}
#getContractByImpl(service_nickname) {
const packageKitServices = {
"update-service": "@mozilla.org/updates/packagekit-update-service;1",
"update-checker": ...
}
const mozUpdaterServices = {
"update-service": "@mozilla.org/updates/update-service;1",
"update-checker": ...
}
if (!isPackagedApp) {
return mozUpdaterServices[service_nickname];
} else {
return packageKitServices[service_nickname];
}
Something like that.
440d0f8 to
df3b071
Compare
…grate into existing AUS
… & PackageKitUpdateService tests
df3b071 to
ed5e811
Compare
2 participants
Description
Bugzilla: Bug-2031290
Dependencies / Related Issues
Screenshots
Testing