Inline feature to render gadget in inline

We originally posted inlining work directly into the existing container shindig-container/server side components... 

After reviewing some of these changes and learning more about the features, we've stepped back and refactored those changes as a feature on the common container. I add this new patch, which is based on new common container and its new patch: https://issues.apache.org/jira/browse/SHINDIG-1460 

After apply the patch, access http://localhost:8080/container/helloworld_common3.html you would see the inline gadget and iframe gadget being rendered on same page, though they are helloworld gadgets. 

Jira url: https://issues.apache.org/jira/browse/SHINDIG-1402

[Jasvir, 2010-11-09 22:03:13]

Hi Kai,

Some preliminary comments.  Just a quick request - the base url for this CL is
set to http://svn.python.org/projects/python/trunk/.  This breaks the viewing of
inline diffs in codereview and makes it hard to review.  Can you change it to
the correct on ( https://svn.apache.org/repos/asf/shindig/trunk ?)

jas

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
File extras/src/main/javascript/features-extras/inline/inline_container.js
(right):

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:27: }else
if(!requiresPubSub2 && !requiresInline) {
trivial nit: space between } and else.  Here and elsewhere.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:35:
//TODO: requires pubsub-2 and inline
Is the problem specific to pubsub2.  I am not sure I understand how anything
that uses gadgets.rpc would work with inlining - can you help me understand how
it would work?  Adding a test gadget that uses gadgets.rpc or rpc based api
(like dynamic-height) would help a lot.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:69: for (
var key in divParams) {
Please use if (divParams.hasOwnProperty(key))... inside this for..in loop here
and elsewhere.

See http://www.jslint.com/lint.html#forin

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:125:
shindig.container.GadgetHolder.prototype.injectScripts =
function(strippedContent) {
Does this not break deferred script semantics?  A deferred script sees the
complete DOM whereas other scripts see only the DOM built up to that
point.<script defer="defer">document.getElementsByTagName("*")</script>
fr'instance will evaluate incorrectly.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:214:
shindig.container.GadgetHolder.prototype.stripStyle = function(strippedContent,
gadgetHtml) {
Regex-based stripping is fragile and broken - please use a parser.  Your regexp
for example doesn't find scripts or styles with spaces before the close tags
closing angle brackets (<script>alert('oh hai');</script >).  It also ignores
attributes that can cause execution like on* handlers and those that take urls
but allow javascript urls.

I recommend using html-sanitizer.js - its already used to implement
gadgets.util.sanitzeHtml.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
File extras/src/main/javascript/features-extras/inline/resource_sharing.js
(right):

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:7:
shindig.resource.uriparser = function (s) {
Should a uri parser sit here in inlining?  It seems it would be useful elsewhere
as well.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:22:
strict:
/^(?:([^:\/?#]+):)?(?:\/\/((?:(([^:@]*)(?::([^:@]*))?)?@)?([^:\/?#]*)(?::(\d*))?))?((((?:[^?#\/]*\/)*)([^?#]*))(?:\?([^#]*))?(?:#(.*))?)/,
It would be helpful for reviewing the regex to comment what parts of the regex
are matching which portions of the url and what the difference between strict
and loose are suposed to be.  Strict looks to me like the one defined by RFC3986
but with credentials broken out - I can see how that would be useful.

[Kai Feng Zhang, 2010-11-11 08:08:38]

Thank you jasvir, I just replied your comments and attached a new patch for
review.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
File extras/src/main/javascript/features-extras/inline/inline_container.js
(right):

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:27: }else
if(!requiresPubSub2 && !requiresInline) {
On 2010/11/09 22:03:13, jasvir wrote:
> trivial nit: space between } and else.  Here and elsewhere.

Done.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:35:
//TODO: requires pubsub-2 and inline
Firstly, this inline feature patch is based on the patch on new common
container, which already went into code base. See:
http://codereview.appspot.com/2897041/. So code here is from that patch. If
gadget requires "inline" and "pubsub-2", we need to provide
InlineHubContainer(from OAH) specially for gadget rendering. So this is specific
to pubsub-2. But for now we have not provide it, it's here just a placeholder
thing. We will look into it soon.

For inlining, gadgets.rpc as a base rpc mechanism works fine, but some other
features may rely on gadgets.rpc, such as setTitle or dynamic-height. For inline
,we must provide correct "from" and "target" params(etc) when invoking
gadgets.rpc. We are still investigating on gadgets.rpc on new common container,
to see how to support gadgets.rpc well.

On 2010/11/09 22:03:13, jasvir wrote:
> Is the problem specific to pubsub2.  I am not sure I understand how anything
> that uses gadgets.rpc would work with inlining - can you help me understand
how
> it would work?  Adding a test gadget that uses gadgets.rpc or rpc based api
> (like dynamic-height) would help a lot.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:69: for (
var key in divParams) {
On 2010/11/09 22:03:13, jasvir wrote:
> Please use if (divParams.hasOwnProperty(key))... inside this for..in loop here
> and elsewhere.
> 
> See http://www.jslint.com/lint.html#forin

Done.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:125:
shindig.container.GadgetHolder.prototype.injectScripts =
function(strippedContent) {
Here we did not consider deferred script. Does rendering gadget in Shindig
support deferred script?

I found a link: http://www.websiteoptimization.com/speed/tweak/defer/, deferred
script has different and weird execution order issue on different browser.

Here what we did is to inject and run all scripts(external and inline) at last,
after DOM parsed and CSS applied.

On 2010/11/09 22:03:13, jasvir wrote:
> Does this not break deferred script semantics?  A deferred script sees the
> complete DOM whereas other scripts see only the DOM built up to that
> point.<script defer="defer">document.getElementsByTagName("*")</script>
> fr'instance will evaluate incorrectly.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/inline_container.js:214:
shindig.container.GadgetHolder.prototype.stripStyle = function(strippedContent,
gadgetHtml) {
1. I tweak the regexp to handle the whitespace which could be in sciprt or style
tag.
2. the on* hanldlers are part of html DOM, and will be injected into page DOM
first, then css and last script execution. So until on* event happened, handlers
will not be called, right?
3. I looked at html-sanitizer.js, seems it's only want to strip a safe html out,
removing style and script. But here we need to get all of them: html, css, and
script, then inject each into page DOM orderly. So I still need to strip style
and script out.

On 2010/11/09 22:03:13, jasvir wrote:
> Regex-based stripping is fragile and broken - please use a parser.  Your
regexp
> for example doesn't find scripts or styles with spaces before the close tags
> closing angle brackets (<script>alert('oh hai');</script >).  It also ignores
> attributes that can cause execution like on* handlers and those that take urls
> but allow javascript urls.
> 
> I recommend using html-sanitizer.js - its already used to implement
> gadgets.util.sanitzeHtml.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
File extras/src/main/javascript/features-extras/inline/resource_sharing.js
(right):

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:7:
shindig.resource.uriparser = function (s) {
Just remove this uriparser, in new patch we use shindig.uri to parse url.

On 2010/11/09 22:03:13, jasvir wrote:
> Should a uri parser sit here in inlining?  It seems it would be useful
elsewhere
> as well.

http://codereview.appspot.com/2927041/diff/1/extras/src/main/javascript/featu...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:22:
strict:
/^(?:([^:\/?#]+):)?(?:\/\/((?:(([^:@]*)(?::([^:@]*))?)?@)?([^:\/?#]*)(?::(\d*))?))?((((?:[^?#\/]*\/)*)([^?#]*))(?:\?([^#]*))?(?:#(.*))?)/,
Just remove this uriparser, in new patch we use shindig.uri to parse url.

On 2010/11/09 22:03:13, jasvir wrote:
> It would be helpful for reviewing the regex to comment what parts of the regex
> are matching which portions of the url and what the difference between strict
> and loose are suposed to be.  Strict looks to me like the one defined by
RFC3986
> but with credentials broken out - I can see how that would be useful.

[Kai Feng Zhang, 2010-11-11 08:11:03]

A new patch. (1) based on new common container (2) support user prefs for inline
gadget

[Kai Feng Zhang, 2010-11-15 13:29:33]

hi jasvir,

Thank you very much for code review.

Would you please have a time to review my new patch? Is there anything I can do
to speed the code review?

Thanks a lot.

On 2010/11/11 08:11:03, Kai Feng Zhang wrote:
> A new patch. (1) based on new common container (2) support user prefs for
inline
> gadget

[Jasvir, 2010-11-15 20:40:08]

+fargo

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
File extras/src/main/javascript/features-extras/inline/inline_container.js
(right):

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:58: if
(this.securityToken_) {
I don't understand the consequences of messing with st.  Adding fargo.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:173: var
scriptsStripped = stylesStripped.replace(regexp, function() {
Thank you for fixing the regex to include spaces.  Unfortunately this is still
insufficient - your regular expression for example matches the appearance of 
"<script>" with a Turkish "i" which will turn an non-executable block into a
executable one.


Perhaps I am failing to understand the various strip and inject calls are
intended to achieve.  Stripping a script block and moving it to the head changes
the semantics of the script - even if you ignore deferred scripts.  A script
that appears at a particular point in an html file sees the DOM up to that point
- moving scripts into the head will break those scripts.  Trivial example is:

<div id="foo"></div>
<script>alert(document.getElementById('foo'));</script>

If you move the script into head, the alert will now show null.

The reason I am objecting is I'd really like to avoid a proliferation of
different semantics depending on how a gadget is included on the page.  We don't
want to place on the gadget developer the burden on testing their gadget inlined
and iframed - besides the gadget spec defines the semantics of the Content tag
to be html - we should break those semantics only for good reason.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:179: var
externalSrc = ss[1].replace(/&amp;/ig, "&");
Ah ok so you're expecting to see unescaped html characters in the url.  Should
you also be unescaping other html entities?

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
File extras/src/main/javascript/features-extras/inline/resource_sharing.js
(right):

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:37: var
params = q.toString().split(/[&;]/);
Why split on semicolon?  Should this code use shindig.uri instead?

[Jasvir Nagra, 2010-11-15 20:47:58]

On Mon, Nov 15, 2010 at 5:29 AM, <kf.zhang@gmail.com> wrote:

> hi jasvir,
>
> Thank you very much for code review.
>
> Would you please have a time to review my new patch? Is there anything I
> can do to speed the code review?
>

Sorry I've been slow on the turnaround - I think I've not understood some of
the reasons a few of the changes.  I'll try to be quicker although more test
cases (or just a chat) would certainly help me out in understanding what
semantics you want to preserve.


>
> Thanks a lot.
>
>
> On 2010/11/11 08:11:03, Kai Feng Zhang wrote:
>
>> A new patch. (1) based on new common container (2) support user prefs
>>
> for inline
>
>> gadget
>>
>
>
>
> http://codereview.appspot.com/2927041/
>

[johnfargo, 2010-11-17 23:14:01]

Hi Kai Feng:

A few comments on the container piece below -- most of which get to the
underlying security model of this approach overall. My concern comes from
extensive experience at Google with unmodified-inlined content, which was
implemented a long time ago and which I spent a year and change removing,
specifically for security reasons, though there are other reasons as well (API
bindings and the like).

So I think I might be missing a little of underlying context on the rationale
for inlining overall. Jas and I are working on an approach that should allow
most gadgets to safely be rendered inline by cajoling them -- a much-preferred
alternative that we think holds the promise of actually increasing performance.
Could I ask you to indulge us in a little bit of exposition?

Cheers,
John

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
File extras/src/main/javascript/features-extras/inline/inline_container.js
(right):

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:33: } else
if (!this.hasFeature_(gadgetInfo, 'pubsub-2') && this.hasFeature_(gadgetInfo,
'inline')) {
Back when iGoogle did inlining, it also originally OK'd any arbitrary gadget
that asked to be inlined. Unfortunately, more was needed, since not all gadgets
behaved properly (the worst could manipulate page state such that the whole
thing got borked).

The steps done were:
1. Added a whitelist of acceptable inlined gadgets.
2. Realized that one compromised (popular) gadget compromised the user's entire
data - ie. the security of the container becomes the min(security of all
gadgets). Copied all inlined gadgets to a separate data store controlled
internally.
3. Removed inlining altogether in favor of IFRAMEs and (hopefully, finally
soon!) cajoled gadgets providing security guarantees by design.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:63:
uri.setQP('mid', String(this.siteId_));
FYI this is no longer really used, and mostly serves to bust caching (as would
any query param).

mid was originally intended to disambiguate gadgets on the page, but now
container code and gadgets.rpc can do so via its own means.

Past that, the main concept of module_id was invented to facilitate inlining by
letting gadget devs write constructs like
var foo__MODULE_ID__ = "this varname is different for two of the same gadget on
the page";

however, this mechanism only helps two identical gadgets both inlined on the
page disambiguate their symbols from one another. Symbols introduced by the
containing page could still conflict.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:110: }
You should be able to use gadgets.io.makeNonProxiedRequest rather than
reimplementing this

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:113:
xhr.open("GET", this.getIframeUrl_(), true);
Is this intended only to work on browsers that support cross-domain XHR, or is
another implicit requirement that the gadget rendering API be installed on the
same domain as the container?

If the latter, that would suggest that other security primitives, like
locked-domain, are disabled for this as well, allowing any arbitrary gadget to
be executed on the container's domain. I'd prefer to see gadget content returned
instead via the metadata APIs (GadgetsHandler et al)

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:135: var
scriptsStripped = this.stripScriptAndHtml(strippedContent, stylesStripped);
scriptsStripped is unused

[Kai Feng Zhang, 2010-11-19 13:43:07]

The basic idea of this inline implementation is, when shindig server render spec
xml into html and return to client side. Client side will strip html DOM part,
style part and script part, then inject each part into current document orderly.


As described above, we implement inline as a shindig feature now based on above
idea. And that's the only choice we have to support inline gadget for now. We
did look into Caja support in Shindig, but the result is not good enough, if we
have multiple inline gadgets on page with same spec xml, they still have
namespace(name/id conflicts) issues. What would you please suggest to do, to
support inline gadget besides Caja?

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
File extras/src/main/javascript/features-extras/inline/inline_container.js
(right):

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:33: } else
if (!this.hasFeature_(gadgetInfo, 'pubsub-2') && this.hasFeature_(gadgetInfo,
'inline')) {
Thanks for the suggestion. Can I know how it's going for your caja inline work?
Is there any plan when you guys will contribute back to Shindig?


On 2010/11/17 23:14:01, johnfargo wrote:
> Back when iGoogle did inlining, it also originally OK'd any arbitrary gadget
> that asked to be inlined. Unfortunately, more was needed, since not all
gadgets
> behaved properly (the worst could manipulate page state such that the whole
> thing got borked).
> 
> The steps done were:
> 1. Added a whitelist of acceptable inlined gadgets.
> 2. Realized that one compromised (popular) gadget compromised the user's
entire
> data - ie. the security of the container becomes the min(security of all
> gadgets). Copied all inlined gadgets to a separate data store controlled
> internally.
> 3. Removed inlining altogether in favor of IFRAMEs and (hopefully, finally
> soon!) cajoled gadgets providing security guarantees by design.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:58: if
(this.securityToken_) {
This code/method is in common container, gadget_holder.js.

I just copied it here and overwrite it but adding new lines:

window.globals_inline[this.siteId_] = window.globals_inline[this.siteId_] || {};
window.globals_inline[this.siteId_].gadgetInfo = this.gadgetInfo_;
window.globals_inline[this.siteId_].ifrUrl = uri.toString();

To keep each inline gadget's iframe url, then it's able to get it back with
gadgets.util.getUrlParameter() method.

On 2010/11/15 20:40:08, jasvir wrote:
> I don't understand the consequences of messing with st.  Adding fargo.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:63:
uri.setQP('mid', String(this.siteId_));
Much clear, thanks.

I just copied method from gadget_holder.js to here for overwriting and added
some specific codes for inline rendering, see above comment.

On 2010/11/17 23:14:01, johnfargo wrote:
> FYI this is no longer really used, and mostly serves to bust caching (as would
> any query param).
> 
> mid was originally intended to disambiguate gadgets on the page, but now
> container code and gadgets.rpc can do so via its own means.
> 
> Past that, the main concept of module_id was invented to facilitate inlining
by
> letting gadget devs write constructs like
> var foo__MODULE_ID__ = "this varname is different for two of the same gadget
on
> the page";
> 
> however, this mechanism only helps two identical gadgets both inlined on the
> page disambiguate their symbols from one another. Symbols introduced by the
> containing page could still conflict.

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:110: }
Thank you, I will change to use gadgets.io API for this request.

On 2010/11/17 23:14:01, johnfargo wrote:
> You should be able to use gadgets.io.makeNonProxiedRequest rather than
> reimplementing this

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:113:
xhr.open("GET", this.getIframeUrl_(), true);
I will try to make some server codes, to allow gadget content returned from
metadata request directly. That will save us an extra request which is ifr url
sent to gadget rendering servlet.

On 2010/11/17 23:14:01, johnfargo wrote:
> Is this intended only to work on browsers that support cross-domain XHR, or is
> another implicit requirement that the gadget rendering API be installed on the
> same domain as the container?
> 
> If the latter, that would suggest that other security primitives, like
> locked-domain, are disabled for this as well, allowing any arbitrary gadget to
> be executed on the container's domain. I'd prefer to see gadget content
returned
> instead via the metadata APIs (GadgetsHandler et al)

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:135: var
scriptsStripped = this.stripScriptAndHtml(strippedContent, stylesStripped);
Remove it soon, thanks.

On 2010/11/17 23:14:01, johnfargo wrote:
> scriptsStripped is unused

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:173: var
scriptsStripped = stylesStripped.replace(regexp, function() {
On 2010/11/15 20:40:08, jasvir wrote:
> Thank you for fixing the regex to include spaces.  Unfortunately this is still
> insufficient - your regular expression for example matches the appearance of 
> "<script>" with a Turkish "i" which will turn an non-executable block into a
> executable one.
> 
I am just not sure how much possible it would happen for such a string
<script>xxx</script> with Turkish "i" would be rendered back by Shindig server,
and then be turned into an executable script.

> 
> Perhaps I am failing to understand the various strip and inject calls are
> intended to achieve.  Stripping a script block and moving it to the head
changes
> the semantics of the script - even if you ignore deferred scripts.  A script
> that appears at a particular point in an html file sees the DOM up to that
point
> - moving scripts into the head will break those scripts.  Trivial example is:
> 
> <div id="foo"></div>
> <script>alert(document.getElementById('foo'));</script>
> 
> If you move the script into head, the alert will now show null.
> 
The basic idea of this inline implementation is, when shindig server render spec
xml into html and return to client side. Client side will strip html DOM part,
style part and script part, then inject each part into current document orderly.
So for your above example, div#foo will be inject into document first and then
script be put into head and executed. Would you please give some suggestion what
we should do?

> The reason I am objecting is I'd really like to avoid a proliferation of
> different semantics depending on how a gadget is included on the page.  We
don't
> want to place on the gadget developer the burden on testing their gadget
inlined
> and iframed - besides the gadget spec defines the semantics of the Content tag
> to be html - we should break those semantics only for good reason.

As described above, we implement inline as a shindig feature now based on above
idea. And that's the only choice we have to support inline gadget for now. We
did look into Caja support in Shindig, but the result is not good enough, if we
have multiple inline gadgets on page with same spec xml, they still have
namespace(name/id conflicts) issues.

What would you please suggest to do, to support inline gadget besides Caja?

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/inline_container.js:179: var
externalSrc = ss[1].replace(/&amp;/ig, "&");
Good point thanks. We now only see unescaped chars in the script url, not in
other html entities. We would definitely handle them if have when returning from
server side.

On 2010/11/15 20:40:08, jasvir wrote:
> Ah ok so you're expecting to see unescaped html characters in the url.  Should
> you also be unescaping other html entities?

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
File extras/src/main/javascript/features-extras/inline/resource_sharing.js
(right):

http://codereview.appspot.com/2927041/diff/16001/extras/src/main/javascript/f...
extras/src/main/javascript/features-extras/inline/resource_sharing.js:37: var
params = q.toString().split(/[&;]/);
Semicolon is useless here, this line is to split params out with '&' or ';' as
the separator.

I will remove this semicolon. Thanks.

On 2010/11/15 20:40:08, jasvir wrote:
> Why split on semicolon?  Should this code use shindig.uri instead?