Patch for common container: make gadgets can be rendered successfully

Working sample page on the new container, there are some issues we have to fix:


1. change the getMetadata request from  "osapi.gadgets.metadata.get" to
"osapi.gadgets.metadata" in service.js.

2. add pubsub-2 support in container.

3. remove the leading "//" when generating iframeUrl.

4. fix the security token issue if the gadget requires security-token feature.

5. need provide the container value in the sample page.

[qiaoysun, 2010-11-04 13:59:51]

On 2010/11/04 13:54:24, qiaoysun wrote:

The corresponding JIRA is : https://issues.apache.org/jira/browse/SHINDIG-1460

[zhoresh, 2010-11-04 18:22:25]

On 2010/11/04 13:59:51, qiaoysun wrote:
> On 2010/11/04 13:54:24, qiaoysun wrote:
> 
> The corresponding JIRA is : https://issues.apache.org/jira/browse/SHINDIG-1460

I have hard time to post comments inlined so the idea is:
- Use the shindig Uri and UriException class
- Use StringUtilss.isBlank to check schema is null

[Han Nguyen, 2010-11-04 20:28:46]

On 2010/11/04 18:22:25, zhoresh wrote:
> On 2010/11/04 13:59:51, qiaoysun wrote:
> > On 2010/11/04 13:54:24, qiaoysun wrote:
> > 
> > The corresponding JIRA is :
https://issues.apache.org/jira/browse/SHINDIG-1460
> 
> I have hard time to post comments inlined so the idea is:
> - Use the shindig Uri and UriException class
> - Use StringUtilss.isBlank to check schema is null

-should log error and avoid e.printStacktrace(); in the catch{} block

[mhermanto, 2010-11-04 20:40:34]

On 2010/11/04 20:28:46, Han Nguyen wrote:
> On 2010/11/04 18:22:25, zhoresh wrote:
> > On 2010/11/04 13:59:51, qiaoysun wrote:
> > > On 2010/11/04 13:54:24, qiaoysun wrote:
> > > 
> > > The corresponding JIRA is :
> https://issues.apache.org/jira/browse/SHINDIG-1460
> > 
> > I have hard time to post comments inlined so the idea is:
> > - Use the shindig Uri and UriException class
> > - Use StringUtilss.isBlank to check schema is null
> 
> -should log error and avoid e.printStacktrace(); in the catch{} block

I also had a hard time posting comments to the code review. I couldn't see the
line-by-line diff. Please try re-uploading the snapshot. Manually written, my
comments are --

=============================

gadget_holder.js
+ console.debug("create iframecontainer in gadget_holder");

Don't have debugging statements. Common container is used for user-facing prod
services. If this is still in prototype stages, you can extend and have it work
only in your container, and promote it to Shindig, accordingly. Google does this
by having a separate feature (google.container) that augments Google-specific
functionalities on top of Shindig like below:

window['google'] = window['google'] || {};
google.container = shindig.container;
google.container.GadgetHolder.prototype.OAAGetIframeHtml = function() { ... };

=============================

service.js
+    osapi.gadgets.metadata(request).execute(function(response) {
org.apache.shindig.gadgets.servlet.GadgetsHandler expect metadata.get (as
annotated in path=). I think you'll also need to change that as well.

==========================

GadgetsHandlerApi.java
+    public String getDomain();
+    public long getModuleId();
FMI, what are the usage intent for these? I don't see any implementation
consuming it, so I'd not know. :)

=======================

DefaultUriIframeManager.java

Suggesting to use a logger, can ignore upon bad host, narrow the try/catch
block, ie:

URI gadgetUri = null;
try { gadgetUri = new URI(host); } catch() { logger.log() }
if (gadgetUri != null && !StringUtils.isEmpty(gadgetUri.getScheme()) ) {
  // do your logic.
} else {
  // do old logic.
}

[Han Nguyen, 2010-11-05 08:38:14]

On 2010/11/04 20:40:34, mhermanto wrote:
> On 2010/11/04 20:28:46, Han Nguyen wrote:
> > On 2010/11/04 18:22:25, zhoresh wrote:
> > > On 2010/11/04 13:59:51, qiaoysun wrote:
> > > > On 2010/11/04 13:54:24, qiaoysun wrote:
> > > > 
> > > > The corresponding JIRA is :
> > https://issues.apache.org/jira/browse/SHINDIG-1460
> > > 
> > > I have hard time to post comments inlined so the idea is:
> > > - Use the shindig Uri and UriException class
> > > - Use StringUtilss.isBlank to check schema is null
> > 
> > -should log error and avoid e.printStacktrace(); in the catch{} block
> 
> I also had a hard time posting comments to the code review. I couldn't see the
> line-by-line diff. Please try re-uploading the snapshot. Manually written, my
> comments are --
> 
> =============================
> 
> gadget_holder.js
> + console.debug("create iframecontainer in gadget_holder");
> 
> Don't have debugging statements. Common container is used for user-facing prod
> services. If this is still in prototype stages, you can extend and have it
work
> only in your container, and promote it to Shindig, accordingly. Google does
this
> by having a separate feature (google.container) that augments Google-specific
> functionalities on top of Shindig like below:
> 
> window['google'] = window['google'] || {};
> google.container = shindig.container;
> google.container.GadgetHolder.prototype.OAAGetIframeHtml = function() { ... };
> 
> =============================
> 
> service.js
> +    osapi.gadgets.metadata(request).execute(function(response) {
> org.apache.shindig.gadgets.servlet.GadgetsHandler expect metadata.get (as
> annotated in path=). I think you'll also need to change that as well.
> 
> ==========================
> 
> GadgetsHandlerApi.java
> +    public String getDomain();
> +    public long getModuleId();
> FMI, what are the usage intent for these? I don't see any implementation
> consuming it, so I'd not know. :)
> 
> =======================
> 
> DefaultUriIframeManager.java
> 
> Suggesting to use a logger, can ignore upon bad host, narrow the try/catch
> block, ie:
> 
> URI gadgetUri = null;
> try { gadgetUri = new URI(host); } catch() { logger.log() }
> if (gadgetUri != null && !StringUtils.isEmpty(gadgetUri.getScheme()) ) {
>   // do your logic.
> } else {
>   // do old logic.
> }

Please make sure the code format is two spaces, no tab.

[qiaoysun, 2010-11-05 08:45:18]

refine the patch according to your comments

[qiaoysun, 2010-11-05 08:55:42]

refine the patch according to your comments

[qiaoysun, 2010-11-05 09:19:17]

On 2010/11/04 20:40:34, mhermanto wrote:
> On 2010/11/04 20:28:46, Han Nguyen wrote:
> > On 2010/11/04 18:22:25, zhoresh wrote:
> > > On 2010/11/04 13:59:51, qiaoysun wrote:
> > > > On 2010/11/04 13:54:24, qiaoysun wrote:
> > > > 
> > > > The corresponding JIRA is :
> > https://issues.apache.org/jira/browse/SHINDIG-1460
> > > 
> > > I have hard time to post comments inlined so the idea is:
> > > - Use the shindig Uri and UriException class
> > > - Use StringUtilss.isBlank to check schema is null
> > 
> > -should log error and avoid e.printStacktrace(); in the catch{} block
> 
> I also had a hard time posting comments to the code review. I couldn't see the
> line-by-line diff. Please try re-uploading the snapshot. Manually written, my
> comments are --
> 
> =============================
> 
> gadget_holder.js
> + console.debug("create iframecontainer in gadget_holder");
> 
> Don't have debugging statements. Common container is used for user-facing prod
> services. If this is still in prototype stages, you can extend and have it
work
> only in your container, and promote it to Shindig, accordingly. Google does
this
> by having a separate feature (google.container) that augments Google-specific
> functionalities on top of Shindig like below:
> 
> window['google'] = window['google'] || {};
> google.container = shindig.container;
> google.container.GadgetHolder.prototype.OAAGetIframeHtml = function() { ... };
> 
> =============================
> 
> service.js
> +    osapi.gadgets.metadata(request).execute(function(response) {
> org.apache.shindig.gadgets.servlet.GadgetsHandler expect metadata.get (as
> annotated in path=). I think you'll also need to change that as well.
> 
> ==========================
> 
> GadgetsHandlerApi.java
> +    public String getDomain();
> +    public long getModuleId();
> FMI, what are the usage intent for these? I don't see any implementation
> consuming it, so I'd not know. :)
> 
> =======================
> 
> DefaultUriIframeManager.java
> 
> Suggesting to use a logger, can ignore upon bad host, narrow the try/catch
> block, ie:
> 
> URI gadgetUri = null;
> try { gadgetUri = new URI(host); } catch() { logger.log() }
> if (gadgetUri != null && !StringUtils.isEmpty(gadgetUri.getScheme()) ) {
>   // do your logic.
> } else {
>   // do old logic.
> }

Thanks Michael. 
Regarding to the change in service.js, the server side didn't need change, as
the key in the rpcOperation map on server side is read from the operation name,
not from the path. So osapi.gadgets.metadat..can be mapped to metadata function
in GadgetHandler.

Regarding to the change in GadgetsHandlerApi.java, when the gadget require
security-token feature, it need generate a security token, and TokenData is an
interface used for the metadatarequest to getToken. So it is required to provide
the corresponding methods, then can be invoked and mapped to the real
implementation. The real logic is in BeanDelegator. If we didn't add those two
methods, when generating security token, the encodeToken need call
token.getDomain, but the BeanDelegator would say there isn't a method called
getDomain.

[qiaoysun, 2010-11-05 09:29:10]

On 2010/11/05 09:19:17, qiaoysun wrote:
> On 2010/11/04 20:40:34, mhermanto wrote:
> > On 2010/11/04 20:28:46, Han Nguyen wrote:
> > > On 2010/11/04 18:22:25, zhoresh wrote:
> > > > On 2010/11/04 13:59:51, qiaoysun wrote:
> > > > > On 2010/11/04 13:54:24, qiaoysun wrote:
> > > > > 
> > > > > The corresponding JIRA is :
> > > https://issues.apache.org/jira/browse/SHINDIG-1460
> > > > 
> > > > I have hard time to post comments inlined so the idea is:
> > > > - Use the shindig Uri and UriException class
> > > > - Use StringUtilss.isBlank to check schema is null
> > > 
> > > -should log error and avoid e.printStacktrace(); in the catch{} block
> > 
> > I also had a hard time posting comments to the code review. I couldn't see
the
> > line-by-line diff. Please try re-uploading the snapshot. Manually written,
my
> > comments are --
> > 
> > =============================
> > 
> > gadget_holder.js
> > + console.debug("create iframecontainer in gadget_holder");
> > 
> > Don't have debugging statements. Common container is used for user-facing
prod
> > services. If this is still in prototype stages, you can extend and have it
> work
> > only in your container, and promote it to Shindig, accordingly. Google does
> this
> > by having a separate feature (google.container) that augments
Google-specific
> > functionalities on top of Shindig like below:
> > 
> > window['google'] = window['google'] || {};
> > google.container = shindig.container;
> > google.container.GadgetHolder.prototype.OAAGetIframeHtml = function() { ...
};
> > 
> > =============================
> > 
> > service.js
> > +    osapi.gadgets.metadata(request).execute(function(response) {
> > org.apache.shindig.gadgets.servlet.GadgetsHandler expect metadata.get (as
> > annotated in path=). I think you'll also need to change that as well.
> > 
> > ==========================
> > 
> > GadgetsHandlerApi.java
> > +    public String getDomain();
> > +    public long getModuleId();
> > FMI, what are the usage intent for these? I don't see any implementation
> > consuming it, so I'd not know. :)
> > 
> > =======================
> > 
> > DefaultUriIframeManager.java
> > 
> > Suggesting to use a logger, can ignore upon bad host, narrow the try/catch
> > block, ie:
> > 
> > URI gadgetUri = null;
> > try { gadgetUri = new URI(host); } catch() { logger.log() }
> > if (gadgetUri != null && !StringUtils.isEmpty(gadgetUri.getScheme()) ) {
> >   // do your logic.
> > } else {
> >   // do old logic.
> > }
> 
> Thanks Michael. 
> Regarding to the change in service.js, the server side didn't need change, as
> the key in the rpcOperation map on server side is read from the operation
name,
> not from the path. So osapi.gadgets.metadat..can be mapped to metadata
function
> in GadgetHandler.
> 
> Regarding to the change in GadgetsHandlerApi.java, when the gadget require
> security-token feature, it need generate a security token, and TokenData is an
> interface used for the metadatarequest to getToken. So it is required to
provide
> the corresponding methods, then can be invoked and mapped to the real
> implementation. The real logic is in BeanDelegator. If we didn't add those two
> methods, when generating security token, the encodeToken need call
> token.getDomain, but the BeanDelegator would say there isn't a method called
> getDomain.

A sample page created with the common container, put three gadgets on the same
page. Following features will be tested: osapi, pubsub-2. I can't upload the
sample page to this site, you can view it at
https://issues.apache.org/jira/browse/SHINDIG-1460

[mhermanto, 2010-11-06 03:06:57]

On Fri, Nov 5, 2010 at 2:19 AM, <qiaoysun@gmail.com> wrote:

> On 2010/11/04 20:40:34, mhermanto wrote:
>
>> On 2010/11/04 20:28:46, Han Nguyen wrote:
>> > On 2010/11/04 18:22:25, zhoresh wrote:
>> > > On 2010/11/04 13:59:51, qiaoysun wrote:
>> > > > On 2010/11/04 13:54:24, qiaoysun wrote:
>> > > >
>> > > > The corresponding JIRA is :
>> > https://issues.apache.org/jira/browse/SHINDIG-1460
>> > >
>> > > I have hard time to post comments inlined so the idea is:
>> > > - Use the shindig Uri and UriException class
>> > > - Use StringUtilss.isBlank to check schema is null
>> >
>> > -should log error and avoid e.printStacktrace(); in the catch{}
>>
> block
>
>  I also had a hard time posting comments to the code review. I couldn't
>>
> see the
>
>> line-by-line diff. Please try re-uploading the snapshot. Manually
>>
> written, my
>
>> comments are --
>>
>
>  =============================
>>
>
>  gadget_holder.js
>> + console.debug("create iframecontainer in gadget_holder");
>>
>
>  Don't have debugging statements. Common container is used for
>>
> user-facing prod
>
>> services. If this is still in prototype stages, you can extend and
>>
> have it work
>
>> only in your container, and promote it to Shindig, accordingly. Google
>>
> does this
>
>> by having a separate feature (google.container) that augments
>>
> Google-specific
>
>> functionalities on top of Shindig like below:
>>
>
>  window['google'] = window['google'] || {};
>> google.container = shindig.container;
>> google.container.GadgetHolder.prototype.OAAGetIframeHtml = function()
>>
> { ... };
>
>  =============================
>>
>
>  service.js
>> +    osapi.gadgets.metadata(request).execute(function(response) {
>> org.apache.shindig.gadgets.servlet.GadgetsHandler expect metadata.get
>>
> (as
>
>> annotated in path=). I think you'll also need to change that as well.
>>
>
>  ==========================
>>
>
>  GadgetsHandlerApi.java
>> +    public String getDomain();
>> +    public long getModuleId();
>> FMI, what are the usage intent for these? I don't see any
>>
> implementation
>
>> consuming it, so I'd not know. :)
>>
>
>  =======================
>>
>
>  DefaultUriIframeManager.java
>>
>
>  Suggesting to use a logger, can ignore upon bad host, narrow the
>>
> try/catch
>
>> block, ie:
>>
>
>  URI gadgetUri = null;
>> try { gadgetUri = new URI(host); } catch() { logger.log() }
>> if (gadgetUri != null && !StringUtils.isEmpty(gadgetUri.getScheme()) )
>>
> {
>
>>   // do your logic.
>> } else {
>>   // do old logic.
>> }
>>
>
> Thanks Michael.
> Regarding to the change in service.js, the server side didn't need
> change, as the key in the rpcOperation map on server side is read from
> the operation name, not from the path. So osapi.gadgets.metadat..can be
> mapped to metadata function in GadgetHandler.
>
> Thanks for the explanation. I understand that path is not relevant here,
but osapi.gadgets.metadata need to be properly configured by the JS, ie:
service.js needs to be changed to --
  osapiServicesConfig[endPoint] = [
    'gadgets.metadata', // without .get
    'gadgets.token' // without .get
  ];

... which in turn also change GadgetsHandler.java @Operation. I've tried
this, and it wouldn't work for me otherwise.


> Regarding to the change in GadgetsHandlerApi.java, when the gadget
> require security-token feature, it need generate a security token, and
> TokenData is an interface used for the metadatarequest to getToken. So
> it is required to provide the corresponding methods, then can be invoked
> and mapped to the real implementation. The real logic is in
> BeanDelegator. If we didn't add those two methods, when generating
> security token, the encodeToken need call token.getDomain, but the
> BeanDelegator would say there isn't a method called getDomain.


Makes sense.

FYI, the diffs don't show up properly in code review because the Base URL
specified in the bug is python. It should be
http://svn.apache.org/repos/asf/shindig/trunk/.

I've created a patch (accounting your and my changes, at
http://codereview.appspot.com/2950041/). One question before I do that,
where's cssClassGadget in gadget_holder.js (I marked this with //???). PTAL,
and I can help commit it.

In terms of sample.html, I'm not sure where to put it yet, but I'd instead
try to go for an integration test that demonstrates the functionality, which
gives the same intent as sample.html, but in an automated fashion. I've
started with unit tests in src/test/javascript/features/container/*_test.js.


> http://codereview.appspot.com/2897041/
>

[qiaoysun, 2010-11-08 10:48:41]

A latest patch integrated with Michael's change

[qiaoysun, 2010-11-08 11:10:14]

A latest patch integrated with Michael's change

[qiaoysun, 2010-11-08 11:11:21]

On 2010/11/08 11:10:14, qiaoysun wrote:
> A latest patch integrated with Michael's change

Could you help to review the latest patch and help to commit? Thanks

[mhermanto, 2010-11-08 19:54:41]

Wrt removing the mapping of osapi services to endpoint cause us not to find
the endpoint. To compromise, I've un-privated the method
registerOsapiServices(), overridable by your own service to have an empty
implementation --
// import shindig service.js
shindig.container.prototype.registerOsapiServices = function() { /*empty*/
};
// run init.js

Wrt removing rpc relay RPC, I'd consult johnfargo@gmail.com. I'd not know
the RPC implications of this.

Wrt DefaultIframeUirManager, please follow up with unit tests.

Otherwise, the rest is committed -- 1032679.
Feel free to resend additional code review for refinements.


On Mon, Nov 8, 2010 at 3:11 AM, <qiaoysun@gmail.com> wrote:

> On 2010/11/08 11:10:14, qiaoysun wrote:
>
>> A latest patch integrated with Michael's change
>>
>
> Could you help to review the latest patch and help to commit? Thanks
>
>
> http://codereview.appspot.com/2897041/
>

[qiaoysun, 2010-11-09 03:09:49]

Thanks Michael!

Wrt the rpc relay, I keep the old logic. As the pubsub2 feature will call its
own rpc relay, so I move it to the else logic to make sure the pubsub2 feature
can work well in common container. 
Also please remove the line "this.el_.innerHTML = this.getIframeHtml_();" before
the if line, as the iframecontainer created by openajax hub will create the
content too. Otherwise, it will have duplicate content for gadget which requires
pubsub2.

Add a test case for DefaultIframeUriManager. Please help to commit, Thanks!


On 2010/11/08 19:54:41, mhermanto wrote:
> Wrt removing the mapping of osapi services to endpoint cause us not to find
> the endpoint. To compromise, I've un-privated the method
> registerOsapiServices(), overridable by your own service to have an empty
> implementation --
> // import shindig service.js
> shindig.container.prototype.registerOsapiServices = function() { /*empty*/
> };
> // run init.js
> 
> Wrt removing rpc relay RPC, I'd consult mailto:johnfargo@gmail.com. I'd not
know
> the RPC implications of this.
> 
> Wrt DefaultIframeUirManager, please follow up with unit tests.
> 
> Otherwise, the rest is committed -- 1032679.
> Feel free to resend additional code review for refinements.
> 
> 
> On Mon, Nov 8, 2010 at 3:11 AM, <mailto:qiaoysun@gmail.com> wrote:
> 
> > On 2010/11/08 11:10:14, qiaoysun wrote:
> >
> >> A latest patch integrated with Michael's change
> >>
> >
> > Could you help to review the latest patch and help to commit? Thanks
> >
> >
> > http://codereview.appspot.com/2897041/
> >

[qiaoysun, 2010-11-09 03:10:44]

Latest patch

[johnfargo, 2010-11-10 23:22:16]

I think I've somehow missed where pubsub-2 uses its own transport mechanism :)

http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
File features/src/main/javascript/features/container/gadget_holder.js (right):

http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
features/src/main/javascript/features/container/gadget_holder.js:187:
iframeUri.getFP('rpctoken'));
I'm confused, can I get some context on why this has been moved to the condition
where !pubsub-2?

[qiaoysun, 2010-11-11 02:10:58]

In iframe.js, when the iframecontainer is init, it calls:
OpenAjax.gadgets.rpc.setupReceiver( internalID, relay );

If we set again in gadgets_holder, the last one will rewrite the former one,
and we will get a invalid auth token error when rpc call validate auth
token.

To avoid the pubsub2 function be broken, I moved those lines to the else
condition.

Do you mean only this line: iframeUri.getFP('rpctoken'))  ? Is there any
specific meaning? I am not sure why it is needed, just moved them all. Could
you explain more about why we need this line? I am fine to move it out.

On Thu, Nov 11, 2010 at 7:22 AM, <johnfargo@gmail.com> wrote:

> I think I've somehow missed where pubsub-2 uses its own transport
> mechanism :)
>
>
>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
> File features/src/main/javascript/features/container/gadget_holder.js
> (right):
>
>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
> features/src/main/javascript/features/container/gadget_holder.js:187:
> iframeUri.getFP('rpctoken'));
> I'm confused, can I get some context on why this has been moved to the
> condition where !pubsub-2?
>
>
> http://codereview.appspot.com/2897041/
>

[johnfargo, 2010-11-11 02:17:20]

On Wed, Nov 10, 2010 at 6:10 PM, 孙巧云 <qiaoysun@gmail.com> wrote:

> In iframe.js, when the iframecontainer is init, it calls:
> OpenAjax.gadgets.rpc.setupReceiver( internalID, relay );
>
> If we set again in gadgets_holder, the last one will rewrite the former
> one, and we will get a invalid auth token error when rpc call validate auth
> token.
>
> To avoid the pubsub2 function be broken, I moved those lines to the else
> condition.
>

Would it be a reasonable solution to just have
gadgets.rpc.setupReceiver(...) remember which internalIDs have been set up
already, and ignore on subsequent calls? Seems more robust in general.


>
> Do you mean only this line: iframeUri.getFP('rpctoken'))  ? Is there any
> specific meaning? I am not sure why it is needed, just moved them all. Could
> you explain more about why we need this line? I am fine to move it out.
>

In this case no, just referring to the whole chunk. Thanks for clarifying.


>
> On Thu, Nov 11, 2010 at 7:22 AM, <johnfargo@gmail.com> wrote:
>
>> I think I've somehow missed where pubsub-2 uses its own transport
>> mechanism :)
>>
>>
>>
>>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
>> File features/src/main/javascript/features/container/gadget_holder.js
>> (right):
>>
>>
>>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
>> features/src/main/javascript/features/container/gadget_holder.js:187:
>> iframeUri.getFP('rpctoken'));
>> I'm confused, can I get some context on why this has been moved to the
>> condition where !pubsub-2?
>>
>>
>> http://codereview.appspot.com/2897041/
>>
>
>

[qiaoysun, 2010-11-11 02:26:40]

On 2010/11/11 02:17:20, johnfargo wrote:
> On Wed, Nov 10, 2010 at 6:10 PM, 孙巧云 <mailto:qiaoysun@gmail.com> wrote:
> 
> > In iframe.js, when the iframecontainer is init, it calls:
> > OpenAjax.gadgets.rpc.setupReceiver( internalID, relay );
> >
> > If we set again in gadgets_holder, the last one will rewrite the former
> > one, and we will get a invalid auth token error when rpc call validate auth
> > token.
> >
> > To avoid the pubsub2 function be broken, I moved those lines to the else
> > condition.
> >
> 
> Would it be a reasonable solution to just have
> gadgets.rpc.setupReceiver(...) remember which internalIDs have been set up
> already, and ignore on subsequent calls? Seems more robust in general.

Yes, I agree. That makes sense. Then there should be an array or map in rpc to
remember who has been set up. Are you in charge of the rpc code? Will you do the
change? Or currently just help to commit my patch to make pubsub2 can work in
common container? Thanks
> 
> 
> >
> > Do you mean only this line: iframeUri.getFP('rpctoken'))  ? Is there any
> > specific meaning? I am not sure why it is needed, just moved them all. Could
> > you explain more about why we need this line? I am fine to move it out.
> >
> 
> In this case no, just referring to the whole chunk. Thanks for clarifying.
> 
> 
> >
> > On Thu, Nov 11, 2010 at 7:22 AM, <mailto:johnfargo@gmail.com> wrote:
> >
> >> I think I've somehow missed where pubsub-2 uses its own transport
> >> mechanism :)
> >>
> >>
> >>
> >>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
> >> File features/src/main/javascript/features/container/gadget_holder.js
> >> (right):
> >>
> >>
> >>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
> >> features/src/main/javascript/features/container/gadget_holder.js:187:
> >> iframeUri.getFP('rpctoken'));
> >> I'm confused, can I get some context on why this has been moved to the
> >> condition where !pubsub-2?
> >>
> >>
> >> http://codereview.appspot.com/2897041/
> >>
> >
> >

[johnfargo, 2010-11-11 02:50:28]

I've done a bunch of rpc code maintenance, yes. I'd be happy to integrate
such change into this CL if you have the time to whip it up.

On Wed, Nov 10, 2010 at 6:26 PM, <qiaoysun@gmail.com> wrote:

> On 2010/11/11 02:17:20, johnfargo wrote:
>
>> On Wed, Nov 10, 2010 at 6:10 PM, 孙巧云 <mailto:qiaoysun@gmail.com>
>>
> wrote:
>
>  > In iframe.js, when the iframecontainer is init, it calls:
>> > OpenAjax.gadgets.rpc.setupReceiver( internalID, relay );
>> >
>> > If we set again in gadgets_holder, the last one will rewrite the
>>
> former
>
>> > one, and we will get a invalid auth token error when rpc call
>>
> validate auth
>
>> > token.
>> >
>> > To avoid the pubsub2 function be broken, I moved those lines to the
>>
> else
>
>> > condition.
>> >
>>
>
>  Would it be a reasonable solution to just have
>> gadgets.rpc.setupReceiver(...) remember which internalIDs have been
>>
> set up
>
>> already, and ignore on subsequent calls? Seems more robust in general.
>>
>
> Yes, I agree. That makes sense. Then there should be an array or map in
> rpc to remember who has been set up. Are you in charge of the rpc code?
> Will you do the change? Or currently just help to commit my patch to
> make pubsub2 can work in common container? Thanks
>
>
>
>  >
>> > Do you mean only this line: iframeUri.getFP('rpctoken'))  ? Is there
>>
> any
>
>> > specific meaning? I am not sure why it is needed, just moved them
>>
> all. Could
>
>> > you explain more about why we need this line? I am fine to move it
>>
> out.
>
>> >
>>
>
>  In this case no, just referring to the whole chunk. Thanks for
>>
> clarifying.
>
>
>  >
>> > On Thu, Nov 11, 2010 at 7:22 AM, <mailto:johnfargo@gmail.com> wrote:
>> >
>> >> I think I've somehow missed where pubsub-2 uses its own transport
>> >> mechanism :)
>> >>
>> >>
>> >>
>> >>
>>
>
>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
>
>> >> File
>>
> features/src/main/javascript/features/container/gadget_holder.js
>
>> >> (right):
>> >>
>> >>
>> >>
>>
>
>
>
http://codereview.appspot.com/2897041/diff/34001/features/src/main/javascript...
>
>> >>
>>
> features/src/main/javascript/features/container/gadget_holder.js:187:
>
>> >> iframeUri.getFP('rpctoken'));
>> >> I'm confused, can I get some context on why this has been moved to
>>
> the
>
>> >> condition where !pubsub-2?
>> >>
>> >>
>> >> http://codereview.appspot.com/2897041/
>> >>
>> >
>> >
>>
>
>
>
> http://codereview.appspot.com/2897041/
>

[qiaoysun, 2010-11-11 07:36:49]

Fix another bug: site id is not the same as iframeId,need add a util funtiocn to
extract siteid

[qiaoysun, 2010-11-11 07:39:00]

On 2010/11/11 07:36:49, qiaoysun wrote:
> Fix another bug: site id is not the same as iframeId,need add a util funtiocn
to
> extract siteid

In gadgetHolder, add a prefix to the siteId as iframeId, but when
getGadgetSite(0), it need gadgetHolder's iframeId equal siteId which will cause
error.

[qiaoysun, 2010-11-15 02:42:11]

Is there any comment to the latest patch? If not, could you help to commit the
changes? Thanks!


On 2010/11/11 07:39:00, qiaoysun wrote:
> On 2010/11/11 07:36:49, qiaoysun wrote:
> > Fix another bug: site id is not the same as iframeId,need add a util
funtiocn
> to
> > extract siteid
> 
> In gadgetHolder, add a prefix to the siteId as iframeId, but when
> getGadgetSite(0), it need gadgetHolder's iframeId equal siteId which will
cause
> error.

[mhermanto, 2010-11-15 18:20:28]

Code looks fine. Shortly/manually verifying this in our setup.

On Sun, Nov 14, 2010 at 6:42 PM, <qiaoysun@gmail.com> wrote:

> Is there any comment to the latest patch? If not, could you help to
> commit the changes? Thanks!
>
>
>
> On 2010/11/11 07:39:00, qiaoysun wrote:
>
>> On 2010/11/11 07:36:49, qiaoysun wrote:
>> > Fix another bug: site id is not the same as iframeId,need add a util
>>
> funtiocn
>
>> to
>> > extract siteid
>>
>
>  In gadgetHolder, add a prefix to the siteId as iframeId, but when
>> getGadgetSite(0), it need gadgetHolder's iframeId equal siteId which
>>
> will cause
>
>> error.
>>
>
>
>
> http://codereview.appspot.com/2897041/
>

[mhermanto, 2010-11-15 20:44:14]

The retrieving of gadget holder and site by id (and iframe id) is
convoluted. It's confusing to users and to myself (I had to spend time to
figure this out). However, I've consolidated them, and hopefully, cleared
this up. Users should *not* be using gadget_holder directly. Instead, they
should work with container and gadget_site (and not another level
gadget_holder). In your case, you should already have a handle to site from
container.newGadgetSite(), and you can call site.getActiveGadgetHolder(),
should you need the holder. I have committed the rest of your changes --
here <http://svn.apache.org/viewvc?rev=1035438&view=rev>,
diff<http://codereview.appspot.com/3120041/>.
Let me know how this goes.




On Mon, Nov 15, 2010 at 10:20 AM, Michael Hermanto <mhermanto@gmail.com>wrote:

> Code looks fine. Shortly/manually verifying this in our setup.
>
>
> On Sun, Nov 14, 2010 at 6:42 PM, <qiaoysun@gmail.com> wrote:
>
>> Is there any comment to the latest patch? If not, could you help to
>> commit the changes? Thanks!
>>
>>
>>
>> On 2010/11/11 07:39:00, qiaoysun wrote:
>>
>>> On 2010/11/11 07:36:49, qiaoysun wrote:
>>> > Fix another bug: site id is not the same as iframeId,need add a util
>>>
>> funtiocn
>>
>>> to
>>> > extract siteid
>>>
>>
>>  In gadgetHolder, add a prefix to the siteId as iframeId, but when
>>> getGadgetSite(0), it need gadgetHolder's iframeId equal siteId which
>>>
>> will cause
>>
>>> error.
>>>
>>
>>
>>
>> http://codereview.appspot.com/2897041/
>>
>
>

[qiaoysun, 2010-11-16 03:07:42]

lgtm, thank you!

On 2010/11/15 20:44:14, mhermanto wrote:
> The retrieving of gadget holder and site by id (and iframe id) is
> convoluted. It's confusing to users and to myself (I had to spend time to
> figure this out). However, I've consolidated them, and hopefully, cleared
> this up. Users should *not* be using gadget_holder directly. Instead, they
> should work with container and gadget_site (and not another level
> gadget_holder). In your case, you should already have a handle to site from
> container.newGadgetSite(), and you can call site.getActiveGadgetHolder(),
> should you need the holder. I have committed the rest of your changes --
> here <http://svn.apache.org/viewvc?rev=1035438&view=rev>,
> diff<http://codereview.appspot.com/3120041/>.
> Let me know how this goes.
> 
> 
> 
> 
> On Mon, Nov 15, 2010 at 10:20 AM, Michael Hermanto <mhermanto@gmail.com>wrote:
> 
> > Code looks fine. Shortly/manually verifying this in our setup.
> >
> >
> > On Sun, Nov 14, 2010 at 6:42 PM, <mailto:qiaoysun@gmail.com> wrote:
> >
> >> Is there any comment to the latest patch? If not, could you help to
> >> commit the changes? Thanks!
> >>
> >>
> >>
> >> On 2010/11/11 07:39:00, qiaoysun wrote:
> >>
> >>> On 2010/11/11 07:36:49, qiaoysun wrote:
> >>> > Fix another bug: site id is not the same as iframeId,need add a util
> >>>
> >> funtiocn
> >>
> >>> to
> >>> > extract siteid
> >>>
> >>
> >>  In gadgetHolder, add a prefix to the siteId as iframeId, but when
> >>> getGadgetSite(0), it need gadgetHolder's iframeId equal siteId which
> >>>
> >> will cause
> >>
> >>> error.
> >>>
> >>
> >>
> >>
> >> http://codereview.appspot.com/2897041/
> >>
> >
> >