Closed Bug 879480 Opened 11 years ago Closed 5 years ago

Allow external processes to request add-on installs / uninstalls

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jorgev, Unassigned, Mentored)

References

Details

Attachments

(2 files, 1 obsolete file)

Screenshot of notification that requests a restart
44.20 KB, image/png
Details
wip patch for installation (2)
24.65 KB, patch
Unfocused
: feedback+
Details | Diff | Splinter Review 
The current way of installing add-ons from an external installer is to drop them to a special location or add a Windows Registry key and then wait for the next Firefox restart for the user to opt-in to the installs. Most external installers just force-restart Firefox, so the opt-in screen appears during the install flow, which makes perfect sense other than the bad experience of the forced restart.

We need a mechanism where an external process can tell Firefox that new add-on(s) have been dropped, so that users are shown the opt-in screen immediately and no restart is required (except maybe to finalize the add-on install, but this would be user-initiated). The same should be implemented for uninstalls.
Attached patch WIP patch fo installation (obsolete) — — Splinter Review 
The flag "-install-global-addon" takes a path where the vendor put his add-on (in the filesystem). If he is not installing via a registry entry, the add-on has to situated in one of the supported locations (excluding profile).
When the flag is used, the install locations are scanned for this add-on and the add-on is added to the database if found. Then the opt-in screen is shown.
It just tries to do the same things that happen on fresh startup.

The patch fails to show the opt-in screen even if it successfully adds the add-on to the database. During startup if an add-on is detected as AddonManager.STARTUP_CHANGE_INSTALLED, it can be decided that it is a new install.
But since checking now happens while Firefox is running, adding startup changes will not work. So even though the new add-on is found on disk, there doesn't seem to be an existing way to confirm the same from processFileChanges. (see addInstalledAddon method)
Please suggest a workaround.
Attachment #767901 - Flags: feedback?(bmcbride)
And for uninstallation, the only way it can be done while Firefox is running is through the registry. If that is done and the "-uninstall-global-addon" flag is used, we want to immediately notify the user about:
* a successful uninstallation
* a need for restart
whichever is the case.

What form of notifications shall we use? And will we show notifications for both the cases?
Status: NEW → ASSIGNED
Flags: needinfo?
Flags: needinfo?
QA Contact: mihai.morar
Adding back Sachin's needinfo? from Comment 2
Flags: needinfo?
(In reply to Sachin Hosmani [:sachin] from comment #2)
> What form of notifications shall we use? And will we show notifications for
> both the cases?

I would suggest something that is least annoying, which would be the notification box that can appear above or below the content. Can we create a prototype that does this and then we can submit it for UX review?
Flags: needinfo?
(In reply to Jorge Villalobos [:jorgev] from comment #4)
> (In reply to Sachin Hosmani [:sachin] from comment #2)
> > What form of notifications shall we use? And will we show notifications for
> > both the cases?
> 
> I would suggest something that is least annoying, which would be the
> notification box that can appear above or below the content. Can we create a
> prototype that does this and then we can submit it for UX review?

What about that notification which asks for a restart of the browser after update?
I'm thinking we could show that non-persistently only on the tab that is focused.
(In reply to Sachin Hosmani [:sachin] from comment #5)
> (In reply to Jorge Villalobos [:jorgev] from comment #4)
> > (In reply to Sachin Hosmani [:sachin] from comment #2)
> > > What form of notifications shall we use? And will we show notifications for
> > > both the cases?
> > 
> > I would suggest something that is least annoying, which would be the
> > notification box that can appear above or below the content. Can we create a
> > prototype that does this and then we can submit it for UX review?
> 
> What about that notification which asks for a restart of the browser after
> update?
> I'm thinking we could show that non-persistently only on the tab that is
> focused.

Those notification boxes can include a button, which in this case would be a restart button. Which notifications are you referring to?
(In reply to Jorge Villalobos [:jorgev] from comment #6)
> (In reply to Sachin Hosmani [:sachin] from comment #5)
> > What about that notification which asks for a restart of the browser after
> > update?
> > I'm thinking we could show that non-persistently only on the tab that is
> > focused.
> 
> Those notification boxes can include a button, which in this case would be a
> restart button. Which notifications are you referring to?

I now realize that the restart required (on update) notification I was talking about is Ubuntu specific (an add-on that gets shipped with Firefox does this).
Anyway, the notification used is this: https://developer.mozilla.org/en-US/docs/XUL/notificationbox.
I just want to confirm this issue about uninstallation.
When this bug is resolved, we will ask the vendor to do this to uninstall an add-on :
1. Remove the registry entry / add-on from the file system
2. Use the flag "-uninstall-global-addon"

* If there is no instance of Firefox running, *a new one will start* and it will detect the removal as it happens now. I hope a fresh Firefox opening, is okay.
* If there is an instance of Firefox running, it will verify the removal and remove the add-on from the db and if a restart is necessary it will say so with a notification. If it is a restart-less add-on it will say the add-on was removed.

Is this all good?

And if we use the notification box I mentioned in comment 7 will we still to submit it for UX review?
(In reply to Sachin Hosmani [:sachin] from comment #8)
> * If there is no instance of Firefox running, *a new one will start* and it
> will detect the removal as it happens now. I hope a fresh Firefox opening,
> is okay.

This isn't really necessary, I think. The add-on is removed from the file system and will disappear next time Firefox starts, so there's no need to prompt the user to do anything.

> * If there is an instance of Firefox running, it will verify the removal and
> remove the add-on from the db and if a restart is necessary it will say so
> with a notification. If it is a restart-less add-on it will say the add-on
> was removed.

This sounds good.

> And if we use the notification box I mentioned in comment 7 will we still to
> submit it for UX review?

Yes, anything that introduces new UI will need to go through UX review.
Okay. But feedback on the patch in comment 1 would be needed because it's pretty much the same kind of file checking I'm using for uninstallation too. Also the problem I mentioned there would apply here too.
This notification box is shown when an external process removes an add-on while Firefox is running and that removal requires a restart. The notification is shown only in the focused tab of Firefox and only once.
Attachment #777278 - Flags: review?(ux-review)
Attachment #767901 - Flags: feedback?(dtownsend+bugmail)
(In reply to Sachin Hosmani [:sachin] from comment #11)
> Created attachment 777278 [details]
> Screenshot of notification that requests a restart

I would change "An external process" to something like "Another application", but I guess UX will also give you feedback on the message.
Depends on: 901553
Comment on attachment 767901 [details] [diff] [review]
WIP patch fo installation

Review of attachment 767901 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of good stuff here, it's clearly well on its way. There are a few UX concerns and some mechanical changes to look at before this needs another look I think.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +2993,5 @@
>        let installLocation = this.installLocationsByName[aSt.name];
>        let addonStates = aSt.addons;
>  
> +      // Filter out only add-on states with IDs not in aIds
> +      if (aIds && aIds instanceof Array && aIds.length > 0) {

I don't think you need do anything other than check aIds isn't null here. Only internal code should be calling this function.

@@ +2994,5 @@
>        let addonStates = aSt.addons;
>  
> +      // Filter out only add-on states with IDs not in aIds
> +      if (aIds && aIds instanceof Array && aIds.length > 0) {
> +        for (let key in addonStates) {

I'm not sure if iterating the keys while you're changing them is safe. More efficient anyway is:

for (let id of aIds) delete addonStates[id];

@@ +3007,5 @@
>          knownLocations.splice(pos, 1);
>          let addons = XPIDatabase.getAddonsInLocation(installLocation.name);
> +
> +        // If aIds was passed, filter out add-ons not in aIds
> +        if (aIds && aIds instanceof Array && aIds.length > 0) {

As above

@@ -3087,5 @@
>      this.persistBootstrappedAddons();
>  
>      // Clear out any cached migration data.
>      XPIDatabase.migrateData = null;
> -

Leave this blank line please

@@ +3421,5 @@
> +   *        A callback that will be passed the add-on
> +   */
> +  addInstalledAddon: function XPI_addInstalledAddon(aFile, aCallback) {
> +    LOG("XPI_addInstalledAddon");
> +    let dir = aFile.clone().parent;

No need for clone() here

@@ +3427,5 @@
> +      // Parent directory doesn't exist
> +      ERROR("File passed doesn't have a parent directory.");
> +      aCallback(null);
> +      return;
> +    }

Better to test if aFile exists. If it does its parent certainly does. If it doesn't then we have other problems

@@ +3468,5 @@
> +          break;
> +        }
> +      }
> +    }
> +#endif

I think rather than splitting these up you should just iterate all the install locations that aren't KEY_APP_PROFILE

@@ +3480,5 @@
> +      // Load the list of bootstrapped add-ons first so processFileChanges can
> +      // modify it
> +      try {
> +        this.bootstrappedAddons = JSON.parse(Prefs.getCharPref(PREF_BOOTSTRAP_ADDONS,
> +                                             "{}"));

This should already be loaded, why does it need to be loaded again?

@@ +3514,5 @@
> +            extensionListChanged = this.processFileChanges(state, {},
> +                                                           false,
> +                                                           Services.appinfo.version,
> +                                                           Services.appinfo.platformVersion,
> +                                                           [id]);

I think this will fail to send out the onExternalInstall/onInstalling/onInstalled functions that FHR and the UI expects to see when new stuff appears. It will also set the install cache to the current state so if any other add-ons have had changes they will be missed during the next startup.

It might be better to just do the addition to the database directly rather than the added complication of this, though that would also mean scanning to make sure this add-on is visible

@@ +6853,5 @@
> +   * @param aFileName
> +   *        The name of the file (or directory) corresponding to the add-on
> +   * @return The id of the add-on if such a file actually exists.
> +   */
> +  lookUpAddon: function DirInstallLocation_lookUpAddon(aFileName) {

Seems like it would be simpler to just reload the map by calling _readAddons and then check it exists by calling getIDForLocation.

@@ +7252,5 @@
> +   * @param aFile
> +   *        An nsIFile corresponding to the add-on
> +   * @return The id of the add-on if such an entry actually exists.
> +   */
> +  lookUpAddon: function RegInstallLocation_lookUpAddon(aFile) {

Same as above basically

::: toolkit/mozapps/extensions/commandLine.js
@@ +17,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow",
> +                                  "resource:///modules/RecentWindow.jsm");
> +
> +function CommandLineHandler() {
> +}

This is all browser-specific code and should probably go in browser/components/nsBrowserContentHandler.js instead

@@ +30,5 @@
> +      // returns true if there were conflicts
> +      function checkConflicts() {
> +        let noRemote = aCmdLine.handleFlag("no-remote", false);
> +        if (noRemote) {
> +          Components.utils.reportError("-install-global-addon flag isn't used with -no-remote");

Not sure what this is about

@@ +56,5 @@
> +          AddonManager.addInstalledAddon(file, function(aId){
> +            if (aId) {
> +              // An ID is returned only if the add-on actually exists.
> +              // Show the opt-in screen.
> +              win.openUILinkIn("about:newaddon?id=" + aId, "tab");

I think we should talk to UX about whether jumping straight to the opt-in screen is the right idea, it is a little invasive.

::: toolkit/mozapps/extensions/content/newaddon.js
@@ +126,5 @@
>  
>    document.getElementById("allow").disabled = false;
>    document.getElementById("buttonDeck").selectedPanel = document.getElementById("continuePanel");
>  }
> +

Unnecessary change
Attachment #767901 - Flags: feedback?(dtownsend+bugmail) → feedback+
> 
> > And if we use the notification box I mentioned in comment 7 will we still to
> > submit it for UX review?
> 
> Yes, anything that introduces new UI will need to go through UX review.

Hi there- we are aware of this bug and will require review. Can someone take the time to sit down with someone on our team to talk this over sometime soon before this goes further into development?

Also, might be good to have product team take a look.

Thanks!
Jaime
Flags: needinfo?(ffos-product)
(In reply to jachen from comment #14)
> Hi there- we are aware of this bug and will require review. Can someone take
> the time to sit down with someone on our team to talk this over sometime
> soon before this goes further into development?

Sure, please contact me and we'll schedule a meeting.

> Also, might be good to have product team take a look.

Yeah, we've talked to Asa about this, but we haven't had a formal discussion.
(In reply to Dave Townsend (:Mossop) from comment #13)
> @@ +30,5 @@
> > +      // returns true if there were conflicts
> > +      function checkConflicts() {
> > +        let noRemote = aCmdLine.handleFlag("no-remote", false);
> > +        if (noRemote) {
> > +          Components.utils.reportError("-install-global-addon flag isn't used with -no-remote");
> 
> Not sure what this is about

That was to make sure -install-global-addon flag isn't used with -no-remote. I thought it wouldn't make sense to use that combination since this flag *looks* for existing instances of Firefox.

> @@ +56,5 @@
> > +          AddonManager.addInstalledAddon(file, function(aId){
> > +            if (aId) {
> > +              // An ID is returned only if the add-on actually exists.
> > +              // Show the opt-in screen.
> > +              win.openUILinkIn("about:newaddon?id=" + aId, "tab");
> 
> I think we should talk to UX about whether jumping straight to the opt-in
> screen is the right idea, it is a little invasive.

But if we don't show it and silently add it, it wouldn't even show the next time the application starts because we'll have added the add-on to the db already and this add-on won't count as startup changed.
Depends on: 640775
Comment on attachment 767901 [details] [diff] [review]
WIP patch fo installation

Review of attachment 767901 [details] [diff] [review]:
-----------------------------------------------------------------

Waiting for Dave's comments to be addressed.
Attachment #767901 - Flags: feedback?(bmcbride)
I've addressed most of Mossop's comments.

- I retained the lookUpAddons methods in DirectoryInstallLocation and WinRegInstallLocation classes because using _readAddons instead of them completely reloads the File-ID maps instead of just what is required. It may end up finding other new add-ons than what was passed to the flag. When the install cache is updated with the new state, it would contain the other new add-ons too. Using lookUpAddon solves this problem.

- When we call onExternalInstall before adding the add-on to the DB (this is the correct way), the UI that listens to that event will find that the add-on is pending an install (which is true when the event is dispatched). But when we add the add-on to the DB, we need some way of canceling out that pending state. My plan was to make the UI listen to onInstalled and replace the wrapper of AddonInternal that the UI has, with a new wrapper of DBAddonInternal. But add-on listeners in the UI are registered per add-on. ie, there isn't a direct way to change the add-on object it is listening to. I've used a messy way to do that (there is a comment) to show my intention. How else might I achieve this?

The uninstallation part of this bug depends on bug 640775 and so I've not included that in the patch.
Attachment #767901 - Attachment is obsolete: true
Attachment #801094 - Flags: feedback?(dtownsend+bugmail)
Attachment #801094 - Flags: feedback?(bmcbride)
Note that I'm on PTO till September 15th so I won't get to this before then.
Comment on attachment 801094 [details] [diff] [review]
wip patch for installation (2)

Review of attachment 801094 [details] [diff] [review]:
-----------------------------------------------------------------

This is coming along, there are some logic problems here where the callback won't get called and some cases where we can simplify a lot of the code.

::: browser/components/nsBrowserContentHandler.js
@@ +535,5 @@
> +            // database.
> +            AddonManager.addInstalledAddon(file, function(aAddon){
> +              if (aAddon) {
> +                // An add-on object is returned only if the add-on exists and is
> +                // visible.

What is this if statement here for?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3444,5 @@
> +                                                 requiresRestart);
> +      }
> +      AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, requiresRestart);
> +
> +      let requiresRestart = XPIProvider.installRequiresRestart(newAddon);

You also need to check if disabling the existing add-on will require a restart

@@ +3458,5 @@
> +        let installReason;
> +
> +        // If the add-on will replace a bootstrapped add-on we must unload the
> +        // old add-on.
> +        if (newDBAddon.id in XPIProvider.bootstrappedAddons) {

This ignores the case where the existing add-on is already disabled, in which case you stull need to call the uninstall method.

@@ +3480,5 @@
> +          XPIProvider.unloadBootstrapScope(newDBAddon.id);
> +        }
> +
> +        if (!requiresRestart)
> +          flushStartupCache();

This only needs to happen if there was an existing add-on

@@ +3527,5 @@
> +        continue;
> +      let installLocation = this.installLocationsByName[locationKey];
> +      // Try to load just this add-on into the location's map.
> +      if (installLocation instanceof DirectoryInstallLocation) {
> +        if (installLocation.directory.equals(parent))

Put this logic into the DirectoryInstallLocation.lookUpAddon

@@ +3562,5 @@
> +
> +    let restartLess;
> +    if (!updateDatabase) {
> +      aCallback(null);
> +      return;

I don't know if these checks are really necessary. We have an external app telling us that a new add-on has been installed, we should just load the database, this isn't during startup so we don't have the same urgency around reducing file I/O

@@ +3573,5 @@
> +        for (let addon of oldAddons) {
> +          if (addon.id === id) {
> +            LOG("Add-on already exists in database.");
> +            aCallback(null);
> +            return;

What if the add-on has changed?

@@ +3604,5 @@
> +              }
> +            }
> +          }
> +        }
> +      }

I think this can be simplified. Just call XPIDatabase.getVisibleAddonForID for the add-ons ID. If there is a visible add-on then check if it's install location is a higher or lower priority than the one being installed.

@@ +3618,5 @@
> +
> +      states = this.getInstallLocationStates();
> +      // Cache the new install location states
> +      let cache = JSON.stringify(states);
> +      Services.prefs.setCharPref(PREF_INSTALL_CACHE, cache);

This is going to make Firefox ignore any other add-ons that have been installed in the filesystem the next time it starts up. I think you shouldn't set this pref at all, the next startup will be a little slower as Firefox has to check the installed add-ons again but it should be safer.

@@ +3642,5 @@
> +    }
> +    if (!restartLess) {
> +      // If it is not a restart-less add-on send a startupcache-invalidate
> +      // signal
> +      flushStartupCache();

You already call this in addMetadata, it shouldn't be necessary again

@@ +3653,5 @@
> +        if (aAddon && aAddon.userDisabled &&
> +            (aAddon.permissions & AddonManager.PERM_CAN_ENABLE)) {
> +          // Call the callback with the new add-on if all went well.
> +          aCallback(aAddon);
> +        }

You're not calling the callback in some cases here

@@ +3656,5 @@
> +          aCallback(aAddon);
> +        }
> +      });
> +    } else {
> +      aCallback(null);

Shouldn't you be passing the new add-on here too?

@@ +6861,5 @@
> +   * @param aFileName
> +   *        The name of the file (or directory) corresponding to the add-on
> +   * @return The id of the add-on if such a file actually exists.
> +   */
> +  lookUpAddon: function DirInstallLocation_lookUpAddon(aFileName) {

All this function needs to do is call _readAddons then getIDForLocation

@@ +7262,5 @@
> +   * @param aFile
> +   *        An nsIFile corresponding to the add-on
> +   * @return The id of the add-on if such an add-on actually exists.
> +   */
> +  lookUpAddon: function RegInstallLocation_lookUpAddon(aFile) {

Ditto
Attachment #801094 - Flags: feedback?(dtownsend+bugmail) → feedback+
(In reply to Dave Townsend (:Mossop) from comment #20)
> Comment on attachment 801094 [details] [diff] [review]
> wip patch for installation (2)
> 
> Review of attachment 801094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is coming along, there are some logic problems here where the callback
> won't get called and some cases where we can simplify a lot of the code.
> 
> ::: browser/components/nsBrowserContentHandler.js
> @@ +535,5 @@
> > +            // database.
> > +            AddonManager.addInstalledAddon(file, function(aAddon){
> > +              if (aAddon) {
> > +                // An add-on object is returned only if the add-on exists and is
> > +                // visible.
> 
> What is this if statement here for?

Since we don't know if we should show the op-in screen or not, I just left the if statement empty, suggesting something could be done with the add-on object returned. Yes it shouldn't be like this.

> @@ +3573,5 @@
> > +        for (let addon of oldAddons) {
> > +          if (addon.id === id) {
> > +            LOG("Add-on already exists in database.");
> > +            aCallback(null);
> > +            return;
> 
> What if the add-on has changed?

So the flag should cover add-on updates too. I had thought only of adding new ones.

> @@ +3618,5 @@
> > +
> > +      states = this.getInstallLocationStates();
> > +      // Cache the new install location states
> > +      let cache = JSON.stringify(states);
> > +      Services.prefs.setCharPref(PREF_INSTALL_CACHE, cache);
> 
> This is going to make Firefox ignore any other add-ons that have been
> installed in the filesystem the next time it starts up. I think you
> shouldn't set this pref at all, the next startup will be a little slower as
> Firefox has to check the installed add-ons again but it should be safer.
> 

I hope you didn't miss my comment 18. lookUpAddon adds only this add-on to its File-ID maps. Since what goes into the pref is what is in the install location ID to File map of every install location, I think this cause what you say.
I tested this and found that other new add-ons don't sneak into the pref.

> @@ +3642,5 @@
> > +    }
> > +    if (!restartLess) {
> > +      // If it is not a restart-less add-on send a startupcache-invalidate
> > +      // signal
> > +      flushStartupCache();
> 
> You already call this in addMetadata, it shouldn't be necessary again
> 
> @@ +3653,5 @@
> > +        if (aAddon && aAddon.userDisabled &&
> > +            (aAddon.permissions & AddonManager.PERM_CAN_ENABLE)) {
> > +          // Call the callback with the new add-on if all went well.
> > +          aCallback(aAddon);
> > +        }
> 
> You're not calling the callback in some cases here

I wrote this with the opt-in screen in mind. Shall I then return the add-on as long as it isn't null?

> @@ +3656,5 @@
> > +          aCallback(aAddon);
> > +        }
> > +      });
> > +    } else {
> > +      aCallback(null);
> 
> Shouldn't you be passing the new add-on here too?

What would the caller want to do with an invisible add-on? Since this method is exposed to the public, I thought we wouldn't want to return such add-on objects.

> @@ +6861,5 @@
> > +   * @param aFileName
> > +   *        The name of the file (or directory) corresponding to the add-on
> > +   * @return The id of the add-on if such a file actually exists.
> > +   */
> > +  lookUpAddon: function DirInstallLocation_lookUpAddon(aFileName) {
> 
> All this function needs to do is call _readAddons then getIDForLocation

But _readAddons reloads the entire File-ID maps with all the add-ons present. It would learn of other new add-ons also.


Also, could you answer the other question in comment 18?
Comment on attachment 777278 [details]
Screenshot of notification that requests a restart

Redirecting review since it was dead in the water when assigned to ux-review@.
Attachment #777278 - Flags: review?(ux-review) → review?(dhenein)
Sachin, are you likely to have more time to work on this or can we re-assign it to someone else?
Yes, I should be able to work on this.
I've been waiting for feedback.
Is it finalized that we want to land this feature?
(In reply to Sachin Hosmani [:sachin] from comment #24)
> Yes, I should be able to work on this.
> I've been waiting for feedback.

Ohai! Are you still able to pick this back up again, Sachin? If so, you should just work from Dave's latest feedback.

> Is it finalized that we want to land this feature?

I know we deferred a bit on deciding whether we actually wanted this as the approach for improving the externally installed add-ons situation, but IMO it's the most appropriate situation. And in addition, I'd like to use this to also help fix bug 923581 (which needs fixed in time for 31.0).
Blocks: 923581
Flags: needinfo?(sachinhosmani2)
Attachment #801094 - Flags: feedback?(bmcbride)
(And this has nothing to do with Firefox OS product...)
Flags: needinfo?(ffos-product)
(In reply to Blair McBride [:Unfocused] from comment #25)
> (In reply to Sachin Hosmani [:sachin] from comment #24)
> > Yes, I should be able to work on this.
> > I've been waiting for feedback.
> 
> Ohai! Are you still able to pick this back up again, Sachin? If so, you
> should just work from Dave's latest feedback.

Hi Blair :)
This is not really fresh in my mind, but we do have some unanswered questions in reply to Dave I have been waiting for.

Also, we don't have UX approval for the UI we want to show for uninstallation. Also, doesn't the uninstallation depend on bug 640775 ? All these add-ons are globally installed and their removal would require that bug fixed first.

> > Is it finalized that we want to land this feature?
> 
> I know we deferred a bit on deciding whether we actually wanted this as the
> approach for improving the externally installed add-ons situation, but IMO
> it's the most appropriate situation. And in addition, I'd like to use this
> to also help fix bug 923581 (which needs fixed in time for 31.0).

31? The trunk already has 33 if I'm right. Anyway, what time frame are you looking at?

I must say that I can't promise to put in much the coming month since I'm pretty busy for a month or two.
Flags: needinfo?(sachinhosmani2)
I can work on this now.

Blair, if I fix it, are we ready to review and land it?

Also, please can you give a look at comment 21?
Flags: needinfo?(bmcbride)
(In reply to Sachin Hosmani [:sachin] from comment #28)
> I can work on this now.
> 
> Blair, if I fix it, are we ready to review and land it?

\o/ Yes, but Irving is working on bug 1049142 at the moment (which will hopefully land soon). That bug re-does parts of how we handle scanning on startup, and how we handle the install cache. I think it'd be beneficial to talk to him about his changes.


(In reply to Sachin Hosmani [:sachin] from comment #21)
> Since we don't know if we should show the op-in screen or not

I think we should show the opt-in screen, but only if the add-on wasn't already installed. For upgrades, we can assume the person has already made a choice.



> > @@ +3653,5 @@
> > > +        if (aAddon && aAddon.userDisabled &&
> > > +            (aAddon.permissions & AddonManager.PERM_CAN_ENABLE)) {
> > > +          // Call the callback with the new add-on if all went well.
> > > +          aCallback(aAddon);
> > > +        }
> > 
> > You're not calling the callback in some cases here
> 
> I wrote this with the opt-in screen in mind. Shall I then return the add-on
> as long as it isn't null?

Yea - I think the callback API should be more generic than just how we want the opt-in screen to work today. Maybe add another param to describe what happened?

Something like:
  callback(const, addon)

Where the const can be something like EXTAPP_INSTALL_FAILED, EXTAPP_INSTALL_NEW_INSTALL, EXTAPP_INSTALL_UPGRADE, EXTAPP_INSTALL_ALREADY_INSTALLED (Warning: I didn't put much thought into these)

> 
> > @@ +3656,5 @@
> > > +          aCallback(aAddon);
> > > +        }
> > > +      });
> > > +    } else {
> > > +      aCallback(null);
> > 
> > Shouldn't you be passing the new add-on here too?
> 
> What would the caller want to do with an invisible add-on? Since this method
> is exposed to the public, I thought we wouldn't want to return such add-on
> objects.

Hmm, yea. Maybe we should pass in the visible add-on, and describe what happened via the other param I suggested above?

> > @@ +6861,5 @@
> > > +   * @param aFileName
> > > +   *        The name of the file (or directory) corresponding to the add-on
> > > +   * @return The id of the add-on if such a file actually exists.
> > > +   */
> > > +  lookUpAddon: function DirInstallLocation_lookUpAddon(aFileName) {
> > 
> > All this function needs to do is call _readAddons then getIDForLocation
> 
> But _readAddons reloads the entire File-ID maps with all the add-ons
> present. It would learn of other new add-ons also.

Could just add a filter param to _readAddons.


(In reply to Sachin Hosmani [:sachin] from comment #18)
> - When we call onExternalInstall before adding the add-on to the DB (this is
> the correct way), the UI that listens to that event will find that the
> add-on is pending an install (which is true when the event is dispatched).
> But when we add the add-on to the DB, we need some way of canceling out that
> pending state. My plan was to make the UI listen to onInstalled and replace
> the wrapper of AddonInternal that the UI has, with a new wrapper of
> DBAddonInternal. But add-on listeners in the UI are registered per add-on.
> ie, there isn't a direct way to change the add-on object it is listening to.
> I've used a messy way to do that (there is a comment) to show my intention.
> How else might I achieve this?

Hmm.... would it work allow AddonWrapper to swap the AddonInternal it uses for another? That way any external code will just be using the same wrapper object.
Flags: needinfo?(bmcbride)
Mentor: bmcbride
Depends on: 1049142
Comment on attachment 801094 [details] [diff] [review]
wip patch for installation (2)

Hey Irving, can you please leave some feedback as to what has changed like we spoke on IRC?
Attachment #801094 - Flags: feedback+ → feedback?(irving)
Comment on attachment 801094 [details] [diff] [review]
wip patch for installation (2)

Review of attachment 801094 [details] [diff] [review]:
-----------------------------------------------------------------

Sachin, sorry about disappearing in mid-conversation on Thursday. Unfortunately I'm not with Mozilla any more, so I won't be able to provide help; I've asked Blair to step back in and work with you.
Attachment #801094 - Flags: feedback?(irving) → feedback?(bmcbride)
Attachment #777278 - Flags: review?(dhenein)
Comment on attachment 801094 [details] [diff] [review]
wip patch for installation (2)

Review of attachment 801094 [details] [diff] [review]:
-----------------------------------------------------------------

Had a look through this again.. I don't think there's much I can add that hasn't already been said in previous comments. (And I lack context as to what you and Irving spoke about)

::: browser/components/nsBrowserContentHandler.js
@@ +545,5 @@
> +        } else {
> +          // The flag install-global-addon is of use only if an instance of the
> +          // app is open. Otherwise, a fresh start will anyway detect add-on
> +          // installation.
> +          dump("No existing instance of app found. This is a fresh startup.");

Hm, this isn't necessarily true :\ Can have non-browser windows open, and on OSX you can have no windows open - but the app still already running.

Instead, you can use nsICommandLine.state to check if this is an initial launch or not.
Attachment #801094 - Flags: feedback?(bmcbride) → feedback+
I have just discovered this bug. After some time it will be successfully finished and in the standard distribution. Does "external processes" apply to a Windows file manager e.g. Explorer ?

According to  "Installing extensions" part "Global installation", see :
http://kb.mozillazine.org/Installing_extensions
I'll have just to copy the extension.xpi file in <installation directory>\browser\extensions folder (by default c:\Program Files (x86)\Mozilla Firefox\browser\extensions\) with my administrator account (Firefox being closed). Is this futuristic ?

It says that each limited user profile will have the acceptance dialogue at next start except if I have first accepted with the administrator. Is this still valid ?

If now I erase the extension.xpi file with the administrator account, does it globally UN-install the global extension ? Does I need to close Firefox ? 
      
What about update of the globally installed extension ? Is it sufficient to update from the administrator account ? Or do I have to UN-install then install the new version ?

If a limited user wish to inactivate the global extension just for a particular profile (e.g. conflict with an other extension) what is the recommended method ? Locally install and disable the local copy ?
I'm a bit busy to work on this at the moment. I'll try to pick it up when I find time.
Assignee: sachinhosmani2 → nobody
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago → 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: