901517 - Observable Object: use EventEmitter.decorate and fix array issue

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: Patch v1

To easily inherit from ObservableObject, it's better to use EventEmitter.decorate. Also, we were returning the wrong path sometimes.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 787451 [details] [diff] [review]
Patch v1

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

Also I'm seing what looks like strict JS errors, can you set strict mode and fix eventual exeptions.
(Like `path` not being declared anywhere here http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/observable-object.js#107)

::: browser/devtools/shared/observable-object.js
@@ -43,4 @@
>  
>  function isObject(value) {
>    return Object(value) === value;
>  }

I think that's a bad way to check for objects.
I'm expecting to see failure if we get cross compartments objects.
Could you use this pattern instead?
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js?force=1#48
(This code has been written by bholley... I trust him when it comes to wrappers ;))

@@ -70,5 @@
>    unwrap: function(target, key, value) {
>      if (!isObject(value) || !this._wrappers.has(value)) {
>        return [value, this._paths.get(target).concat(key)];
>      }
> -    return [this._wrappers.get(value), this._paths.get(value)];

I'm new to ObservableObject implementation, I do not easily what it fixes,
what about having a test covering this fix?

Comment 3

[Paul Rouget [:paul]]

Attachment: Patch v1.1

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Also I'm seing what looks like strict JS errors, can you set strict mode and
> fix eventual exeptions.
> (Like `path` not being declared anywhere here
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> observable-object.js#107)

Addressed ("use strict").

> ::: browser/devtools/shared/observable-object.js
> @@ -43,4 @@
> >  
> >  function isObject(value) {
> >    return Object(value) === value;
> >  }
> 
> I think that's a bad way to check for objects.
> I'm expecting to see failure if we get cross compartments objects.
> Could you use this pattern instead?
> http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/
> specialpowersAPI.js?force=1#48
> (This code has been written by bholley... I trust him when it comes to
> wrappers ;))

wut? How can it be bad?

> @@ -70,5 @@
> >    unwrap: function(target, key, value) {
> >      if (!isObject(value) || !this._wrappers.has(value)) {
> >        return [value, this._paths.get(target).concat(key)];
> >      }
> > -    return [this._wrappers.get(value), this._paths.get(value)];
> 
> I'm new to ObservableObject implementation, I do not easily what it fixes,
> what about having a test covering this fix?

We were getting an exception when re-using existing wrapped objects into new properties. I have added some more tests to cover this.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 788918 [details] [diff] [review]
Patch v1.1

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

> > ::: browser/devtools/shared/observable-object.js
> > @@ -43,4 @@
> > >  
> > >  function isObject(value) {
> > >    return Object(value) === value;
> > >  }
> > 
> > I think that's a bad way to check for objects.
> > I'm expecting to see failure if we get cross compartments objects.
> > Could you use this pattern instead?
> > http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/
> > specialpowersAPI.js?force=1#48
> > (This code has been written by bholley... I trust him when it comes to
> > wrappers ;))
> 
> wut? How can it be bad?

If you have a wrapper at some point, your isObject function will behave incorrectly.
You can see that by executing this in the (old) error console:
  JObject(content) == content 
  false

Please tweak this method, that would be unfortunate to loose time on wrapper issues...

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v2

Comment 6

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=fdf537f42bb5

Comment 7

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/fde7ed54c2cb

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fde7ed54c2cb