There are several params that are overloaded and badly named. This bug is for the cleanup work.

Hey nrthomas, for Firefox Lorentz we are going to add the following new properties to the update xml. showPrompt - this makes it so a major update doesn't have to display a prompt billboardURL - this removes the override of detailsURL when wanting to display a billboard. extra1 - this is app provided data to support Bug 538331. With this an app can provide a value in extra1 that is read after an update during startup to perform a custom action after an update. I'm not positive what the possible values will be but it will likely be something like "openPage", "showNotification", "showalert", and "silent". For the first three we might want to have a space then a url after the value to specify a url to open. The version and extensionVersion values in the update xml are poorly named. App update will still support them but the following new names will be preferred over the old names version will be displayVersion extensionVersion will be appVersion Also, I filed bug 459972 awhile back to add platformVersion to the update xml. Could that bug be morphed to add / fix the above?

After getting a decent amount of this patch written I think we should also have a property for displaying the "No Thanks" button. I am leaning towards going with showNever though that I'll try to come up with a better name.

Hi Robert. What's the time scale for landing these changes, and do they need to ship in (say) 3.6.2 in order to improve offering updates to lorentz ? Are we aiming to backport to 3.5 and 3.0 too ? We'll have to modify our update generation utility, and the script which generates the config for that. Some of that work will be done slightly differently for nightly updates (assuming it needs to support the new params too). A fair chunk of work to fitted in somewhere. platformVersion would not add too much to that. AUS will need to be taught about all new properties too, it handles all the existing ones by name, and I guess handle the version -> displayVersion changes.

This would land for lorentz. There has been no discussion of backporting and there is limited value since we would need strings to handle the new notifications though it could handle the silent option. I'm going to try to land the changes on trunk next week. The good news is that it is forward and backwards compatible so the changes to the aus side can happen after. The showPrompt, showNever, and billboardURL would need to be populated to have those behaviors that used to be accomplished via a single overridden property value. extra1 would need to be populated for the new behavior but it would fallback to the current behavior if it isn't present. As for versions that don't have these changes you should be able to use the same snippet generation code that includes the properties that are being deprecated and the client will just ignore the new properties. So, you can just include extensionVersion and it will be used for appVersion or both extensionVersion and appVersion and appVersion will take precedence over extensionVersion. The same is true for version. After the clients that don't have these changes is EOL then the old properties can be removed. I might be able to backport those specific parts so the transition period is shorter if you prefer.

I'll come up with tests in the next day or so

Comment on attachment 424761 [details] [diff] [review] 1. update snippet patch >diff --git a/toolkit/mozapps/update/public/nsIUpdateService.idl b/toolkit/mozapps >+ /** >+ * Whether to show the "No Thanks" button in the update prompt. This allows >+ * the user to never receive a notification for that specific update version >+ * again. >+ */ >+ attribute boolean showNeverForVersion; I wonder if this would sound better as "allowIgnoreUpdate"? Not really sure either sounds right but that is the only comment I can make about this otherwise great patch.

Comment on attachment 424762 [details] [diff] [review] 2. string cleanup patch This is equally great, perhaps even more so because I can't even find a nit to pick about it.

Dave, thanks for the review! I'll try to think of a different name before I checkin. I'm working on mochitests for app update and had to do a lot of refactoring to share code between the xpcshell tests and the mochitests.

almost there... then to finish up the mochitests. what a rat hole!

carrying forward r+

bah... I keep finding idiosyncrasies with the update prompt and mochitest that I have to workaround... these are the tests from hell.

Sorry about the size... alot of it is the moving of code into a shared file.

Comment on attachment 426650 [details] [diff] [review] 3. update snippet tests The mochitest-chrome tests passed but they leaked so removing review request and will resubmit after I figure out what is going on.

I fixed the original leaks / assertion and fixed another assertion I found over the weekend so I should have the tests done fairly soon.

This is just the xpcshell tests along with the moving of common code between xpcshell and mochitest into a new file

Forgot to refresh

Comment on attachment 427299 [details] [diff] [review] 4. UI cleanup for mochitest and a lil more cleanup I have added a little more UI cleanup. :(

Comment on attachment 427295 [details] [diff] [review] 2. update snippet xpcshell tests dolske said he'd take a look at this patch since mossop is busy.

carrying forward r+

Finally done

Carrying forward r+

Comment on attachment 428147 [details] [diff] [review] 3. UI cleanup for mochitest and a lil more cleanup bah... still needs a little more love

Spent a couple of hours testing the different update scenarios with this patch and all appears to be good!

btw: I've pushed all of the patches as well as all of the patches except for the strings patch to the try server and everything looks fine.

Fixed a bug (pre-existing) with setting the wizard title for updates found page. The title should be set via a dtd but I'd prefer to do that in another bug in case string changes aren't desired for Lorentz

removed a little more dead code

Comment on attachment 428393 [details] [diff] [review] 3. UI cleanup for mochitest and a lil more cleanup From what I can see this all looks fine, just a couple of things that could be done a bit simpler: >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js >+ onWizardNext: function() { >+ var regex = new RegExp('\\s*heed'); >+ var btn = gUpdates.wiz.getButton("next"); >+ btn.className = btn.className.replace(regex, ""); >+ } This is kind of ugly, how about just btn.classList.remove("heed"). Hopefully that works in XUL as well as HTML. >+ gUpdates.setButtons("askLaterButton", >+ update.showNeverForVersion ? "noThanksButton" : null, >+ "updateButton_" + update.type, true); >+ var btn = gUpdates.wiz.getButton("next"); >+ btn.className += " heed"; btn.classList.add("heed")

carrying forward review - thanks Dave!

Comment on attachment 428148 [details] [diff] [review] 4. app update mochitests Didn't really do a very thorough review though I can if you think it is necessary but these all look fantastic.

Thanks Dave, I don't think they need it... I've run them through the try server numerous times successfully.

Comment on attachment 427295 [details] [diff] [review] 2. update snippet xpcshell tests >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Should be "the Mozilla Foundation", here and elsewhere (see Gerv's recent blog post). >+function run_test_pt08() { > var patches = getRemotePatchString("complete", null, null, null, "0"); > patches += getRemotePatchString("partial", null, null, null, "0"); > var updates = getRemoteUpdateString(patches); > gResponseBody = getRemoteUpdatesXMLString(updates); >- run_test_helper_pt1("run_test_pt7 - one update with complete and partial " + >- "patches with size 0", 0, run_test_pt8); >+ run_test_helper_pt1("run_test_pt08 - one update with complete and partial " + >+ "patches with size 0", 0, run_test_pt09); > } This (and a few other tests) follow a similar pattern, where each one is setting up some state variables and then executing the test. Might it help simplify things to have an array of test-specific data to help cut down on those? Maybe not if there's no straightforward way to deal with the unique bits for each test section... Recent example of what I'm talking about: http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/unit/test_iniProcessor.js#70

(In reply to comment #53) > (From update of attachment 427295 [details] [diff] [review]) > > >+ * The Initial Developer of the Original Code is > >+ * Mozilla Corporation. > > Should be "the Mozilla Foundation", here and elsewhere (see Gerv's recent blog > post). Will do > >+function run_test_pt08() { > > var patches = getRemotePatchString("complete", null, null, null, "0"); > > patches += getRemotePatchString("partial", null, null, null, "0"); > > var updates = getRemoteUpdateString(patches); > > gResponseBody = getRemoteUpdatesXMLString(updates); > >- run_test_helper_pt1("run_test_pt7 - one update with complete and partial " + > >- "patches with size 0", 0, run_test_pt8); > >+ run_test_helper_pt1("run_test_pt08 - one update with complete and partial " + > >+ "patches with size 0", 0, run_test_pt09); > > } > > This (and a few other tests) follow a similar pattern, where each one is > setting up some state variables and then executing the test. Might it help > simplify things to have an array of test-specific data to help cut down on > those? Maybe not if there's no straightforward way to deal with the unique bits > for each test section... > > Recent example of what I'm talking about: > > http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/unit/test_iniProcessor.js#70 I considered doing that when these tests were written. I find it easier to refer to the settings in the test function vs. having to refer to them at the beginning of the file and the function calls are already heavily optimized to use defaults.

Pushed to mozilla-central 1 - http://hg.mozilla.org/mozilla-central/rev/be1110af60cd 2 - http://hg.mozilla.org/mozilla-central/rev/d394fbcfdfed 3 - http://hg.mozilla.org/mozilla-central/rev/c3b5fdbd8735 4 - http://hg.mozilla.org/mozilla-central/rev/c903b06b7ba5 5 - http://hg.mozilla.org/mozilla-central/rev/58b03899d500

Comment on attachment 431599 [details] [diff] [review] combined patch for 1.9.2 (no string changes) - requires patch from bug 536547 This is one of the app update changes wanted for Lorentz... this has baked for a while now and I'd like to get this in earlier rather than landing all of the patches at the same time.

Includes the patches from this bug and bug 536547 bug 480178 bug 548061 bug 551283 bug 549969 bug 545707 bug 543312 bug 552617 bug 554561 bug 553763

It also includes the fix for bug 538533

Comment on attachment 496425 [details] [diff] [review] 1.9.2 patch Dave, would you mind taking a look at this? It updates the code to be essentially the same as on trunk along with support for the existing format

Comment on attachment 496425 [details] [diff] [review] 1.9.2 patch Found a couple of minor idiosyncrasies with the layout... I'll resubmit after I have them fixed