This is similar to bug 307928 except this is for the update compatibility wizard that is used to update incompatible extensions when the app is upgraded and the app extension version changes. Patch coming up.
Created attachment 198184 [details] [diff] [review] patch
Comment on attachment 198184 [details] [diff] [review] patch so the problem is that this fails? or that this works? Note that we don't have UI for this pref anymore, the pref now corresponds to whitelisting, not xpinstall in general. And is disabling updates really the expected result, or is it _new_ software that people don't want? But 307928 in conjunction with this would force users to toggle pref, do updates, toggle pref, which really isn't what I think we want to do here...
Attempting to install an update with this wizard when xpinstall.enabled is false will fail and it will appear as a hang to the user. This wizard is shown before the main event loop starts so there is no ui except for the wizard at this stage and hence no ability for the user to toggle the pref. Bug 307928 disables checking for updates similarly since it is not possible to install updates when xpinstall.enabled is false. As for expected results I agree that we should provide other options but the xpinstall.enabled pref's purpose is to disable installs so we shouldn't be showing / enabling ui for installing updates.
To clarify just in case... I do think we need the ability for a "just this time" option but I think we also need to keep the ability to disable ALL installs as xpinstall.enabled provides so sysadmins can continue to lock down a system.
The options available as I see it: a) leave it how it is now where if xpinstall.enabled is false we fail during the install and the ui appears to hang. b) add the ability to toggle the pref in the wizard. There is a patch in bug 288054 that does this though this option was shot down for reasons I happen to agree with but IMO it is better than option a. c) go with this patch. d) ...? I think it would be a good thing if app update also warns the user during an upgrade that will initiate a compatibility check when xpinstall.enabled is set to false and provide the ability to change this pref value if it is not locked. This would of course require new strings. The same pretty much applies to bug 307928 and if new strings were allowed more could be done... I think it is better to prevent than to fail halfway through the operation which is where we are now and that is the direction that bug is headed at this time. Though it is not clear whether the patch for bug 288054 will be backed out even if it is it will not fix this bug or bug 307928.
Created attachment 198242 [details] screenshot It has been a while since I last verified this bug still exists so I went ahead and did so today and took a screenshot of the wizard page where it dies. If an update is found the user has the option to install it. If they try to install it the wizard page for downloading/installing is displayed with all buttons disabled and it never goes any further.
So, if I'm reading this patch right, what you're saying is that if xpinstall.enabled is set to false, we just totally skip the part of the compatability wizard where we check for updates to the extensions? That seems like the right behaviour to me, although we might *possibly* want to inform users that the step was skipped and they should manually update extensions if they want to find compatible versions.
That is what the patch does. Also, part of an earlier patch in bug 288054 does inform the user and it would be relatively easy to add though it would require new strings.
Comment on attachment 198184 [details] [diff] [review] patch Clearing review - a more comprehensive fix which would include new strings is more appropriate for the trunk
13 years ago
Attachment #198184 - Attachment is obsolete: true
Target Milestone: --- → Firefox 2 alpha2
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
K, lemme know if there's anything I can do to help here, in terms of strings, Rob.
(In reply to comment #10) > K, lemme know if there's anything I can do to help here, in terms of strings, Thanks Mike... I'd like to provide the option to set xpinstall.enabled to true if the wizard. I wrote a patch that did this previously but it was shot down due to not wanting prefs set via ui outside of preferences. Since we are doing this now I hope that this won't be shot down again due to that. I have an old patch that accomplishes 90% of this and I'll update it when I have the time. I'd also like to take care of bug 333860 since it is essentially the same problem except it is due to the app being offline.
I have a patch that is about 80% complete for fixing this bug.
Whiteboard: 1d → .5d
Created attachment 225403 [details] screenshot of wizard with patch applied Mike, this shows the update wizard when xpinstall.enabled is false and the user can change the pref and when xpinstall.enabled is false and the user can't change the pref due to it being locked. patch coming up.
Attachment #225403 - Flags: ui-review?(beltzner)
Created attachment 225404 [details] [diff] [review] patch
mconnor, perhaps one of the interns could take on bug 333860?
beltzner: I should mention that the strings used are from an old patch and obviously need revision.
Comment on attachment 225403 [details] screenshot of wizard with patch applied The string seems a little odd to me. Perhaps: "This update can not be installed because software installation is currently disabled. You can change this setting below." Or something that better indicates that this is changing a global setting?
Attachment #225403 - Flags: ui-review?(beltzner) → ui-review-
Created attachment 225915 [details] screenshot beltzner, screenshot with suggested string change. I also went through the update wiz strings and updated as appropriate / removed no longer used strings so you should probably review the strings in the patch instead of the screenshot.
Created attachment 225916 [details] [diff] [review] patch Mike and Mike, besides displaying the appropriate text, etc. regarding this bug this also removes the no longer used strings from the update wizard (e.g. the ones that should have been removed when app update was removed from this wizard) and updates the strings to use add-ons instead of extensions and themes, etc.
Version: unspecified → 2.0 Branch
Whiteboard: .5d → [needs review]
Comment on attachment 225916 [details] [diff] [review] patch Benjamin, mconnor is a bit swamped with review requests... could you review this? Thanks
Attachment #225916 - Flags: review?(mconnor) → review?(benjamin)
Comment on attachment 225916 [details] [diff] [review] patch >Index: toolkit/mozapps/extensions/content/update.js >+ this.xpinstallEnabled = pref.getBoolPref(PREF_XPINSTALL_ENABLED); >+ this.xpinstallLocked = pref.prefIsLocked(PREF_XPINSTALL_ENABLED); please add a try/catch around these, for XR apps that may not have set the default pref.
Attachment #225916 - Flags: review?(benjamin) → review+
Checked in to trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Attachment #225916 - Flags: approval1.8.1?
Comment on attachment 225916 [details] [diff] [review] patch I'll post a followup bug for suggested string changes, sorry to be late.
Attachment #225916 - Flags: ui-review?(beltzner) → ui-review+
Attachment #225916 - Flags: approval1.8.1? → approval1.8.1+
I am going to hold off on landing this on the branch until bug 342350 can be landed on the branch in order to avoid changing the strings multiple times.
Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0
No longer blocks: 333860
You need to log in before you can comment on or make changes to this bug.