> > My improved contribution. It supersedes my earlier patches, which I have > removed to avoid confusion. This version should resolve the issues > FLEX-33149 and FLEX-33150 and includes Alex's suggestion to use a GeoIP > service combines with the Apache mirrors list as a fallback while we wait > for a project-native CGI solution. >
Erik, The patch looks extremely good. I applied the patch and tested it and everything works as promised! The app was able to automatically select the correct mirror, download the flex sdk, verify the signature and continue with installation process as before. I have a few issues/questions that I think will improve the quality of the patch. 1. I see your comment about moving URL_CONFIG_XML constant from ViewResourceConstants to the main class. Resource constants dont necessarily have to be localizable strings. It can be urls, icons, sound files etc. I dont think such constants should be in the main class itself. IMHO it makes it inconsistent. A good compromise would be place this constant in the ViewResourceConstants class, but NOT load it as a locale string using iResourceManager . Would that be acceptable? 2. You have added a few log mesasges. Can you please convert them into locale strings for the en_US locale at least? This will make it easier for other locale contributors to provide corresponding translations. 3. When verifying the Apache Flex SDK MD5 signature, a message is being logged for every progress update which causes a ton of log messages . You probably can just log when the verification starts and when it ends. 4. A log message about the result of the verification of the Apache Flex SDK would be good. Especially true when the md5 check fails and we abort, we need to let the user know what just happened. 5. Not a huge deal, but if possible, in the sequence of logs, can we make sure that the Version number log is always the first one? 6. For some reason, the 'temp' folder is not getting deleted after the install. This used to work. Can you please debug and see if you introduced something that causes this issue? 7. In the fetchApacheMirrorFromCGI() method, the result handler to _internetUtil.fetch is specified to be the same method - fetchApacheMirrorFromCGI() This method is a bit too confusing. I think it will be better if you had a separate event handler where you process the result and extract the url. 8. Can you move all the mirror fetching logic into one class and provide a common api? The internal logic can be abstracted from the main class. I hope you dont mind fixing the patch based on these comments and reattaching it when you get a chance. I can then commit this patch to svn. Thanks again for the high quality contribution! Om
