> 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? > > Ian Clelland wrote: > This is for efficiency, and not to change usage, or to support any other > use case, right?
Not really for efficiency, but for understanding. If not static, then it holds a reference to the containing class. It's confusing if it doesn't use it. Private so that others don't try and use it. > 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. > > Ian Clelland wrote: > 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. Use android.net.Uri, not java.net.URL - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12548/#review23273 ----------------------------------------------------------- On July 17, 2013, 3:22 p.m., Ian Clelland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12548/ > ----------------------------------------------------------- > > (Updated July 17, 2013, 3:22 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 > >
