Hi Michael: Thanks for the review!
Somehow the code review tool created two of the same code review for this, so I'm closing this one in favor of the other (which oddly has a much higher ID): http://codereview.appspot.com/1875044/show Comments posted here, but the latest patch to 1875044 has all updates. 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> Nope, adding gadget section. On 2010/07/21 06:51:07, mhermanto wrote:
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 On 2010/07/21 06:51:07, mhermanto wrote:
s/evalution/evaluation/
Done. 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. ? and # will be added in uri.toString(); this comment indicates that ? and # are NOT returned in the getQuery() and getFragment() calls, since in general use they're far and away more often split off anyway. Deleting comment since it's confusing: it's not a limitation so much as an API choice. On 2010/07/21 06:51:07, mhermanto wrote:
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 Because resolve() handles relative paths: shindig.uri("http://www.other.com/foo/one/two/three";).resolve("bar") --> http://www.other.com/foo/one/two/bar Updating comment. On 2010/07/21 06:51:07, mhermanto wrote:
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_ ? ":" : "", No, but this logic is not especially clear. Fixing next up. On 2010/07/21 06:51:07, mhermanto wrote:
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_, Correct. Authority includes all of that. If port is desired to be stripped out, it needs to be done separately. On 2010/07/21 06:51:07, mhermanto wrote:
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('&'); Yes. Fixing in latest update. On 2010/07/21 06:51:07, mhermanto wrote:
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); I had designs on using ' for single chars, "" for longer strings, but I'm just going 100% on ". On 2010/07/21 06:51:07, mhermanto wrote:
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, " "); I believe I saw this in the unesc method a while back, but to be honest this particular use of it was copy-and-pasted from other code. This should be revisited. On 2010/07/21 06:51:07, mhermanto wrote:
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]; :) Sadly, it'll now be gone, w/ changes to maintain param order. On 2010/07/21 06:51:07, mhermanto wrote:
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) { The point of doing so is to keep the generated JS small. Unlike internal method names, exported ones cannot be obfuscated. I suppose it's just a few bytes (here), but when used all over the place gets noticeable. I'm open to change on this one. On 2010/07/21 06:51:07, mhermanto wrote:
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() { Making this more explicit in the next version. I'll add more JsDoc in a follow-up. Through the magic of JS, it accepts one of two constructs: 1. setQP("name", "value") --> overrides or sets query param value 2. setQP({ key: "val", other: "again", foo: "bar" }) --> overrides or sets multiple values at once. In the interest of API brevity, there is no separate removeParam method. Callers may set param value to undefined in such case. On 2010/07/21 06:51:07, mhermanto wrote:
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) { On 2010/07/21 06:51:07, mhermanto wrote:
fpmap not used.
Done. http://codereview.appspot.com/1673051/diff/1/5#newcode231 features/src/main/javascript/features/shindig.uri/uri.js:231: toString: toString Interesting, so catching the either-or case? Seems useful. I'll add this in util.js -- which you'll notice for now isn't really tested. I'm still working out the design but my thinking is to include the most minimal useful Uri parser/builder class possible here, while adding various helper methods (resolve, setExistingParam, fullyQualify, hasSameOrigin, and others) in util.js, which may correspond to a new feature (shindig.uri.util) inheriting from the base. The idea is to avoid adding the kitchen sink to the base class -- just enough primitives that others can be added easily. On 2010/07/21 06:51:07, mhermanto wrote:
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/show
