Code review request for View Enhancements To Open Gadgets and URLs

Please review code changes for the View/Sites Enhancements To Support Opening Of Gadgets and URLs.

Link to the spec changes: http://code.google.com/p/opensocial-resources/issues/detail?id=1167

Code changes include updates in the common container sample code with new media sample gadget which uses these new APIs to open gadget/url in dialog or tab.

[jasonchiangc, 2011-06-20 18:10:54]

Please review code changes for the View/Sites Enhancements To Support Opening Of
Gadgets and URLs.

Link to the spec changes:
http://code.google.com/p/opensocial-resources/issues/detail?id=1167

Code changes include updates in the common container sample code with new media
sample gadget which uses these new APIs to open gadget/url in dialog or tab.

[Paul Lindner, 2011-07-07 16:03:37]

First pass -- didn't look at tests or examples closely.

Please run the js code through gjslint, that will catch most of the issues I
pointed out.

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
File content/samplecontainer/examples/commoncontainer/cconviews.js (right):

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:29: * @param
{string}
{string=} for optional params here and throughout

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:140: $("#dialog_"
+ dialog_counter).dialog({
do we really need jquery here?

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:298: "ui-widget
ui-widget-content ui-helper-clearfix ui-corner-all").find(".portlet-header")
I think all of these css classes here and throughout should have a unique prefix
so that they don't possibly clash.  Bonus points if you can make the prefix
configurable..

http://codereview.appspot.com/4641058/diff/1/features/pom.xml
File features/pom.xml (right):

http://codereview.appspot.com/4641058/diff/1/features/pom.xml#newcode9
features/pom.xml:9: * with the License.  You may obtain a copy of the License at
chunk mismatch here.. weird..

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/views/viewenhancements-container.js
(right):

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:67: *
@param {Object}
Object.<string, string|Object>=

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:85: if
(orig_site != undefined && orig_site.getActiveGadgetHolder() != undefined){
prefer !== form where possible.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:150: *
         url: URL to a web page to open in a URL site in the container. (Note
this should not
bad jsdoc format.  Try running gjslint to get some help.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:155: *
@param {string}
string= here and throughout for opt_ params

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/views/viewenhancements.js (right):

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:22: 
These are probably not needed if you have the deps set up correctly.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:66: * @param
{string}
{string=}

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:89: * @param
retValue:
Type?  {Object}?

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:100: * @param
resultCallback:
{function} here I believe.

[rbaxter85, 2011-07-08 00:38:14]

Thanks for making a first pass Paul.  Is this the tool your referring to?
http://code.google.com/p/closure-linter/source/browse/trunk/closure_linter/gj...

On 2011/07/07 16:03:37, Paul Lindner wrote:
> First pass -- didn't look at tests or examples closely.
> 
> Please run the js code through gjslint, that will catch most of the issues I
> pointed out.
> 
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> File content/samplecontainer/examples/commoncontainer/cconviews.js (right):
> 
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> content/samplecontainer/examples/commoncontainer/cconviews.js:29: * @param
> {string}
> {string=} for optional params here and throughout
> 
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> content/samplecontainer/examples/commoncontainer/cconviews.js:140:
$("#dialog_"
> + dialog_counter).dialog({
> do we really need jquery here?
> 
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> content/samplecontainer/examples/commoncontainer/cconviews.js:298: "ui-widget
> ui-widget-content ui-helper-clearfix ui-corner-all").find(".portlet-header")
> I think all of these css classes here and throughout should have a unique
prefix
> so that they don't possibly clash.  Bonus points if you can make the prefix
> configurable..
> 
> http://codereview.appspot.com/4641058/diff/1/features/pom.xml
> File features/pom.xml (right):
> 
> http://codereview.appspot.com/4641058/diff/1/features/pom.xml#newcode9
> features/pom.xml:9: * with the License.  You may obtain a copy of the License
at
> chunk mismatch here.. weird..
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> File features/src/main/javascript/features/views/viewenhancements-container.js
> (right):
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements-container.js:67:
*
> @param {Object}
> Object.<string, string|Object>=
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements-container.js:85:
if
> (orig_site != undefined && orig_site.getActiveGadgetHolder() != undefined){
> prefer !== form where possible.
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements-container.js:150:
*
>          url: URL to a web page to open in a URL site in the container. (Note
> this should not
> bad jsdoc format.  Try running gjslint to get some help.
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements-container.js:155:
*
> @param {string}
> string= here and throughout for opt_ params
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> File features/src/main/javascript/features/views/viewenhancements.js (right):
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements.js:22: 
> These are probably not needed if you have the deps set up correctly.
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements.js:66: * @param
> {string}
> {string=}
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements.js:89: * @param
> retValue:
> Type?  {Object}?
> 
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/views/viewenhancements.js:100: * @param
> resultCallback:
> {function} here I believe.

[rbaxter85, 2011-07-11 14:37:07]

Paul still working on updating the patch, you can expect a new patch sometime
this week.  Sorry for the delay.

On 2011/07/08 00:38:14, rbaxter85 wrote:
> Thanks for making a first pass Paul.  Is this the tool your referring to?
>
http://code.google.com/p/closure-linter/source/browse/trunk/closure_linter/gj...
> 
> On 2011/07/07 16:03:37, Paul Lindner wrote:
> > First pass -- didn't look at tests or examples closely.
> > 
> > Please run the js code through gjslint, that will catch most of the issues I
> > pointed out.
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> > File content/samplecontainer/examples/commoncontainer/cconviews.js (right):
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> > content/samplecontainer/examples/commoncontainer/cconviews.js:29: * @param
> > {string}
> > {string=} for optional params here and throughout
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> > content/samplecontainer/examples/commoncontainer/cconviews.js:140:
> $("#dialog_"
> > + dialog_counter).dialog({
> > do we really need jquery here?
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
> > content/samplecontainer/examples/commoncontainer/cconviews.js:298:
"ui-widget
> > ui-widget-content ui-helper-clearfix ui-corner-all").find(".portlet-header")
> > I think all of these css classes here and throughout should have a unique
> prefix
> > so that they don't possibly clash.  Bonus points if you can make the prefix
> > configurable..
> > 
> > http://codereview.appspot.com/4641058/diff/1/features/pom.xml
> > File features/pom.xml (right):
> > 
> > http://codereview.appspot.com/4641058/diff/1/features/pom.xml#newcode9
> > features/pom.xml:9: * with the License.  You may obtain a copy of the
License
> at
> > chunk mismatch here.. weird..
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > File
features/src/main/javascript/features/views/viewenhancements-container.js
> > (right):
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> >
features/src/main/javascript/features/views/viewenhancements-container.js:67:
> *
> > @param {Object}
> > Object.<string, string|Object>=
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> >
features/src/main/javascript/features/views/viewenhancements-container.js:85:
> if
> > (orig_site != undefined && orig_site.getActiveGadgetHolder() != undefined){
> > prefer !== form where possible.
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> >
features/src/main/javascript/features/views/viewenhancements-container.js:150:
> *
> >          url: URL to a web page to open in a URL site in the container.
(Note
> > this should not
> > bad jsdoc format.  Try running gjslint to get some help.
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> >
features/src/main/javascript/features/views/viewenhancements-container.js:155:
> *
> > @param {string}
> > string= here and throughout for opt_ params
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/views/viewenhancements.js
(right):
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/views/viewenhancements.js:22: 
> > These are probably not needed if you have the deps set up correctly.
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/views/viewenhancements.js:66: * @param
> > {string}
> > {string=}
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/views/viewenhancements.js:89: * @param
> > retValue:
> > Type?  {Object}?
> > 
> >
>
http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/views/viewenhancements.js:100: *
@param
> > resultCallback:
> > {function} here I believe.

[jasonchiangc, 2011-07-14 15:54:35]

new patch based on Paul's comments, thanks.

[jasonchiangc, 2011-07-14 16:01:03]

Have a new patch based on Paul's previous review.

Thanks,

Jason

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
File content/samplecontainer/examples/commoncontainer/cconviews.js (right):

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:29: * @param
{string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
> {string=} for optional params here and throughout

Done.

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:140: $("#dialog_"
+ dialog_counter).dialog({
On 2011/07/07 16:03:37, Paul Lindner wrote:
> do we really need jquery here?

I use jquery since sample container already import jquery library and it's easy
to use.

http://codereview.appspot.com/4641058/diff/1/content/samplecontainer/examples...
content/samplecontainer/examples/commoncontainer/cconviews.js:298: "ui-widget
ui-widget-content ui-helper-clearfix ui-corner-all").find(".portlet-header")
On 2011/07/07 16:03:37, Paul Lindner wrote:
> I think all of these css classes here and throughout should have a unique
prefix
> so that they don't possibly clash.  Bonus points if you can make the prefix
> configurable..

these are the same css classes used in sample container's viewController.js, I
prefer not to change the viewController code.

http://codereview.appspot.com/4641058/diff/1/features/pom.xml
File features/pom.xml (right):

http://codereview.appspot.com/4641058/diff/1/features/pom.xml#newcode9
features/pom.xml:9: * with the License.  You may obtain a copy of the License at
On 2011/07/07 16:03:37, Paul Lindner wrote:
> chunk mismatch here.. weird..

will have a new patch and hope it doesn't have this issue

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/views/viewenhancements-container.js
(right):

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:67: *
@param {Object}
On 2011/07/07 16:03:37, Paul Lindner wrote:
> Object.<string, string|Object>=

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:85: if
(orig_site != undefined && orig_site.getActiveGadgetHolder() != undefined){
On 2011/07/07 16:03:37, Paul Lindner wrote:
> prefer !== form where possible.

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:150: *
         url: URL to a web page to open in a URL site in the container. (Note
this should not
On 2011/07/07 16:03:37, Paul Lindner wrote:
> bad jsdoc format.  Try running gjslint to get some help.

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements-container.js:155: *
@param {string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
> string= here and throughout for opt_ params

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/views/viewenhancements.js (right):

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:22: 
On 2011/07/07 16:03:37, Paul Lindner wrote:
> These are probably not needed if you have the deps set up correctly.

remove the gadgets['views'] = gadgets['views'] || {};

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:66: * @param
{string}
On 2011/07/07 16:03:37, Paul Lindner wrote:
> {string=}

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:89: * @param
retValue:
On 2011/07/07 16:03:37, Paul Lindner wrote:
> Type?  {Object}?

Done.

http://codereview.appspot.com/4641058/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/views/viewenhancements.js:100: * @param
resultCallback:
On 2011/07/07 16:03:37, Paul Lindner wrote:
> {function} here I believe.

Done.

[jasonchiangc, 2011-07-19 23:20:56]

new patch with dependency on Embedded experiences

[rbaxter85, 2011-07-20 00:45:30]

http://codereview.appspot.com/4641058/diff/32001/features/src/main/javascript...
File features/src/main/javascript/features/views/viewenhancements-container.js
(right):

http://codereview.appspot.com/4641058/diff/32001/features/src/main/javascript...
features/src/main/javascript/features/views/viewenhancements-container.js:137:
navigateCallback(site, metadata);
check that navigateCallback is not null

http://codereview.appspot.com/4641058/diff/32001/features/src/main/javascript...
features/src/main/javascript/features/views/viewenhancements-container.js:160:
resultCallbackMap[site.getId()] = resultCallback;
check that resultCallback is not null before we put it in the map

http://codereview.appspot.com/4641058/diff/32001/features/src/main/javascript...
features/src/main/javascript/features/views/viewenhancements-container.js:220:
navigateCallback(site, metadata);
check that navigateCallback is not null

[plindner1, 2011-07-20 08:51:43]

Not sure what I think about this.

views are supposed to be a core-level feature, and this patch adds a dependency
to EE.  Is there a way to split off the new code into it's own feature?

[jasonchiangc, 2011-07-20 21:59:14]

move these into a new "open-views" feature based on Paul's feedback

[jasonchiangc, 2011-07-21 15:28:19]

minor update, add more error checking