Code review comment for lp:~dooferlad/linaro-android-frontend/ui_download_links_update

On Mon, 23 Jan 2012 16:42:47 -0000
James Tunnicliffe <email address hidden> wrote:

> Hi Paul, thanks for the comments.
>
> > 1. Are you sure that making 3 synchronous AJAX requests doesn't
> > hurt page download/interactive performance?
>
> It hasn't caused any obvious performance problems for me, but if one
> or more of the requests took a long time it could be a problem. Will
> investigate how easy it is to move them to asynchronous requests.

Well, my idea of that would be done was to have chain of async result
handlers - i.e. result handler of 1st request would check if it got
what's needed and if so, display it, otherwise queue up another request
whose handler would do similarly, etc.

I don't pledge for sync processing to be changed immediately, just want
to make sure it is known as point of potential problem and we're ready
to resolve it.

>
> > 2. create_manifest.py script apparently doesn't belong here, but
> > rather should be part of linaro-android-build-tools and be called
> > from build-android script at the appropriate place.
>
> Ah, thanks. Will move it.
>
> > 3. Did you give extra thought what manifest.txt may contain? Just
> > filenames? Maybe file size? What about MD5SUMS file we already
> > create for subset of files (actually, one of the reason why is it
> > created for subset and not all was because  that would require
> > faithful duplication of artifact filename patterns - both in
> > Jenkins and in this script, same problem would exist for
> > create_manifest.py too). (I don't have any specific suggestion
> > regarding what manifest.txt should include, just wanted to make
> > sure this is though-out).
>
> The only thing the web UI will use is file names and that is the only
> reason to add the file. If we extend its use later we could update the
> UI to cope.

Makes sense.

>
> > 4. Ah, last one, naming. Maybe "MANIFEST" is better, it's kind of
> > common practice (that depends on contents format of course).
>
> Not a problem - changing a file name is easy enough.

Ok, so I assume there will be more commits for this merge request
before approving it.

Thanks,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog