You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by felix lee <fe...@gmail.com> on 2011/05/17 02:20:21 UTC
Review Request: fix osapi taming for caja es53
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/
-----------------------------------------------------------
Review request for shindig.
Summary
-------
1. osapi dynamically defined methods are now tamed correctly.
2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
Diffs
-----
/trunk/features/src/main/javascript/features/caja/feature.xml 1103952
/trunk/features/src/main/javascript/features/caja/taming.js 1103952
/trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
/trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
/trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
/trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
/trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
/trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
/trunk/features/src/test/javascript/features/alltests.js 1103952
/trunk/features/src/test/javascript/features/mocks/env.js 1103952
Diff: https://reviews.apache.org/r/747/diff
Testing
-------
"mvn test" passes
sample osapi gadget loaded in sample container works (mostly).
Thanks,
felix
Re: Review Request: fix osapi taming for caja es53
Posted by Paul Lindner <li...@inuus.com>.
> On 2011-05-17 00:56:02, Paul Lindner wrote:
> > LGTM -- Tests pass
> >
> > How risky is the json parsing change? Did you test this out in various browsers?
> >
> >
>
> felix lee wrote:
> it's not very risky, the code path only happens if Caja is enabled, and prior to this Caja didn't really work. the change to gadgets.json.stringify makes it use the ES5 definition of JSON.stringify, which everyone has converged to.
>
> at the moment, this patch makes Chrome and Safari work for the cajoled sample gadget I'm testing. Firefox and Opera are still broken, which I'll fix in a subsequent patch. (they still work fine if I turn off caja.) I haven't looked at IE yet.
okay, that's good. I'll commit now.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/#review678
-----------------------------------------------------------
On 2011-05-17 00:20:21, felix lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/747/
> -----------------------------------------------------------
>
> (Updated 2011-05-17 00:20:21)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> 1. osapi dynamically defined methods are now tamed correctly.
>
> 2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
>
> 3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
>
>
> Diffs
> -----
>
> /trunk/features/src/main/javascript/features/caja/feature.xml 1103952
> /trunk/features/src/main/javascript/features/caja/taming.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
> /trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
> /trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
> /trunk/features/src/test/javascript/features/alltests.js 1103952
> /trunk/features/src/test/javascript/features/mocks/env.js 1103952
>
> Diff: https://reviews.apache.org/r/747/diff
>
>
> Testing
> -------
>
> "mvn test" passes
> sample osapi gadget loaded in sample container works (mostly).
>
>
> Thanks,
>
> felix
>
>
Re: Review Request: fix osapi taming for caja es53
Posted by felix lee <fe...@gmail.com>.
> On 2011-05-17 00:56:02, Paul Lindner wrote:
> > LGTM -- Tests pass
> >
> > How risky is the json parsing change? Did you test this out in various browsers?
> >
> >
it's not very risky, the code path only happens if Caja is enabled, and prior to this Caja didn't really work. the change to gadgets.json.stringify makes it use the ES5 definition of JSON.stringify, which everyone has converged to.
at the moment, this patch makes Chrome and Safari work for the cajoled sample gadget I'm testing. Firefox and Opera are still broken, which I'll fix in a subsequent patch. (they still work fine if I turn off caja.) I haven't looked at IE yet.
- felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/#review678
-----------------------------------------------------------
On 2011-05-17 00:20:21, felix lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/747/
> -----------------------------------------------------------
>
> (Updated 2011-05-17 00:20:21)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> 1. osapi dynamically defined methods are now tamed correctly.
>
> 2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
>
> 3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
>
>
> Diffs
> -----
>
> /trunk/features/src/main/javascript/features/caja/feature.xml 1103952
> /trunk/features/src/main/javascript/features/caja/taming.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
> /trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
> /trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
> /trunk/features/src/test/javascript/features/alltests.js 1103952
> /trunk/features/src/test/javascript/features/mocks/env.js 1103952
>
> Diff: https://reviews.apache.org/r/747/diff
>
>
> Testing
> -------
>
> "mvn test" passes
> sample osapi gadget loaded in sample container works (mostly).
>
>
> Thanks,
>
> felix
>
>
Re: Review Request: fix osapi taming for caja es53
Posted by Paul Lindner <li...@inuus.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/#review678
-----------------------------------------------------------
Ship it!
LGTM -- Tests pass
How risky is the json parsing change? Did you test this out in various browsers?
- Paul
On 2011-05-17 00:20:21, felix lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/747/
> -----------------------------------------------------------
>
> (Updated 2011-05-17 00:20:21)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> 1. osapi dynamically defined methods are now tamed correctly.
>
> 2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
>
> 3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
>
>
> Diffs
> -----
>
> /trunk/features/src/main/javascript/features/caja/feature.xml 1103952
> /trunk/features/src/main/javascript/features/caja/taming.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
> /trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
> /trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
> /trunk/features/src/test/javascript/features/alltests.js 1103952
> /trunk/features/src/test/javascript/features/mocks/env.js 1103952
>
> Diff: https://reviews.apache.org/r/747/diff
>
>
> Testing
> -------
>
> "mvn test" passes
> sample osapi gadget loaded in sample container works (mostly).
>
>
> Thanks,
>
> felix
>
>
Re: Review Request: fix osapi taming for caja es53
Posted by felix lee <fe...@gmail.com>.
> On 2011-05-17 00:50:02, felix lee wrote:
> > oops, I was looking at the wrong window. not all tests pass. fixing...
no, I was right the first time, misread the output. tests pass cleanly.
- felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/#review676
-----------------------------------------------------------
On 2011-05-17 00:20:21, felix lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/747/
> -----------------------------------------------------------
>
> (Updated 2011-05-17 00:20:21)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> 1. osapi dynamically defined methods are now tamed correctly.
>
> 2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
>
> 3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
>
>
> Diffs
> -----
>
> /trunk/features/src/main/javascript/features/caja/feature.xml 1103952
> /trunk/features/src/main/javascript/features/caja/taming.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
> /trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
> /trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
> /trunk/features/src/test/javascript/features/alltests.js 1103952
> /trunk/features/src/test/javascript/features/mocks/env.js 1103952
>
> Diff: https://reviews.apache.org/r/747/diff
>
>
> Testing
> -------
>
> "mvn test" passes
> sample osapi gadget loaded in sample container works (mostly).
>
>
> Thanks,
>
> felix
>
>
Re: Review Request: fix osapi taming for caja es53
Posted by felix lee <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/747/#review676
-----------------------------------------------------------
oops, I was looking at the wrong window. not all tests pass. fixing...
- felix
On 2011-05-17 00:20:21, felix lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/747/
> -----------------------------------------------------------
>
> (Updated 2011-05-17 00:20:21)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> 1. osapi dynamically defined methods are now tamed correctly.
>
> 2. the browser-native gadgets.json.stringify had a bug in the replacer function. for keys that should be ignored, it was returning null instead of undefined, so a value like {a___:3} was being stringified as {'a___':null} rather than {}
>
> 3. features/alltests.js didn't work, because it was out of sync with reality. I updated it to match the jsunit rule in pom.xml.
>
>
> Diffs
> -----
>
> /trunk/features/src/main/javascript/features/caja/feature.xml 1103952
> /trunk/features/src/main/javascript/features/caja/taming.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-jsimpl.js 1103952
> /trunk/features/src/main/javascript/features/core.json/json-native.js 1103952
> /trunk/features/src/main/javascript/features/opensocial-reference/taming.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/feature.xml 1103952
> /trunk/features/src/main/javascript/features/osapi.base/osapi.js 1103952
> /trunk/features/src/main/javascript/features/osapi.base/taming.js 1103952
> /trunk/features/src/test/javascript/features/alltests.js 1103952
> /trunk/features/src/test/javascript/features/mocks/env.js 1103952
>
> Diff: https://reviews.apache.org/r/747/diff
>
>
> Testing
> -------
>
> "mvn test" passes
> sample osapi gadget loaded in sample container works (mostly).
>
>
> Thanks,
>
> felix
>
>