http://codereview.appspot.com/1673051/diff/1/4 File features/src/main/javascript/features/shindig.uri/feature.xml (right):
http://codereview.appspot.com/1673051/diff/1/4#newcode25 features/src/main/javascript/features/shindig.uri/feature.xml:25: <container> Allow gadgets (in a gadget context) to use this feature as a utility class. Reason not to? http://codereview.appspot.com/1673051/diff/1/5 File features/src/main/javascript/features/shindig.uri/uri.js (right): http://codereview.appspot.com/1673051/diff/1/5#newcode28 features/src/main/javascript/features/shindig.uri/uri.js:28: * essentially the minimum required for various helper routines. Lazy evalution s/evalution/evaluation/ http://codereview.appspot.com/1673051/diff/1/5#newcode38 features/src/main/javascript/features/shindig.uri/uri.js:38: * + Query and fragment do not contain their preceding ? and # chars. What happen if I call setQP() but preceding "?" doesn't work? Will it automatically add "?" for me? Same with fragment "#". http://codereview.appspot.com/1673051/diff/1/5#newcode44 features/src/main/javascript/features/shindig.uri/uri.js:44: * var other = // resolve() provided in shindig.uri.full I'm not clear with resolve(), why not just setPath() ? http://codereview.appspot.com/1673051/diff/1/5#newcode116 features/src/main/javascript/features/shindig.uri/uri.js:116: schema_ ? ":" : "", Will empty-string return true? http://codereview.appspot.com/1673051/diff/1/5#newcode117 features/src/main/javascript/features/shindig.uri/uri.js:117: authority_, I presume :xxx port numbers are accounted in authority_ as well. http://codereview.appspot.com/1673051/diff/1/5#newcode133 features/src/main/javascript/features/shindig.uri/uri.js:133: return str.join('&'); Can we easily drop the very first unnecessary &? http://codereview.appspot.com/1673051/diff/1/5#newcode137 features/src/main/javascript/features/shindig.uri/uri.js:137: if (str[0] === '?' || str[0] === '#') str = str.substr(1); Nits, consistently use either ' or " throughout this file. http://codereview.appspot.com/1673051/diff/1/5#newcode145 features/src/main/javascript/features/shindig.uri/uri.js:145: value = value.replace(/\+/g, " "); Space needs to be manually unescaped? http://codereview.appspot.com/1673051/diff/1/5#newcode168 features/src/main/javascript/features/shindig.uri/uri.js:168: return (pmap || {})[key]; Neat JS construct. http://codereview.appspot.com/1673051/diff/1/5#newcode206 features/src/main/javascript/features/shindig.uri/uri.js:206: getQP: function(key) { Don't abbreviate method names. It will blend nicer if we have getQueryParams() and getQueryString(). Similarly elsewhere. http://codereview.appspot.com/1673051/diff/1/5#newcode221 features/src/main/javascript/features/shindig.uri/uri.js:221: setQP: function() { Does this function suppose to take a JSON map of key/vars? http://codereview.appspot.com/1673051/diff/1/5#newcode225 features/src/main/javascript/features/shindig.uri/uri.js:225: setFP: function(fpmap) { fpmap not used. http://codereview.appspot.com/1673051/diff/1/5#newcode231 features/src/main/javascript/features/shindig.uri/uri.js:231: toString: toString Suggesting to add setExistingQueryOrFragmentParam(), which will set param in either query or fragment string when exists. This will be useful for common container in replacing up_*= and st=, which can be in either cases. http://codereview.appspot.com/1673051/diff/1/2 File features/src/test/javascript/features/shindig.uri/uritest.js (right): http://codereview.appspot.com/1673051/diff/1/2#newcode42 features/src/test/javascript/features/shindig.uri/uritest.js:42: assertEquals("qv2", uri.getQP("qk2")); Test get[Q|F]P() on non-existent param name. What happen if there are two params with the same name? http://codereview.appspot.com/1673051/diff/1/2#newcode59 features/src/test/javascript/features/shindig.uri/uritest.js:59: assertEquals("www.example.com", uri.getAuthority()); Test what happen when one calls getQuery() and getQP(). http://codereview.appspot.com/1673051/diff/1/2#newcode76 features/src/test/javascript/features/shindig.uri/uritest.js:76: assertEquals("http", uri.getSchema()); Similarly, test with getFragment() and getFP(). http://codereview.appspot.com/1673051/diff/1/2#newcode195 features/src/test/javascript/features/shindig.uri/uritest.js:195: assertEquals("//www.example.com/my//path", uri.toString()); nice http://codereview.appspot.com/1673051/diff/1/2#newcode216 features/src/test/javascript/features/shindig.uri/uritest.js:216: .setQuery("?one=two&three=four") I think we should not rely on user prefixing with "?", ie: should just be setQueryString("one=two&three=four"). Same with fragment. http://codereview.appspot.com/1673051/diff/1/2#newcode218 features/src/test/javascript/features/shindig.uri/uritest.js:218: assertEquals("http://www.example.com/my/path?one=two&three=four#five=six";); Where is the actual value being compared to? Same with below tests. http://codereview.appspot.com/1673051/show
