You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Mike May <th...@gmail.com> on 2012/05/10 18:46:50 UTC
Review Request: init.js blindly assumes that the JS servlet feature URL is
the last script tag defined
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/
-----------------------------------------------------------
Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
Summary
-------
init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
This addresses bug SHINDIG-1774.
https://issues.apache.org/jira/browse/SHINDIG-1774
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
Diff: https://reviews.apache.org/r/5087/diff
Testing
-------
All tests pass.
Local testing done.
Thanks,
Mike
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Stanton Sievers <si...@gmail.com>.
> On 2012-05-10 17:00:08, Stanton Sievers wrote:
> > Please take a look at https://issues.apache.org/jira/browse/SHINDIG-1533. It tried to address this issue earlier by allowing one to define a window global to specify the ID of the script tag.
> >
>
> Mike May wrote:
> That functionality remains. This code adds the logic to try and find the correct script if the window global id is not defined. I can see an implementer not realizing the id needed to be define when they include Javascript below the JSservlet URL.
Without being able to read the config, this approach is going to be limited, I think. The JsServlet endpoint is configurable in container.js via "gadgets.uri.js.path". If a deployer of Shindig changes that configuration, this code no longer works. If we're fine with this being a "best effort" approach and then the __CONTAINER_SCRIPT_ID being the "catch all", then I think this approach is OK.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7777
-----------------------------------------------------------
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Mike May <th...@gmail.com>.
> On 2012-05-10 17:00:08, Stanton Sievers wrote:
> > Please take a look at https://issues.apache.org/jira/browse/SHINDIG-1533. It tried to address this issue earlier by allowing one to define a window global to specify the ID of the script tag.
> >
>
> Mike May wrote:
> That functionality remains. This code adds the logic to try and find the correct script if the window global id is not defined. I can see an implementer not realizing the id needed to be define when they include Javascript below the JSservlet URL.
>
> Stanton Sievers wrote:
> Without being able to read the config, this approach is going to be limited, I think. The JsServlet endpoint is configurable in container.js via "gadgets.uri.js.path". If a deployer of Shindig changes that configuration, this code no longer works. If we're fine with this being a "best effort" approach and then the __CONTAINER_SCRIPT_ID being the "catch all", then I think this approach is OK.
This is a good point. If we stick with the "best effort" point of view we can do the following:
- update container.js @ gadgets.uri.js.path to contain a little documentation for end users stating that if they change this value they can use __CONTAINER_SCRIPT_ID as a fail safe.
- update container.js @ gadgets.uri.js.path to contain a little documentation that will point out they can update the RegEx if they are compiling on their own.
The first update/documentation piece is what is really needed here. Any objections?
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7777
-----------------------------------------------------------
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Mike May <th...@gmail.com>.
> On 2012-05-10 17:00:08, Stanton Sievers wrote:
> > Please take a look at https://issues.apache.org/jira/browse/SHINDIG-1533. It tried to address this issue earlier by allowing one to define a window global to specify the ID of the script tag.
> >
That functionality remains. This code adds the logic to try and find the correct script if the window global id is not defined. I can see an implementer not realizing the id needed to be define when they include Javascript below the JSservlet URL.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7777
-----------------------------------------------------------
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7777
-----------------------------------------------------------
Please take a look at https://issues.apache.org/jira/browse/SHINDIG-1533. It tried to address this issue earlier by allowing one to define a window global to specify the ID of the script tag.
- Stanton
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Ryan Baxter <rb...@gmail.com>.
> On 2012-05-10 17:17:36, Ryan Baxter wrote:
> > LGTM
Committed revision 1336874. Please close the review.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7788
-----------------------------------------------------------
On 2012-05-10 17:52:31, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 17:52:31)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1336812
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336812
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7788
-----------------------------------------------------------
Ship it!
LGTM
- Ryan
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7790
-----------------------------------------------------------
Ship it!
LGTM.
- Stanton
On 2012-05-10 17:52:31, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 17:52:31)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1336812
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336812
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Mike May <th...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/
-----------------------------------------------------------
(Updated 2012-05-10 17:52:31.455712)
Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
Changes
-------
Added documentation describing actions required if changing gadgets.uri.js.path
Summary
-------
init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
This addresses bug SHINDIG-1774.
https://issues.apache.org/jira/browse/SHINDIG-1774
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1336812
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336812
Diff: https://reviews.apache.org/r/5087/diff
Testing
-------
All tests pass.
Local testing done.
Thanks,
Mike
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7782
-----------------------------------------------------------
Ship it!
+1
- Henry
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Henry Saputra <hs...@apache.org>.
> On 2012-05-10 16:51:58, Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js, line 36
> > <https://reviews.apache.org/r/5087/diff/2/?file=108385#file108385line36>
> >
> > I was wondering if I can push the pattern /gadgets/js to shindig.properties or container.js for easy configuration change?
>
> Stanton Sievers wrote:
> Henry, I don't think we have access to config at this point. This routine is the first to be called once init.js has loaded, so none of the config has be initialized yet.
Yeah your are right, this code is executed very early.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7776
-----------------------------------------------------------
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Stanton Sievers <si...@gmail.com>.
> On 2012-05-10 16:51:58, Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js, line 36
> > <https://reviews.apache.org/r/5087/diff/2/?file=108385#file108385line36>
> >
> > I was wondering if I can push the pattern /gadgets/js to shindig.properties or container.js for easy configuration change?
Henry, I don't think we have access to config at this point. This routine is the first to be called once init.js has loaded, so none of the config has be initialized yet.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7776
-----------------------------------------------------------
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: init.js blindly assumes that the JS servlet feature URL
is the last script tag defined
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5087/#review7776
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js
<https://reviews.apache.org/r/5087/#comment17084>
I was wondering if I can push the pattern /gadgets/js to shindig.properties or container.js for easy configuration change?
- Henry
On 2012-05-10 16:46:50, Mike May wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5087/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 16:46:50)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Summary
> -------
>
> init.js assumes the feature URL for the JS servlet is the last script tag when this will not always be the case.
>
>
> This addresses bug SHINDIG-1774.
> https://issues.apache.org/jira/browse/SHINDIG-1774
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/init.js 1336780
>
> Diff: https://reviews.apache.org/r/5087/diff
>
>
> Testing
> -------
>
> All tests pass.
>
> Local testing done.
>
>
> Thanks,
>
> Mike
>
>