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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Strong (they/them - no direct email)]

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.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch

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.

Comment 2

[Robert Strong (they/them - no direct email)]

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.

Comment 3

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: better patch

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.

Comment 5

[PikeUK]

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?

Comment 6

[Robert Strong (they/them - no direct email)]

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 7

[Robert Strong (they/them - no direct email)]

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?

Comment 8

[Benjamin Smedberg]

You want to check for both updateURL="none" and appManaged="true" ? That's ok, I
supposed.

Comment 9

[Robert Strong (they/them - no direct email)]

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.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: work in progress

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.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch

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.

Comment 12

[Asaf Romano (gone)]

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

Comment 13

[Steffen Wilberg]

*** Bug 300342 has been marked as a duplicate of this bug. ***