Hi guys, If there's no other objections nor concerns, I'll commit this patch by the ...
12 years, 12 months ago
(2011-05-05 14:48:34 UTC)
#3
Hi guys,
If there's no other objections nor concerns, I'll commit this patch by the end
of the week. Thanks!
On 2011/05/02 21:12:23, leegd28 wrote:
> update the patch with correct svn base and comments from Henry
http://codereview.appspot.com/4430047/diff/3001/config/container.js File config/container.js (right): http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92 config/container.js:92: "shindig.signing.global-callback-url" : "http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback", Not sure why you need to ...
12 years, 12 months ago
(2011-05-05 15:32:30 UTC)
#4
thanks for the review. please check the comments below. http://codereview.appspot.com/4430047/diff/3001/config/container.js File config/container.js (right): http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92 config/container.js:92: ...
12 years, 12 months ago
(2011-05-05 18:24:36 UTC)
#6
thanks for the review. please check the comments below.
http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):
http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
yes, I moved it from shindig.properties into container.js.
so Context Root can be resolved here.
I also noticed a duplicate with "gadgets.uri.oauth.callbackTemplate", do you
have suggestions to consolidate these two? thanks.
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
the value is only available in the jsp. does common container has a place to
pass in this value? I set it to window.__CONTEXT_ROOT for now to make it
consistent with other global variables in init.js.
The other alternative sounds good. How about this...
1. add a configuration in container.js
"container" : {
"relayPath": "${CONTEXT_ROOT}/gadgets/files/container/rpc_relay.html",
"contextroot":"${CONTEXT_ROOT}"
}
2. then it's returned and resolved as part of "/gadgets/ifr?..."
then gadgets.config.init will initialize window.__CONTEXT_ROOT in
gadget-holder.js
function init(config) {
if (config.container){
var rpath = config["container"]["relayPath"];
osapi.container.GadgetHolder.prototype.relayPath_ = rpath;
window.__CONTEXT_ROOT = config["container"]["contextroot"];
}
};
http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
File
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java
(right):
http://codereview.appspot.com/4430047/diff/3001/java/common/src/main/java/org...
java/common/src/main/java/org/apache/shindig/common/servlet/GuiceServletContextListener.java:67:
binder.bindConstant().annotatedWith(Names.named("shindig.contextroot")).to(contextRoot);
As it is part of ServletContext, there's no need to set up from
System.properties?
http://codereview.appspot.com/4430047/diff/3001/config/container.js File config/container.js (right): http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92 config/container.js:92: "shindig.signing.global-callback-url" : "http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback", I am not too familiar with ...
12 years, 12 months ago
(2011-05-06 06:13:08 UTC)
#7
http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):
http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I am not too familiar with the param but you might want to check comment in the
GadgetOAuthCallbackGenerator class.
AFAIK, the properties of Shindig are shared across all container, so thats why
shindig.signing.global-callback-url is put in the shindig.properties file.
On 2011/05/05 18:24:36, leegd28 wrote:
> yes, I moved it from shindig.properties into container.js.
> so Context Root can be resolved here.
> I also noticed a duplicate with "gadgets.uri.oauth.callbackTemplate", do you
> have suggestions to consolidate these two? thanks.
>
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
I like this approach better
On 2011/05/05 18:24:36, leegd28 wrote:
>
> the value is only available in the jsp. does common container has a place to
> pass in this value? I set it to window.__CONTEXT_ROOT for now to make it
> consistent with other global variables in init.js.
>
> The other alternative sounds good. How about this...
> 1. add a configuration in container.js
> "container" : {
> "relayPath": "${CONTEXT_ROOT}/gadgets/files/container/rpc_relay.html",
> "contextroot":"${CONTEXT_ROOT}"
> }
> 2. then it's returned and resolved as part of "/gadgets/ifr?..."
> then gadgets.config.init will initialize window.__CONTEXT_ROOT in
> gadget-holder.js
>
> function init(config) {
> if (config.container){
> var rpath = config["container"]["relayPath"];
> osapi.container.GadgetHolder.prototype.relayPath_ = rpath;
> window.__CONTEXT_ROOT = config["container"]["contextroot"];
> }
> };
http://codereview.appspot.com/4430047/diff/3001/config/container.js File config/container.js (right): http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92 config/container.js:92: "shindig.signing.global-callback-url" : "http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback", I did a search and only ...
12 years, 12 months ago
(2011-05-06 17:56:43 UTC)
#8
http://codereview.appspot.com/4430047/diff/3001/config/container.js
File config/container.js (right):
http://codereview.appspot.com/4430047/diff/3001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I did a search and only found "shindig.signing.global-callback-url" is used by
OAuthModule. Insteadof getting it by using Guice injection ( from
shindig.properties) it's now picked up from containerConfig ( as it's provided
by default container). Since there's no other use of that property, we're ok
here?
if there's some usecase that may break, Would you please give me some details?
like how to see the failure via testing... thanks.
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examples/commoncontainer/index.jsp File content/samplecontainer/examples/commoncontainer/index.jsp (right): http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examples/commoncontainer/index.jsp#newcode23 content/samplecontainer/examples/commoncontainer/index.jsp:23: window.__CONTEXT_ROOT = '<%=request.getContextPath()%>'; Sorry, I need to revise the ...
12 years, 11 months ago
(2011-05-09 15:10:26 UTC)
#9
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):
http://codereview.appspot.com/4430047/diff/3001/content/samplecontainer/examp...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
Sorry, I need to revise the approach after trying it.
It's too late to pass context root from a <script> tag loading.
context needs to be set as part of href of <script> tags anyway.
I think the code that requires context root on client side can be divided into 2
parts. The set is sample container implementation (index.jsp,
assember.js,viewcontroller.js) and the other set is the core common container
implementation (like init.js).
For the usage in sample container, I think it's flexible to the sample
container, it can choose any variable to pass the value around.
For core common container implementation, I added a default a value. If
window.__CONTEXT_ROOT is not set by sample container bootstrap (like index.jsp),
it will use "" as default.
this will be added to initializeConfig() as part of init.js
if (!window.__CONTEXT_ROOT) window.__CONTEXT_ROOT = "";
Please let me know what you think. Thanks for your help, it has been there for a
couple of weeks. appreciate your help to help move it forward.
LGTM with proposed change to keep shindig.signing.global-callback-url in shindig.properties file. - Henry http://codereview.appspot.com/4430047/diff/22001/config/container.js File config/container.js ...
12 years, 11 months ago
(2011-05-10 06:56:39 UTC)
#11
LGTM with proposed change to keep shindig.signing.global-callback-url in
shindig.properties file.
- Henry
http://codereview.appspot.com/4430047/diff/22001/config/container.js
File config/container.js (right):
http://codereview.appspot.com/4430047/diff/22001/config/container.js#newcode92
config/container.js:92: "shindig.signing.global-callback-url" :
"http://${SERVER_HOST}:${SERVER_PORT}/${CONTEXT_ROOT}gadgets/oauthcallback",
I dont think we need to move this to container.js file. This property could
should be in shindig.properties because its not container specific but service
provider specific.
Some comments about keeping with html than jsp. - Henry http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/examples/commoncontainer/index.jsp File content/samplecontainer/examples/commoncontainer/index.jsp (right): http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/examples/commoncontainer/index.jsp#newcode23 ...
12 years, 11 months ago
(2011-05-12 23:25:27 UTC)
#15
Some comments about keeping with html than jsp.
- Henry
http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/exam...
File content/samplecontainer/examples/commoncontainer/index.jsp (right):
http://codereview.appspot.com/4430047/diff/32001/content/samplecontainer/exam...
content/samplecontainer/examples/commoncontainer/index.jsp:23:
window.__CONTEXT_ROOT = '<%=request.getContextPath()%>';
I dont think we should change the html with jsp file. The container usually have
different context than the gadget server.
I would think instead of using window.__CONTEXT_ROOT probably inject the context
root when constructing the osapi.container.Container and provide function to
change it.
The demo page could have field to add context root that could be set before ask
the common container code to render gadget.
Thoughts?
I agree container and gadget apis could have different context. However jsp would be the ...
12 years, 11 months ago
(2011-05-13 14:49:05 UTC)
#16
I agree container and gadget apis could have different context. However jsp
would be the better solution in this case.
When container page is loaded, common container css and js are loaded too.use
jsp it would be flexible to resolve a different context path for the url to load
common container css and js.
use inputbox could be problematic... that means common container and css
couldn't be loaded upon page loading, seperate loading can only be done via
xhr(quite a change from the current sample container, need to rewrite ) or an
additional full page load with jsp support.
yeah, context root can be passed into osapi.container.Container. However that
doesn't help.
Context root path for APIs(js features) needs to be resolved in init.js.
There're couple of other global variables defined there eg
window.__CONTAINER_URI,window._API_URI.
Thinking about rename it to window._API_CONTEXT_ROOT, so it will be inline with
other global variable. what do you think?
One suggestion is to keep sample index.html and modify the conf/container.js core.io section to: // ...
12 years, 11 months ago
(2011-05-13 23:46:26 UTC)
#17
One suggestion is to keep sample index.html and modify the conf/container.js
core.io section to:
// Only configuration for required features will be used.
// See individual feature.xml files for configuration details.
"gadgets.features" : {
"core.io" : {
// Note: /proxy is an open proxy. Be careful how you expose this!
// Note: Here // is replaced with the current protocol http/https
"proxyUrl" :
"//%host%/gadgets/proxy?container=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
"jsonProxyUrl" : "//%host%/gadgets/makeRequest"
},
Then modify the init.js to remove the code that setss core.io config:
Index: features/src/main/javascript/features/container/init.js
===================================================================
--- features/src/main/javascript/features/container/init.js (revision 1102897)
+++ features/src/main/javascript/features/container/init.js (working copy)
@@ -28,13 +28,6 @@
gadgets.config.init({
'rpc': {
'parentRelayUrl': ''
- },
- 'core.io': {
- 'jsonProxyUrl': 'http://%host%/gadgets/makeRequest',
- 'proxyUrl': 'http://%host%/gadgets/proxy' +
- '?refresh=%refresh%' +
- '&container=%container%%rewriteMime%' +
- '&gadget=%gadget%/%rawurl%'
}
});
}
With new JS serving framework, the config will be pushed to client for container
by ConfigInjectionProcessor so I think common container init.js does not have to
set the config for core.io.
Including Michael Hermanto for his comment.
- Henry
Forgot to copy the ${CONTEXT_ROOT}: "gadgets.features" : { "core.io" : { ... "proxyUrl" : "//%host%${CONTEXT_ROOT}/gadgets/proxycontainer=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%", ...
12 years, 11 months ago
(2011-05-14 19:33:16 UTC)
#18
Forgot to copy the ${CONTEXT_ROOT}:
"gadgets.features" : {
"core.io" : {
...
"proxyUrl" :
"//%host%${CONTEXT_ROOT}/gadgets/proxycontainer=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
...
- Henry
On 2011/05/13 23:46:26, henry.saputra wrote:
> One suggestion is to keep sample index.html and modify the conf/container.js
> core.io section to:
>
> // Only configuration for required features will be used.
> // See individual feature.xml files for configuration details.
> "gadgets.features" : {
> "core.io" : {
> // Note: /proxy is an open proxy. Be careful how you expose this!
> // Note: Here // is replaced with the current protocol http/https
> "proxyUrl" :
>
"//%host%/gadgets/proxy?container=%container%%rewriteMime%&refresh=%refresh%&url=%url%%rewriteMime%&gadget=%gadget%%rawurl%",
> "jsonProxyUrl" : "//%host%/gadgets/makeRequest"
> },
>
> Then modify the init.js to remove the code that setss core.io config:
>
> Index: features/src/main/javascript/features/container/init.js
> ===================================================================
> --- features/src/main/javascript/features/container/init.js (revision 1102897)
> +++ features/src/main/javascript/features/container/init.js (working copy)
> @@ -28,13 +28,6 @@
> gadgets.config.init({
> 'rpc': {
> 'parentRelayUrl': ''
> - },
> - 'core.io': {
> - 'jsonProxyUrl': 'http://%host%/gadgets/makeRequest',
> - 'proxyUrl': 'http://%host%/gadgets/proxy' +
> - '?refresh=%refresh%' +
> - '&container=%container%%rewriteMime%' +
> - '&gadget=%gadget%/%rawurl%'
> }
> });
> }
>
> With new JS serving framework, the config will be pushed to client for
container
> by ConfigInjectionProcessor so I think common container init.js does not have
to
> set the config for core.io.
>
> Including Michael Hermanto for his comment.
>
> - Henry
Thanks. I investigated a little here today. You're right, gadget.config.init() in init.js is not necessary. ...
12 years, 11 months ago
(2011-05-16 21:35:23 UTC)
#19
Thanks. I investigated a little here today. You're right, gadget.config.init()
in init.js is not necessary. Even it's invoked, it's always overwritten by
what's defined in container.js since that's injected by
ConfigInjectionProcessor.
Possible to remove gadgets.config.init from init.js? -- I know this probably
better to addressed as a seperate issue.
I will update the patch to remove the changes in init.js.
Also don't need to define window.__Context_Root on client side anymore.
Regarding jsp vs html...it will be helpful if you could please explain a little
why html is preferred.
If it's static html, the src(to load container) needs to be dynamically
generated and set. We need to guarantee the order here.
Using script element with jsp would have guaranteed order.
<script type="text/javascript"
src="/gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
<script type="text/javascript" src="./assembler.js"></script>
I think example files should not be in jsp just because it shows that you ...
12 years, 11 months ago
(2011-05-16 23:32:09 UTC)
#20
I think example files should not be in jsp just because it shows that you dont
need Java stack to actually build container with common container.
And yes the order of the script tag need to be guaranteed.
- Henry
On 2011/05/16 21:35:23, Li Xu wrote:
> Thanks. I investigated a little here today. You're right, gadget.config.init()
> in init.js is not necessary. Even it's invoked, it's always overwritten by
> what's defined in container.js since that's injected by
> ConfigInjectionProcessor.
> Possible to remove gadgets.config.init from init.js? -- I know this probably
> better to addressed as a seperate issue.
>
> I will update the patch to remove the changes in init.js.
>
> Also don't need to define window.__Context_Root on client side anymore.
>
> Regarding jsp vs html...it will be helpful if you could please explain a
little
> why html is preferred.
> If it's static html, the src(to load container) needs to be dynamically
> generated and set. We need to guarantee the order here.
> Using script element with jsp would have guaranteed order.
>
>
>
> <script type="text/javascript"
>
src="/gadgets/js/container:rpc:pubsub-2.js?c=1&debug=1&container=default"></script>
> <script type="text/javascript" src="./assembler.js"></script>
Issue 4430047: Enable shindig to run on non-ROOT context path
Created 13 years ago by Li Xu
Modified 12 years, 11 months ago
Reviewers: Han Nguyen, dev-remailer_shindig.apache.org, Paul Lindner, mhermanto, henry.saputra, woodstae_gmail.com, mgmarum
Base URL: http://svn.apache.org/repos/asf/shindig/trunk
Comments: 22