incubator-flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Om <bigosma...@gmail.com>
Subject Erik de Bruin's patch for FLEX-33149 and FLEX-33150
Date Thu, 02 Aug 2012 20:51:24 GMT
>
> 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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message