We have most of what bug 1121018 removes. I believe all that is needed is patch type (complete or partial) and at the same time I want to add more telemetry for the ui.

The update check ping will also be sent by UI's and I'll add that later. To keep history from older clients I have changed the code for results that have been broken out into more specific codes. I'll submit a download result code ping next.

Comment on attachment 8570331 [details] [diff] [review] 1. patch - check telemetry ping typo :(

I combined the patch to remove the no update found from the check ping and instead send that as a boolean for no update found vs. update found.

Fixed some naming

Comment on attachment 8570354 [details] [diff] [review] 1. patch - check telemetry ping Going to hold off on requesting review until after bug 1136358 finishes review

In errors.h there have been no reports of the error codes in telemetry that I reused. I went with new telemetry histogram id's since most of the existing histograms changed and this will make it separate the old from the new. Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbefa5cdcd4

Note: the patch requires the patch in bug 1121018. I don't know at this time if having check codes for the ui will be valuable and if it is aded I'll do it in another bug.

Comment on attachment 8584439 [details] [diff] [review] patch rev1 Review of attachment 8584439 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Histograms.json @@ -3467,1 @@ > "expires_in_version": "never", Is just removing telemetry constants the right way to obsolete them? I thought it was to set expires_in_version? Will just removing them make them disappear from the dashboard? ::: toolkit/mozapps/update/UpdateTelemetry.jsm @@ +16,5 @@ > + // The update check was performed by the call to notify in nsUpdateService.js. > + NOTIFY: "NOTIFY", > + // The update check was performed by the call to checkForBackgroundUpdates in > + // nsUpdateService.js. > + EXTERNAL: "EXTERNAL", Please add some justification on why these things are tracked differently. Do you expect they will differ? Why is it useful to track them differently? @@ +123,5 @@ > + if (aCode != this.CHK_NO_UPDATE_FOUND) { > + Services.telemetry.getHistogramById(id).add(aCode); > + noUpdateFound = false; > + } > + id = "UPDATE_CHECK_NO_UPDATE_" + aSuffix; Note that because these strings are computed dynamically it makes the grep test fail. Meaning for someone reading the code wanting to find references to UPDATE_CHECK_NO_UPDATE_EXTERNAL it makes it harder for them. Would be nice to have the strings explicitly. Could pass in for example isExternal and then id = isExternal ? "UPDATE_CHECK_NO_UPDATE_EXTERNAL" : "UPDATE_CHECK_NO_UPDATE_NOTIFY". It's not worth changing at this point, but just a thought. ::: toolkit/mozapps/update/nsUpdateService.js @@ +2166,5 @@ > LOG("UpdateService:_postUpdateProcessing - patch found in downloading " + > "state"); > if (update && update.state != STATE_SUCCEEDED) { > // Resume download > +// Possible place where download is stuck? nit: Indentation, maybe expand on what this comment means more. @@ +2640,5 @@ > "update is disabled"); > return; > } > > +#ifdef MOZ_METRO I'd just remove this, Windows 10 is removing support for new style enabled desktop browsers. Making any chance of a revival 0. @@ +4637,2 @@ > if (update.state == STATE_FAILED && > + (WRITE_ERRORS.indexOf(update.errorCode) != -1 || I think the better ES6 way to do this is: (WRITE_ERRORS.includes(update.errorCode) || ::: toolkit/mozapps/update/tests/chrome/test_0105_background_restartNotification_VersionCompatAddons.xul @@ +31,5 @@ > + debugDump("entering"); > + > + Services.prefs.setIntPref(PREF_APP_UPDATE_PROMPTWAITTIME, 1); > + > + let url = URL_HTTP_UPDATE_XML + "?showDetails=1" + Would be helpful but not required to explicitly add a comment in this file here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome/utils.js#154 ...that a URL should not be specified with URL parameters. ::: toolkit/mozapps/update/tests/chrome/utils.js @@ +1144,5 @@ > + aAddon.userDisabled = gDisableNoUpdateAddon; > + } > + } > + > + if (aAddon.name.indexOf("updatecompatibility") == 0) { Unless we're explicitly trying not to use ES6, please change here and elsewhere: if (aAddon.name.startsWith("updatecompatibility") { ::: toolkit/mozapps/update/updater/updater.cpp @@ +487,5 @@ > NS_tdirent *entry; > > dir = NS_topendir(path); > if (!dir) { > + LOG(("ensure_remove_recursive: path is not a directory: " LOG_S \ nit: Here and elsewhere, you don't need the trailing backslash here because of auto string literal concat and whitespace ignored. Only need it for preprocessor definitions of macros.

(In reply to Brian R. Bondy [:bbondy] from comment #8) > Comment on attachment 8584439 [details] [diff] [review] > patch rev1 > > Review of attachment 8584439 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/telemetry/Histograms.json > @@ -3467,1 @@ > > "expires_in_version": "never", > > Is just removing telemetry constants the right way to obsolete them? I > thought it was to set expires_in_version? Will just removing them make them > disappear from the dashboard? Yes though getting them removed from the data is likely another story. From the "Adding a new Telemetry probe" page https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe expires_in_version: Required. The version number in which the histogram expires, e.g. "30"; a version number of type "N" and "N.0" is automatically converted to "N.0a1" in order to expire the histogram also in the development channels. A telemetry probe acting on an expired histogram will be considered a non-op. For histograms that never expire the value "never" can be used as in the example above. > ::: toolkit/mozapps/update/UpdateTelemetry.jsm > @@ +16,5 @@ > > + // The update check was performed by the call to notify in nsUpdateService.js. > > + NOTIFY: "NOTIFY", > > + // The update check was performed by the call to checkForBackgroundUpdates in > > + // nsUpdateService.js. > > + EXTERNAL: "EXTERNAL", > > Please add some justification on why these things are tracked differently. > Do you expect they will differ? Why is it useful to track them differently? Will do > @@ +123,5 @@ > > + if (aCode != this.CHK_NO_UPDATE_FOUND) { > > + Services.telemetry.getHistogramById(id).add(aCode); > > + noUpdateFound = false; > > + } > > + id = "UPDATE_CHECK_NO_UPDATE_" + aSuffix; > > Note that because these strings are computed dynamically it makes the grep > test fail. Meaning for someone reading the code wanting to find references > to UPDATE_CHECK_NO_UPDATE_EXTERNAL it makes it harder for them. Would be > nice to have the strings explicitly. Could pass in for example isExternal > and then id = isExternal ? "UPDATE_CHECK_NO_UPDATE_EXTERNAL" : > "UPDATE_CHECK_NO_UPDATE_NOTIFY". It's not worth changing at this point, but > just a thought. I hadn't thought of that and it makes sense > ::: toolkit/mozapps/update/nsUpdateService.js > @@ +2166,5 @@ > > LOG("UpdateService:_postUpdateProcessing - patch found in downloading " + > > "state"); > > if (update && update.state != STATE_SUCCEEDED) { > > // Resume download > > +// Possible place where download is stuck? > > nit: Indentation, maybe expand on what this comment means more. Meant to remove that and look into it later. > @@ +2640,5 @@ > > "update is disabled"); > > return; > > } > > > > +#ifdef MOZ_METRO > > I'd just remove this, Windows 10 is removing support for new style enabled > desktop browsers. Making any chance of a revival 0. Thanks! > @@ +4637,2 @@ > > if (update.state == STATE_FAILED && > > + (WRITE_ERRORS.indexOf(update.errorCode) != -1 || > > I think the better ES6 way to do this is: > > (WRITE_ERRORS.includes(update.errorCode) || It is ES7 ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes ) and there is already a comment in the patch of "// Replace with Array.prototype.includes when it has stabilized." Also, this came up in irc today: [10:37] <Mossop> mconley: JS team are the arbiters of truth [10:37] <mconley> ok, cool [10:38] <mconley> yeah, I was just talking to jorendorff [10:38] <mconley> and he's going to work on a wiki page to lay out what's good to use, and what is not [10:38] <mconley> apparently, all implemented ES6 stuff is OK to use, with the following exceptions: [10:38] <mconley> Classes, Array.prototype.includes, ArrayBuffer.transfer [10:39] <mconley> those are all Nightly-only, and will break on Aurora uplift. > ::: > toolkit/mozapps/update/tests/chrome/ > test_0105_background_restartNotification_VersionCompatAddons.xul > @@ +31,5 @@ > > + debugDump("entering"); > > + > > + Services.prefs.setIntPref(PREF_APP_UPDATE_PROMPTWAITTIME, 1); > > + > > + let url = URL_HTTP_UPDATE_XML + "?showDetails=1" + > > Would be helpful but not required to explicitly add a comment in this file > here: > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/ > chrome/utils.js#154 > ...that a URL should not be specified with URL parameters. Not sure what you mean? > ::: toolkit/mozapps/update/tests/chrome/utils.js > @@ +1144,5 @@ > > + aAddon.userDisabled = gDisableNoUpdateAddon; > > + } > > + } > > + > > + if (aAddon.name.indexOf("updatecompatibility") == 0) { > > Unless we're explicitly trying not to use ES6, please change here and > elsewhere: > if (aAddon.name.startsWith("updatecompatibility") { That can be changed now that it has stabilized. Thanks! > ::: toolkit/mozapps/update/updater/updater.cpp > @@ +487,5 @@ > > NS_tdirent *entry; > > > > dir = NS_topendir(path); > > if (!dir) { > > + LOG(("ensure_remove_recursive: path is not a directory: " LOG_S \ > > nit: Here and elsewhere, you don't need the trailing backslash here because > of auto string literal concat and whitespace ignored. Only need it for > preprocessor definitions of macros. Will do

Pushed to mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7b6c34ec5c