> On 2012-05-04 15:10:34, Ryan Baxter wrote: > > trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js, > > line 160 > > <https://reviews.apache.org/r/4976/diff/6/?file=106670#file106670line160> > > > > you should just return null, or you can let the function return > > undefined. Right now the function could return a string, null, or > > undefined.
Good point. > On 2012-05-04 15:10:34, Ryan Baxter wrote: > > trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js, > > line 150 > > <https://reviews.apache.org/r/4976/diff/6/?file=106670#file106670line150> > > > > If this function is supposed to be private you should move it outside > > of the osapi.container.Container.addMixin. Actually the other "private" > > functions in here should probably be moved there as well. It should be ok for the purpose of this CR. I wont change this now so if need to refactor it later it will be in one separate patch request. > On 2012-05-04 15:10:34, Ryan Baxter wrote: > > trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js, > > line 79 > > <https://reviews.apache.org/r/4976/diff/6/?file=106670#file106670line79> > > > > is there a reason why you separate out associatedContext? You check > > for the associatedContext in opt_containerContext and set the value to > > associatedContext then after you loops is done you just add it to > > openSocialContext. Why not remove that special check for the associated > > context and just have the "else" part take care of it for you? The idea is if container has extra context, the openSocial namespace should have at least associatedContext attribute per spec. I should probably set the empty associatedContext early and let the rest iteration take care of it. > On 2012-05-04 15:10:34, Ryan Baxter wrote: > > trunk/content/samplecontainer/examples/embeddedexperiences/EEContainer.js, > > line 179 > > <https://reviews.apache.org/r/4976/diff/6/?file=106667#file106667line179> > > > > Did you mean 'or' instead of 'and'? I think it would also be good to > > create constants for target type and display type that the container can > > use. No it was intended to check if EE is gadget but I realize that the EEContainer.js also should be able to render URL based EE. I will just remove the check for gadget type. - Henry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4976/#review7557 ----------------------------------------------------------- On 2012-05-04 07:28:05, Henry Saputra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4976/ > ----------------------------------------------------------- > > (Updated 2012-05-04 07:28:05) > > > Review request for shindig and Ryan Baxter. > > > Summary > ------- > > Add code to handle EE extension for OpenSocial 2.5.0 core gadget spec: > http://docs.opensocial.org/display/OSD/More+precision+for+EE+data+model > http://docs.opensocial.org/display/OSD/Core-Gadget+-+Embedded+Experiences > > I remove the unneeded model for EmbeddedExperience because ActivityEntry uses > ExtendableBean, which act like a Map, to store opensocial and its extension > so there is no need to explicitly map embed or preferredExperience with > physical class: > java/social-api/src/main/java/org/apache/shindig/social/core/model/EmbeddedExperienceImpl.java > java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/EmbeddedExperience.java > > > This addresses bug SHINDIG-1762. > https://issues.apache.org/jira/browse/SHINDIG-1762 > > > Diffs > ----- > > trunk/content/samplecontainer/examples/embeddedexperiences/BlogViewer.xml > PRE-CREATION > trunk/content/samplecontainer/examples/embeddedexperiences/EEContainer.js > 1333592 > trunk/content/sampledata/canonicaldb.json 1333592 > trunk/features/src/main/javascript/features/embeddedexperiences/constant.js > 1333592 > > trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_container.js > 1333592 > > trunk/features/src/main/javascript/features/embeddedexperiences/embedded_experiences_gadgets.js > 1333592 > trunk/features/src/main/javascript/features/embeddedexperiences/feature.xml > 1333592 > > trunk/features/src/main/javascript/features/open-views.ee/open-views-ee-container.js > 1333592 > > trunk/features/src/test/javascript/features/embeddedexperiences/embedded_experiences_container_test.js > 1333592 > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/model/EmbeddedExperienceImpl.java > 1333592 > > trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/EmbeddedExperience.java > 1333592 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/RestfulJsonActivityEntryTest.java > 1333592 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/fixtures/ActivityEntryJsonDelete.json > 1333592 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/fixtures/ActivityEntryJsonGroup.json > 1333592 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialServiceTest.java > 1333592 > > Diff: https://reviews.apache.org/r/4976/diff > > > Testing > ------- > > Updated the unit tests and modify EE sample gadget to test passing additional > context and respect preferredExperience display link text. > > > Thanks, > > Henry > >
