567228 - after uninstalling a widget add-on on trunk, some widgets remain until app is restarted

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

i think this should block 0.4. the widget bar is also still visible, and should go away when there are no widgets.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

remove all widgets and the add-on bar at uninstall.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 446615 [details] [diff] [review]
v1

>+  get container() {
>+    if (!this._container)
>+      this._createContainer();

Nit: you could combine the container getter and the _createContainer function, as one just wraps the other; you would just need to have _init call this.container rather than this._createContainer to add the container on initialization.  Alternately, perhaps _init doesn't actually need to add the container, because whatever needs it will just call this.container to access it, which will lazily create it?


>+    if (!this._container)
>+      throw new Error("Unable to add widget container to the application window");

Nit: by my reading of _createContainer, this._container cannot possibly evaluate to false at this point, because _createContainer will throw (thus returning before this line is evaluated) if anything prevents this._container from being assigned to a DOM object, which makes this line unnecessary.


>+  // Remove container
>+  _removeContainer: function BW__removeContainer() {
>+    if (this.container)
>+      this.container.parentNode.removeChild(this.container);
>   },

Nit: I would make this check for this._container rather than this.container, since this.container will create the container if it doesn't already exist (although _removeContainer will never be called if the container doesn't exist in the current version of the code--but see below for a suggested simplification that would change that).


>-    this._items.forEach(function(item) this.removeItems([item.widget]), this);
>+    let len = this._items.length;
>+    for (var i = 0; i < len; i++)
>+      this.removeItems([this._items[0].widget]);

This took me a few seconds to figure out!  I didn't expect a loop across the indexes of an array whose body accessed the same array's indexes but didn't actually access the ones being looped over.

Nevertheless, it'll work fine, although you could simplify to:

  this.removeItems(this._items.slice[0]);

However, note that this would call removeItems whether or not there are any items to remove (in which case you would want to make sure to also make the change noted in the previous nit of removing this._container rather than this.container in _removeItems).

If keeping this the same, however, then nit: since you are switching from var to let in other places, it would make sense to do so when declaring "i" in this for loop as well.

Comment 3

[Dietrich Ayala (:dietrich)]

thanks! pushed: http://hg.mozilla.org/labs/jetpack-sdk/rev/703e6e37847b

(In reply to comment #2)
> Nit: you could combine the container getter and the _createContainer function,

done

> Alternately, perhaps _init doesn't actually need to add the
> container, because whatever needs it will just call this.container to access
> it, which will lazily create it?

done also

note: i also made some test changes to get them to work with lazy creation of the container.


> Nit: I would make this check for this._container rather than this.container,
> since this.container will create the container if it doesn't already exist
> (although _removeContainer will never be called if the container doesn't exist
> in the current version of the code--but see below for a suggested
> simplification that would change that).

done, for the first part. couldn't make the 2nd change, see below.

> Nevertheless, it'll work fine, although you could simplify to:
> 
>   this.removeItems(this._items.slice[0]);

the widget is being passed in, not the whole cached widget data entry in _items. it's a bit klunky, but allows re-use of the public removeItems api instead of making a custom one just for destroy to use.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> the widget is being passed in, not the whole cached widget data entry in
> _items. it's a bit klunky, but allows re-use of the public removeItems api
> instead of making a custom one just for destroy to use.

Aha!  In that case, either of these would have also worked:

  this.removeItems([item.widget for (item in this._items)]);

  this.removeItems(this._items.map(function (item) item.widget));

Comment 5

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".