Closed Bug 392180 Opened 17 years ago Closed 15 years ago

updateURL should have variable indicating the context of the check

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: aryx, Assigned: mossop)

References

Details

(Whiteboard: [amo:want P1])

Attachments

(1 file, 1 obsolete file)

Similar to the force=1 for manual checks for software updates, there should be a variable %FORCE% which can be used in the updateURL for throttling extension updates like software updates.
I'm not understanding what this is supposed to acheive
Some extensions have different branches for different application branches (example: Enigmail and Thunderbird). If a major update for the application gets pushed, all these people with the extension installed have to update. With this parameter, other updates (which are compatible with the application version) can be throttled. Furthermore, analyzing the update requests to the server gets more correct because forced checks can be ignored.
So it sounds like what you are looking for is some kind of update type parameter which could take 3 or so values, automatic, user initiated and app update compatibility or something?

Seem sensible?
Yes, I don't know if there are other types. The number of user initiated requests should be very small compared to other cases. Maybe the automatic requests should be separated into "on startup" and "scheduled". (I don't know the ways trunk can check for this.)
So off the top of my head we have:

Auto update checks (compatability and new versions)
Compatibility update checks (on install of incompatible extension).
Update checks (on install of new app)
Update checks (triggered by user from the add-ons UI).

Not sure if I missed anything there.

So this is going to require API changes and I'm not sure this is going to provide major benefits for authors. Maybe AMO would use it?
This would be nice to have for AMO. Although we don't plan on having to throttle updates anytime soon, it would be necessary if we ever had to. And it would also be useful for things like the Facebook App, which abuse update check to get a list of installed extensions :)
Product: Firefox → Toolkit
Target Milestone: --- → mozilla1.9.1
Fuller list of current update checks and what check mode they use for my reference:

Daily backround check                CHECK_NEWVERSION
Compatibility check during install   CHECK_COMPATIBILITY
App update compatibility sync        SYNC_COMPATIBILITY
App update new version check         CHECK_NEWVERSION
User initiated update check          CHECK_NEWVERSION
App update check for compatibility   CHECK_NEWVERSION?
This is increasingly more important to the accuracy of AMO's statistics. Can we take a look at this in Q2? (I can't request the wanted flag for .next)
Summary: Provide force variable to use in the updateURL → updateURL should have variable indicating the context of the check
Target Milestone: mozilla1.9.1 → mozilla1.9.2
Blocks: 343779
Flags: wanted1.9.2?
Whiteboard: [amo:want P1]
(In reply to comment #7)
Updated to match current trunk:

Daily background check                                CHECK_NEWVERSION
User initiated update check                           CHECK_NEWVERSION
Compatibility check during install                    CHECK_COMPATIBILITY

Compatibility check when new app version is detected  NOTIFY_NEWVERSION
New version check when new app version is detected    NOTIFY_NEWVERSION

Compatibility check in a new app version              SYNC_COMPATIBILITY
Update check in a new app version                     CHECK_NEWVERSION
Assignee: nobody → dtownsend
(In reply to comment #9)
> Daily background check                                CHECK_NEWVERSION
> User initiated update check                           CHECK_NEWVERSION

In order for this to be useful for us stats-wise, the daily background check will need to have its own identifier so that we can only count 24 hour checks for ADU stats.
(In reply to comment #10)
> (In reply to comment #9)
> > Daily background check                                CHECK_NEWVERSION
> > User initiated update check                           CHECK_NEWVERSION
> 
> In order for this to be useful for us stats-wise, the daily background check
> will need to have its own identifier so that we can only count 24 hour checks
> for ADU stats.

Yep I agree, just making notes of what the code is doing right now here.
There are basically two ways we can go here. We could just include a simple number to describe which situation we are checking in (1-7). Whenever we added a new case or modified a case we'd need to add new numbers and any app or extension developers using the API would need to pick one closest to their reason for searching for an update.

An alternative is to send a value that is a bitfield, including information about whether the action was user initiated or not, whether the update is looking for new versions or just compatibility information and then some additional value to separate out the different cases. The benefit here is that other users of the API could still send some meaningful values. AMO could potentially also reduce the database load by only returning compatibility information, not new versions if only compatibility is requested.

The bitfield would still give us unique values for each case we use, but would AMO ever use the additional information?
I doubt AMO would make any changes to the update.rdf script to make use of special parameters -- it's heavily cached and I don't think any slight performance improvements would be worth the risk of making large changes to it.

I am fine with either method. It would probably be more consistent with the other parameters to do the bitfield than an arbitrary number, but it doesn't matter a whole lot to us.
Attached patch patch rev 1 (obsolete) — Splinter Review
Flip-flopped a little here but here is what I've gone with. Through the API you can pass an update type and there are 3 exposed possible values (app update detected, app installed, user requested). There are also some internal update types (background update and add-on install), values can range from 16 to 31. Added to this are the values 32 and 64 depending on whether the update check cares about compatibility and/or new versions. The final value is replaced into the update URL in place of the %UPDATE_TYPE% string.

The upshot is that callers who aren't passing a type will send an update type of 32 or 96. For the defined cases in Firefox we should send:

Daily background check                              112
User initiated update check                         97
Compatibility check during install                  49

New app detected in the background                  98
New app detected after user did check for updates   98

Compatibility check when new app is installed       35
New version check when new app is instaled          99

Justin, do we need to separate out the app detected cases? I think they are similar enough for it not to be worthwhile. And do we need to bump the request version just for adding the additional parameter?
Attachment #389128 - Flags: review?(robert.bugzilla)
Status: NEW → ASSIGNED
(In reply to comment #14)
> Justin, do we need to separate out the app detected cases? I think they are
> similar enough for it not to be worthwhile. And do we need to bump the request
> version just for adding the additional parameter?

AMO doesn't need the app detected case separated, so totally up to you. And I don't think we need to bump the version -- we didn't when we added other params like locale. I think we probably only need to bump when we change the way a parameter works, like with appVersion.
Comment on attachment 389128 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>--- a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>+++ b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>...
>@@ -304,25 +319,29 @@ interface nsIExtensionManager : nsISuppo
>    *          This param is only honored when updateCheckType is equal to
>    *          UPDATE_NOTIFY_NEWVERSION and it defaults to the current version of
>    *          the application when it is not specified.
>    * @param   platformVersion (optional)
>    *          The version of the toolkit to check for compatible updates.
>    *          This param is only honored when updateCheckType is equal to
>    *          UPDATE_NOTIFY_NEWVERSION and it defaults to the current version of
>    *          the toolkit when it is not specified.
>+   * @param   updateType (optional)
>+   *          The type of the update check. Should be one of the UPDATE_WHEN
>+   *          values.
>    *
>    * @throws  NS_ERROR_ILLEGAL_VALUE if any item is invalid.
>    */
>    void update([array, size_is(itemCount)] in nsIUpdateItem items,
>                in unsigned long itemCount,
>                in unsigned long updateCheckType,
>                in nsIAddonUpdateCheckListener listener,
>                [optional] in AString appVersion,
>-               [optional] in AString platformVersion);
>+               [optional] in AString platformVersion,
>+               [optional] in unsigned long updateType);
Seems like updateType should be before appVersion so the call sites that currently don't provide appVersion and platformVersion don't have to specify null, null, updateType.

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>@@ -4864,29 +4872,34 @@ ExtensionManager.prototype = {
>     gOS.notifyObservers(this.datasource.getItemForID(id),
>                         EM_ACTION_REQUESTED_TOPIC, reason);
>   },
> 
>   /**
>    * See nsIExtensionManager.idl
>    */
>   update: function EM_update(items, itemCount, updateCheckType, listener,
>-                             appVersion, platformVersion) {
>+                             appVersion, platformVersion, updateType) {
>+
>+    // Only allow the public UPDATE_WHEN values through
>+    if (updateType > 15)
Fix the comment. Please use a const for 15 so it doesn't appear to be a magic number

Also, the relationship between the constants in the nsIExtensionManager.idl and nsExtensionManager.js.in are confusing. Could you add comments detailing their use and how they relate to one another.

r=me with that
Attachment #389128 - Flags: review?(robert.bugzilla) → review+
Attached patch patch rev 2Splinter Review
Updated from review. Needs sr since we're adding a parameter to the update method on nsIExtensionManager.
Attachment #389128 - Attachment is obsolete: true
Attachment #398662 - Flags: superreview?(benjamin)
Attachment #398662 - Flags: review+
Attachment #398662 - Flags: superreview?(benjamin) → superreview+
Landed: http://hg.mozilla.org/mozilla-central/rev/f9e240895ffc
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [amo:want P1] → [amo:want P1][needs baking]
Target Milestone: mozilla1.9.2 → mozilla1.9.3a1
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090911 Minefield/3.7a1pre

Checking for extension updates manually now doesn't work (throbber spins, progress bar is stuck at 0 "Checking for Updates").

Error Console shows:

Error: nsIExtensionMananger is not defined
Source File: chrome://mozapps/content/extensions/extensions.js
Line: 2258

I'm guessing it's this bug.
Yeah slight typo there, landed the fix: http://hg.mozilla.org/mozilla-central/rev/9fd629c21da8
Depends on: 516506
Comment on attachment 398662 [details] [diff] [review]
patch rev 2

AMO really needs this to get sane stats during Firefox upgrades. It's been baking for a while and there is one regression that got fixed so that needs approval too.
Attachment #398662 - Flags: approval1.9.2?
Whiteboard: [amo:want P1][needs baking] → [amo:want P1]
Attachment #398662 - Flags: approval1.9.2? → approval1.9.2+
Depends on: 540087
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.