Closed Bug 299887 Opened 19 years ago Closed 19 years ago

Only check for updates for items that are updateable (error checking for reporter updates - bug 299040 fallout)

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

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

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

bug 299040 set the updateURL for reporter to about:blank which generates an
error when checking for updates on reporter. Instead of using about:blank I
suggest using a more descriptive value (e.g. none). Patch coming up.
Attached patch patch (obsolete) — Splinter Review
Don't check for updates when updateURL="none"

Besides reporter, I also set updateURL="none" for all themes using toolkit.
Other extensions that should not be checked for updates can be decided in bug
299040 and handled in this manner.
Attachment #188477 - Flags: review?(benjamin)
BTW: if you prefer this could also be handled by setting a pref for each
extension shipped instead of setting an updateURL value to be checked:

pref("extensions.%UUID%.update.enabled",false);

where %UUID% is the extension's ID.
I am ok with either method but I think I prefer using the pref method since it
centralizes management of this based on the app. If the extension is installed
separately from the app the extension can specify the pref in its default prefs
to enable updates if it is desired.
Attachment #188477 - Flags: review?(benjamin)
Attached patch better patch (obsolete) — Splinter Review
I think using the existing method of using the pref is cleaner. I also disable
"Update" in the EM's context menu when the pref to enable updates for an
extension is set to false. This is only for Firefox and I'll submit separate
patches to disable update for Thunderbird's and Sunbird's default themes if
this is the way to go for this.

BTW: I also fixed one error that is thrown when bringing up the EM's context
menu when nothing is selected.
Attachment #188477 - Attachment is obsolete: true
Attachment #188499 - Flags: review?(benjamin)
This is probably a stupid question but how about the appManaged arc, could that
be used to indicate that no attempts to update should be made?
Not a stupid question at all... I hadn't noticed that Ben recently added that
and it looks like he may have done so for siimilar reasons as needed here.
Instead of adding an updateURL of about:blank as bug 299040 did we should just
leave it empty, use the appManaged property, and add the appManaged property to
the default themes.

I still think it would be a good thing to control the command state of
cmd_update in a similar fashion as the patch does except it should also include
checking the appManaged property.

Good catch and thanks
Comment on attachment 188499 [details] [diff] [review]
better patch

Benjamin - I am going to resubmit using the appManaged property Ben recently
added. Are you ok with reading the pref as this patch does as well as the
appManaged property for controlling the command state for update so the correct
widget state can be shown to the user?
Attachment #188499 - Attachment is obsolete: true
Attachment #188499 - Flags: review?(benjamin)
You want to check for both updateURL="none" and appManaged="true" ? That's ok, I
supposed.
Actually, I am trying to avoid special casing updateURL="anything" for disabling
updates. I would prefer not adding default pref values to disable updates for
appManaged items since that would apply to all installs of the item. We only
have an nsIUpdateItem at the point in the code where we need to check for
appManaged="true". As I see it this needs a cleaner fix than what I have come up
with so far.
Attached patch work in progress (obsolete) — Splinter Review
This fixes several problems with extension updates. The one it doesn't is not
checking for updates to items that we don't have write access to (currently
this is checked in the EM ui for individual items but not elsewhere) and I
would prefer to deal with that in bug 300265. There are multiple criteria
involved in deciding whether or not an extension - or theme for that matter -
can be updated. If any of the following is true updates will not be performed
on the item.
opType == OP_NEEDS_INSTALL
opType == OP_NEEDS_UNINSTALL
opType == OP_NEEDS_UPGRADE
appManaged == "true"
extension's update.enabled pref value == false

I need to go through the code at least one more time but reviews are welcome at
this time. I want to make sure this is a good direction to go in under these
circumstances.
Attached patch patchSplinter Review
This should cover all of the conditions when an update check shouldn't happen
for an item and properly set the command state for update of individual items
in the managers.
Attachment #188899 - Attachment is obsolete: true
Attachment #189312 - Flags: review?(benjamin)
Attachment #189312 - Flags: review?(benjamin)
Attachment #189312 - Flags: review+
Attachment #189312 - Flags: approval1.8b4+
Whiteboard: [checkin needed][a+]
Summary: Error when checking for reporter updates - fallout from bug 299040 → Only check for updates for items that are uupdateable (error checking for reporter updates - bug 299040 fallout)
Summary: Only check for updates for items that are uupdateable (error checking for reporter updates - bug 299040 fallout) → Only check for updates for items that are updateable (error checking for reporter updates - bug 299040 fallout)
Checking in content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <-- 
extensions.js
new revision: 1.64; previous revision: 1.63
done
Checking in content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <-- 
extensions.xul
new revision: 1.32; previous revision: 1.31
done
Checking in content/update.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.js,v  <--  update.js
new revision: 1.13; previous revision: 1.12
done
Checking in public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v 
<--  nsIExtensionManager.idl
new revision: 1.36; previous revision: 1.35
done
Checking in src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--
 nsExtensionManager.js.in
new revision: 1.124; previous revision: 1.123
done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Whiteboard: [checkin needed][a+]
*** Bug 300342 has been marked as a duplicate of this bug. ***
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.