Fisrt, I must thank you for this review. It's really nice you took the
necessary time to do it and so quickly. Thanks a lot !
On Sunday 16 September 2007 01:29:11 Gustavo Sverzut Barbieri wrote:
> On 9/15/07, Cedric BAIL <[EMAIL PROTECTED]> wrote:
> > During my testing of Ecore_Con, I found some little bug when you are
> > doing multiple download simultaneously :
> >
> > - The data buffer we are receiving must be copied, or it could be
> > reused by curl and we will receive garbage in the event handler. -
> > Sometime the complete event show up before we receive the last chunk of
> > data. I put all the complete event inside a list and the event are send
> > inside an ecore_idler.
> >
> > I also added some functionnality :
> > - Use ECORE_MAGIC.
> > - The status code is no longer curl internal status, but ftp or
> > http return code (I think it's more usefull than CURLE_OK). - You can now
> > add a time condition on you requested url (see HTTP code 304). - Normally
> > we must receive progress events also (I admit I didn't test if it worked
> > fine, but it should).
> >
> > I used this modified ecore_con_url to developpe a library downloading
> > file in a cache directory cleanly. I needed to include it inside Ecore as
> > I wanted to use some ecore private facility like ECORE_MAGIC. I attached
> > the patch if people are interested.
>
> + struct _Ecore_Con_Event_Url_Progress_Download
> + {
> + Ecore_Con_Url *url_con;
> + double total;
> + double now;
> + };
> +
> + struct _Ecore_Con_Event_Url_Progress_Upload
> + {
> + Ecore_Con_Url *url_con;
> + double total;
> + double now;
> + };
>
> You should made just one struct/typedef and IF required at later
> point, you can "inherit" this structure and add more fields for one
> specific case... but I don't think it will be required.
You are right.
> + char active : 1;
>
> it will cause bug with newer compilers, it should be "unsigned char"
> otherwise the unique bit will be used for the signal, thus comparisons
> would be "active == -1" not "active == 1".
Didn't know that. Will fix.
> +struct _litle_ecore_con_url_event_s
>
> typo: s/litle/little/g ?
> +static int
> +_url_complete_idler_cb(void *data)
> +{
> + _litle_ecore_con_url_event_t *lev;
> +
> + ecore_list_first_goto(_url_complete_list);
>
> Why don't you remove this "_url_complete_list" altogether and use
> "data" parameter to provide the "lev"? This way you use the idler's
> list (implicit list usage) and remove 2 globals since you don't need
> _url_complete_list or even _url_complete_idler.
Good question :) Will fix.
> + ecore_idler_del(_url_complete_idler);
> + _url_complete_idler = NULL;
> +
> + return 1;
> +}
> it's deleted automatically if you return "0", so this should work more
> nicely:
> _url_complete_idler = NULL;
> return 0;
> }
> Or just remove _url_complete_idler if you use the way I said.
Yep.
> + ecore_list_append(_url_complete_list, lev);
> +
> + if (_url_complete_idler == NULL)
> + _url_complete_idler = ecore_idler_add(_url_complete_idler_cb, NULL);
>
> using the data approach would reduce this to:
>
> ecore_idler_add(_url_complete_idler_cb, lev);
>
> much simpler :-)
>
> +EAPI void*
> +ecore_con_url_data_set(Ecore_Con_Url *url_con, const void *data)
>
> I don't like the setter returning the old value too much,
> ecore_evas_data_set() don't do that (API consistency).
I don't know where I saw this. Will fix for ecore_con and ecore_download.
> +EAPI void
> +ecore_con_url_time(Ecore_Con_Url *url_con, Ecore_Con_Url_Time
> condition, time_t tm)
>
> add "set" to the name? -> ecore_con_url_time_set()
> @@ -256,18 +422,46 @@
> size_t real_size = size * nmemb;
>
> url_con = (Ecore_Con_Url *)userp;
> - e = calloc(1, sizeof(Ecore_Con_Event_Url_Data));
> + e = calloc(1, sizeof(Ecore_Con_Event_Url_Data) + sizeof(char) *
> real_size); if (e)
> {
> e->url_con = url_con;
> - e->data = buffer;
> + e->data = (void*) ((Ecore_Con_Event_Url_Data*)(e + 1));
> e->size = real_size;
> + memcpy(e->data, buffer, real_size);
> ecore_event_add(ECORE_CON_EVENT_URL_DATA, e,
> - _ecore_con_event_url_data_free, NULL);
> + _ecore_con_event_url_free, NULL);
> }
> return real_size;
> }
>
> No, if you want to do that, modify Ecore_Con_Event_Url_Data to be:
>
> struct _Ecore_Con_Event_Url_Data {
> Ecore_Con_Url *url_con;
> int size;
> unsigned char data[1];
> };
>
> And allocation code (i don't see need for calloc()):
> e = malloc(sizeof(Ecore_Con_Event_Url_Data) + sizeof(char) *
> (real_size - 1));
> That's it, minor things but will look much better with them :-)
Thanks for the review, I will fix them all and propose a new patch.
Cedric
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
