http://codereview.appspot.com/50047/diff/1/38 File features/src/main/javascript/features/osapi/makerequest.js (right):
http://codereview.appspot.com/50047/diff/1/38#newcode23 Line 23: osapi.makeRequest = function() { why not switch to osapi.http at this point? We have the handler ready to go. http://codereview.appspot.com/50047/diff/1/40 File features/src/main/javascript/features/osapi/osapi.js (right): http://codereview.appspot.com/50047/diff/1/40#newcode23 Line 23: /** clean up the indent of the above two http://codereview.appspot.com/50047/diff/1/40#newcode38 Line 38: (function() { we're already inside a closure: why not move this function to just inside the top closure, instead of creating it repeatedly? e.g. function createMethod(serviceName, methodName, endpointName) { } ... which will also save the variable aliasing to work around closure behavior. http://codereview.appspot.com/50047/diff/1/9 File java/common/src/main/java/org/apache/shindig/protocol/RpcServiceFetcher.java (right): http://codereview.appspot.com/50047/diff/1/9#newcode62 Line 62: private ContainerConfig containerConfig; final http://codereview.appspot.com/50047/diff/1/9#newcode62 Line 62: private ContainerConfig containerConfig; final http://codereview.appspot.com/50047/diff/1/9#newcode80 Line 80: public Map<String, Set<String>> getServicesForContainer(String container, String host) { in practice, we don't need or want this to be host-relative. For example, locked domains puts every gadget on a different domain, but we don't want to recompute services for each such gadget. Store this once per-container, not once per-container-and-host. http://codereview.appspot.com/50047/diff/1/9#newcode85 Line 85: Map<String, Set<String>> endpointServices = Maps.newHashMapWithExpectedSize(endpoints.size()); this should be a Multimap<String, String>, not a Map<String, Set<String>> http://codereview.appspot.com/50047/diff/1/9#newcode87 Line 87: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) { this can be a simple for loop. http://codereview.appspot.com/50047/diff/1/9#newcode94 Line 94: @SuppressWarnings("unchecked") move the @SuppressWarnings to the lines that need it instead of the whole function http://codereview.appspot.com/50047/diff/1/9#newcode98 Line 98: List<String> endpoints = (List<String>) properties.get(OSAPI_BASE_ENDPOINTS); take advantage of container config EL support: containerConfig.get("${Cur['gadgets.features'].endPoints}") http://codereview.appspot.com/50047/diff/1/9#newcode100 Line 100: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) { this could be a simple for (String endpoint : endpoints) loop http://codereview.appspot.com/50047/diff/1/9#newcode110 Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD); use Uri, not URL. (Avoid java.net.URL like the plague, it is a mind-bogglingly bad class.) http://codereview.appspot.com/50047/diff/1/9#newcode110 Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD); spaces around operators http://codereview.appspot.com/50047/diff/1/9#newcode111 Line 111: HttpClient client = new HttpClient(); any particular reason not to just inject HttpFetcher (or RequestPipeline)? http://codereview.appspot.com/50047/diff/1/9#newcode129 Line 129: private void getServicesFromJsonResponse(Set<String> services, String content) make this return a Set<String>, instead of taking one. And should use Collections.unmodifiableSet() or ImmutableSet. http://codereview.appspot.com/50047/diff/1/8 File java/common/src/main/java/org/apache/shindig/protocol/RpcServiceLookup.java (right): http://codereview.appspot.com/50047/diff/1/8#newcode58 Line 58: private Map<String, Map<String, Set<String>>> containerServices; all final http://codereview.appspot.com/50047/diff/1/8#newcode79 Line 79: public synchronized Map<String, Set<String>> getServicesFor(String container, String host) { instead of synchronizing the methods, use concurrent hash maps (e.g. MapMaker from Google Collections). Also worth considering expiration of values, for two reasons: - If there's an error loading the services, we'd like to be able to retry in the near future - Supports dynamically configured or updated servers (if a separate server handles social requests, it can be updated with support for a new handler, and we can detect that automatically) http://codereview.appspot.com/50047/diff/1/8#newcode79 Line 79: public synchronized Map<String, Set<String>> getServicesFor(String container, String host) { Multimap would be a good choice here http://codereview.appspot.com/50047/diff/1/8#newcode81 Line 81: if (container == null || container.length() == 0) { seems like this should be Preconditions.checkArgument() http://codereview.appspot.com/50047/diff/1/8#newcode86 Line 86: if (foundServices == null && !retrievedContainers.contains(container)) { when would it be null, but retrieved? http://codereview.appspot.com/50047/diff/1/8#newcode87 Line 87: foundServices = fetcher.getServicesForContainer(container, host); we're fetching services for a container+host, but then caching purely on container... http://codereview.appspot.com/50047/diff/1/8#newcode89 Line 89: retrievedContainers.add(container); instead of maintaining a list of retrievedContainers (just to distinguish null), swap in an ImmutableMap.of() for the stored value http://codereview.appspot.com/50047/diff/1/8#newcode91 Line 91: return (foundServices != null) ? foundServices : ImmutableMap.<String, Set<String>>of(); would look simpler to say: if (foundServices == null { foundServices = ImmutableMap.of(); } return foundServices; ... since then you get generic types inferred http://codereview.appspot.com/50047/diff/1/8#newcode99 Line 99: public synchronized void setServicesFor(String container, Map<String, Set<String>> services) { why is this public? http://codereview.appspot.com/50047/diff/1/11 File java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java (right): http://codereview.appspot.com/50047/diff/1/11#newcode390 Line 390: Collection<Feature> values = prefs.getFeatures().values(); values -> features http://codereview.appspot.com/50047/diff/1/11#newcode398 Line 398: private void addOsapiSystemListMethodsConfig(String container, Map<String, Object> config, String host) {
100 columns
http://codereview.appspot.com/50047/diff/1/11#newcode405 Line 405: private Map<String, Map<String, Map<String, String>>> mapEndpoints( wow, Map of Maps of Maps. Ouch. Code needs some comments explaining what structure it's trying to build up. FWIW, I think Map<String, Map<String, List<String>>> would be clearer - filling in empty strings for the methods-to-be seems unnecessarily complex. (Or at that point, Map<String, Multimap<String, String>>. Muuuch simpler.) http://codereview.appspot.com/50047/diff/1/11#newcode409 Line 409: for (Iterator<String> iterator = endpoints.keySet().iterator(); iterator.hasNext();) { for (String endpoint : endpoints) http://codereview.appspot.com/50047/diff/1/11#newcode412 Line 412: if (serviceMethods == null || serviceMethods.size() == 0) { serviceMethods should never be null. Also isEmpty() instead of size==0 http://codereview.appspot.com/50047/diff/1/11#newcode424 Line 424: for (Iterator<String> methodIter = methods.iterator(); methodIter.hasNext();) { for (String method : methods) http://codereview.appspot.com/50047
