Closed Bug 530872 Opened 15 years ago Closed 14 years ago

app.update.url params / update.xml cleanup and addition of a custom string property for apps

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 38 obsolete files)

47.19 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
7.38 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
110.24 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
62.94 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
158.00 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
53.40 KB, patch
Details | Diff | Splinter Review
58.93 KB, patch
mossop
: review+
Details | Diff | Splinter Review
There are several params that are overloaded and badly named. This bug is for the cleanup work.
Blocks: 475671
Summary: app.update.url params cleanup → app.update.url params / update.xml cleanup
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?
Assignee: nobody → robert.bugzilla
Summary: app.update.url params / update.xml cleanup → app.update.url params / update.xml cleanup and addition of a custom string property for apps
Target Milestone: --- → mozilla1.9.3
Attached patch patch in progress (obsolete) — Splinter Review
Status: NEW → ASSIGNED
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.
Attachment #423945 - Attachment is obsolete: true
Attachment #424214 - Attachment is obsolete: true
Attachment #424215 - Attachment is obsolete: true
Attached patch 1. update snippet patch (obsolete) — Splinter Review
I'll come up with tests in the next day or so
Attachment #424534 - Attachment is obsolete: true
Attachment #424761 - Flags: review?(dtownsend)
Attached patch 2. string cleanup patch (obsolete) — Splinter Review
Attachment #424536 - Attachment is obsolete: true
Attachment #424762 - Flags: review?(dtownsend)
Attachment #424535 - Attachment is obsolete: true
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.
Attachment #424761 - Flags: review?(dtownsend) → review+
Attachment #424762 - Flags: review?(dtownsend) → review+
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.
Attachment #424764 - Attachment is obsolete: true
Attachment #425420 - Attachment is obsolete: true
almost there... then to finish up the mochitests. what a rat hole!
Attachment #425692 - Attachment is obsolete: true
carrying forward r+
Attachment #424761 - Attachment is obsolete: true
Attachment #426472 - Flags: review+
Attachment #426472 - Attachment is obsolete: true
Attachment #426472 - Attachment is obsolete: false
Attachment #425760 - Attachment is obsolete: true
Attachment #426472 - Attachment description: 1. update snippet - unbitrotted (file moves) → 1. update snippet patch - unbitrotted (file moves)
bah... I keep finding idiosyncrasies with the update prompt and mochitest that I have to workaround... these are the tests from hell.
Attachment #426474 - Attachment is obsolete: true
Attached patch 3. update snippet tests (obsolete) — Splinter Review
Sorry about the size... alot of it is the moving of code into a shared file.
Attachment #426644 - Attachment is obsolete: true
Attachment #426650 - Flags: review?(dtownsend)
Attachment #426650 - Attachment description: 3. update snippet tests - patch in progress → 3. update snippet tests
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.
Attachment #426650 - Flags: review?(dtownsend)
I fixed the original leaks / assertion and fixed another assertion I found over the weekend so I should have the tests done fairly soon.
Attached patch 2. update snippet xpcshell tests (obsolete) — Splinter Review
This is just the xpcshell tests along with the moving of common code between xpcshell and mochitest into a new file
Attachment #426650 - Attachment is obsolete: true
Attachment #427295 - Flags: review?(dtownsend)
Forgot to refresh
Attachment #427296 - Attachment is obsolete: true
Attachment #427299 - Flags: review?(dtownsend)
Attachment #427296 - Flags: review?(dtownsend)
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. :(
Attachment #427299 - Attachment is obsolete: true
Attachment #427299 - Flags: review?(dtownsend)
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.
Attachment #427295 - Flags: review?(dtownsend) → review?(dolske)
Attachment #427300 - Attachment is obsolete: true
Attachment #427741 - Attachment is obsolete: true
Attachment #427295 - Attachment description: 3. update snippet xpcshell tests → 2. update snippet xpcshell tests
Attachment #428137 - Attachment is patch: true
Attachment #428137 - Attachment mime type: application/octet-stream → text/plain
Attachment #428035 - Attachment is obsolete: true
Attached patch 5. string cleanup patch (obsolete) — Splinter Review
carrying forward r+
Attachment #424762 - Attachment is obsolete: true
Attachment #428139 - Flags: review+
Finally done
Attachment #428137 - Attachment is obsolete: true
Attachment #428147 - Flags: review?(dtownsend)
Attached patch 4. app update mochitests (obsolete) — Splinter Review
Attachment #428138 - Attachment is obsolete: true
Attachment #428148 - Flags: review?(dtownsend)
Attachment #428139 - Attachment is obsolete: true
Attachment #428139 - Flags: review+
Carrying forward r+
Attachment #428150 - Flags: review+
Comment on attachment 428147 [details] [diff] [review]
3. UI cleanup for mochitest and a lil more cleanup

bah... still needs a little more love
Attachment #428147 - Attachment is obsolete: true
Attachment #428147 - Flags: review?(dtownsend)
Spent a couple of hours testing the different update scenarios with this patch and all appears to be good!
Attachment #428163 - Flags: review?(dtownsend)
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
Attachment #428163 - Attachment is obsolete: true
Attachment #428385 - Flags: review?(dtownsend)
Attachment #428163 - Flags: review?(dtownsend)
removed a little more dead code
Attachment #428385 - Attachment is obsolete: true
Attachment #428393 - Flags: review?(dtownsend)
Attachment #428385 - Flags: review?(dtownsend)
Attachment #428150 - Attachment is obsolete: true
Attachment #428406 - Flags: review+
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")
Attachment #428393 - Flags: review?(dtownsend) → review+
carrying forward review - thanks Dave!
Attachment #428393 - Attachment is obsolete: true
Attachment #429149 - Flags: review+
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.
Attachment #428148 - Flags: review?(dtownsend) → review+
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
Attachment #427295 - Flags: review?(dolske) → review+
(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.
Attachment #428406 - Attachment is patch: true
Attachment #428406 - Attachment mime type: application/octet-stream → text/plain
Attachment #428406 - Flags: review+
Attachment #426472 - Attachment description: 1. update snippet patch - unbitrotted (file moves) → 1. update snippet patch - unbitrotted (file moves) - checked in
Attachment #429150 - Attachment description: 5. string cleanup patch - unbitrotted → 5. string cleanup patch - unbitrotted - checked in
Target Milestone: mozilla1.9.3 → mozilla1.9.3a3
Flags: in-testsuite+
Attachment #431599 - Attachment description: combined patch for 1.9.2 - requires patch from bug 536547 → combined patch for 1.9.2 (no string changes) - requires patch from bug 536547
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.
Attachment #431599 - Flags: approval1.9.2.3?
Depends on: 553763
Depends on: 554561
Attachment #431599 - Attachment is obsolete: true
Attachment #431599 - Flags: approval1.9.2.4?
It also includes the fix for bug 538533
Attached patch 1.9.2 patch (obsolete) — Splinter Review
Attachment #437751 - Attachment is obsolete: true
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
Attachment #496425 - Flags: review?(dtownsend)
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
Attachment #496425 - Attachment is obsolete: true
Attachment #496425 - Flags: review?(dtownsend)
Attached patch 1.9.2 patchSplinter Review
Attachment #496672 - Flags: review?(dtownsend)
Attachment #496672 - Flags: review?(dtownsend) → review+
Blocks: 625140
You need to log in before you can comment on or make changes to this bug.