> On July 17, 2013, 2:54 p.m., Andrew Grieve wrote: > > framework/src/org/apache/cordova/Whitelist.java, line 65 > > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line65> > > > > Instead of parsing the url yourself, it might be better to use: > > > > Uri.parse(url) > > and use: > > getScheme(), getHost(), getPort(), getPath(). > > > > Probably even better would be to take in a Uri as a parameter both here > > and in isUrlWhitelisted. That way it's not re-parsed for each pattern.
I'd love to do this, and an earlier iteration of the code did just that. Unfortunately, the Java URL parser won't recognize arbitrary schemes, unless there is a ProtocolHandler registered for them. I could certainly parse it just the once, and pass around a custom URI-like structure, for efficiency, if you think that's warranted. > On July 17, 2013, 2:54 p.m., Andrew Grieve wrote: > > framework/src/org/apache/cordova/Whitelist.java, line 13 > > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line13> > > > > make private & static? This is for efficiency, and not to change usage, or to support any other use case, right? > On July 17, 2013, 2:54 p.m., Andrew Grieve wrote: > > framework/src/org/apache/cordova/Whitelist.java, line 184 > > <https://reviews.apache.org/r/12548/diff/1/?file=321768#file321768line184> > > > > Doesn't look like this cache is very good. > > > > - It's unbounded > > - It's caching URLs without stripping query params > > - It's failing to cache negative hits. > > > > I don't think performance is an issue here, so probably a good idea to > > just delete the cache. Good catch. Deleted. It was there simply because the original implementation was caching results. - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12548/#review23273 ----------------------------------------------------------- On July 15, 2013, 2:57 p.m., Ian Clelland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12548/ > ----------------------------------------------------------- > > (Updated July 15, 2013, 2:57 p.m.) > > > Review request for cordova. > > > Repository: cordova-android-git > > > Description > ------- > > Replaced the current android whitelist implementation with a new one which > conforms to CB-4093. > > This is also more robust against bad urls in the config file, and handles > several cases which the previous implementation did not (eg. > http://www.apache.org:passw...@evil.com/ would be accepted previously) > > > Diffs > ----- > > framework/src/org/apache/cordova/Config.java 6e0c147 > framework/src/org/apache/cordova/Whitelist.java 736e5a7 > > Diff: https://reviews.apache.org/r/12548/diff/ > > > Testing > ------- > > This passes all of the recently-added whitelist tests in cordova-mobile-spec. > > > Thanks, > > Ian Clelland > >
