598979 - change "widget" module to use EventEmitter event registration model

Status: RESOLVED FIXED

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

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v1

Associated pull request: http://github.com/mozillalabs/jetpack-sdk/pull/17

This patch also includes fix for the widget test failures that occur with ff4b and f3.* now.

Comment 2

[Dietrich Ayala (:dietrich)]

What part of the patch addresses the test failures? If possible, it'd be better to split those changes out. If that's not possible, then just comment here about what caused the failure, and how you addressed it.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Please look at the pull request link I've posted the last commit message explains the it also it includes only changes addressing that. Separating patch won't be easy since change is done on top of other changes in this patch.

Comment 4

[Dietrich Ayala (:dietrich)]

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


>@@ -224,9 +260,8 @@ let browserManager = {
>   // propagating the change to all windows.
>   updateItem: function browserManager_updateItem(item, property, value) {
>     let idx = this.items.indexOf(item);
>-    if (idx == -1)
>-      throw new Error("The widget " + item + " cannot be updated because it is not currently registered.");
>-    this.windows.forEach(function (w) w.updateItem(item, property, value));
>+    if (idx != -1)
>+      this.windows.forEach(function (w) w.updateItem(item, property, value));
>   },

why are you changing this to silently fail?

r=me otherwise on this patch.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #4)
> Comment on attachment 482535 [details] [diff] [review]
> v1
> 
> 
> >@@ -224,9 +260,8 @@ let browserManager = {
> >   // propagating the change to all windows.
> >   updateItem: function browserManager_updateItem(item, property, value) {
> >     let idx = this.items.indexOf(item);
> >-    if (idx == -1)
> >-      throw new Error("The widget " + item + " cannot be updated because it is not currently registered.");
> >-    this.windows.forEach(function (w) w.updateItem(item, property, value));
> >+    if (idx != -1)
> >+      this.windows.forEach(function (w) w.updateItem(item, property, value));
> >   },
> 
> why are you changing this to silently fail?
> 
> r=me otherwise on this patch.

The reason is the assignments that happen in the constructors. With this check exception will be always thrown. Another thing that I noticed that if you can set properties while constructing a Widget there is nothing wrong with changing them even if it's not added yet since actual values will be propagated only when widget is added.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Please let me know whether this reasoning is acceptable or not.

Comment 7

[Dietrich Ayala (:dietrich)]

Yep, that's fine, thanks!

Comment 8

[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".