> 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 > >
