On Mon, 17 Sep 2007 10:29:54 +0200 Cedric BAIL <[EMAIL PROTECTED]> babbled:
OK - i'm now catching up on my email/patches/whatever....
so got an updated patch? gustavo's comments are valid and good ones :)
> 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
>
--
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler) [EMAIL PROTECTED]
裸好多
Tokyo, Japan (東京 日本)
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
