March 2009 - Dillo-dev - lists.mailman3.com

Re: css parsing numbers

by Johannes.Hofmann＠gmx.de

On Mon, Mar 02, 2009 at 01:06:52PM -0300, Jorge Arellano Cid wrote: > On Sun, Mar 01, 2009 at 12:31:18PM +0100, Hofmann Johannes wrote: > > On Sat, Feb 28, 2009 at 12:22:32AM +0100, Hofmann Johannes wrote: > > > On Fri, Feb 27, 2009 at 07:06:27PM +0100, Hofmann Johannes wrote: > > > > On Fri, Feb 27, 2009 at 03:29:55PM +0000, corvid wrote: > > > > > > On Fri, Feb 27, 2009 at 02:26:03PM +0000, corvid wrote: > > > > > > > > Committed. I hoped to be able to make that code a bit shorter, but > > > > > > > > could not find a reasonable solution... > > > > > > > > > > > > > > Yeah. In any case, it's just going to get worse/rearranged again > > > > > > > for negative numbers... > > > > > > > > > > > > Exactly. I started my simplification attempts, added negative number > > > > > > support - and blew it all up :) > > > > > > I'm really considering a flex based scanner now. What do you think? > > > > > > > > > > I only touched flex very briefly for a class at school long ago, > > > > > and I don't remember anything from the experience, but the > > > > > idea interests me since it has to be less trouble than trying > > > > > to do it by hand... > > > > > > > > > > > > > > > This turned out to be easier than expected. Attached patch adds a flex > > > based scanner for CSS data. > > > It's not optimized or polished but seems to work here. > > > > To make reviewing easier I created a repo for the flex experiment: > > http://freehg.org/u/dillo/flex/ > > I think the use of flex makes the code simpler and more > > maintainable. For some reason rendering is a bit different, e.g. on > > wikipedia.org maybe because negative numbers are supported. > > It would be nice if people could test it regarding the performance > > impact. > > > > What do you think. Is it worth to add the flex dependency? > > I'm a bit worried about the flex dependency. > > It's being a long time since I saw/used flex/bison/yacc & > friends. AFAIR, there was a myriad of slightly incompatible > flavours. > > It may be safer to generate a C-source parser with the tool, > and to include it as a source file. I'm currently checking re2c (http://re2c.org/) as a flex alternative and it looks pretty cool. It does not need a lib and produces pretty clean C code. So we might ship the generated code in the release tarballs. I'll post a prototype when I'm ready. Cheers, Johannes

March 2, 2009

minor leak with CustShrinkTabPager

by jcid＠dillo.org

On Mon, Mar 02, 2009 at 09:02:18AM +0100, Hofmann Johannes wrote: > Hi, > > thanks to Jeremy's valgrind log at > http://starurchin.org/dillo/valgrind.html > I noticed a minor leak with CustShrinkTabPager(). > > The TabGroup::pager() method internally clones the passed pager, so > it must be deleted afterwards, e.g. like this: > [...] Committed. -- Cheers Jorge.-

March 2, 2009

Re: css parsing numbers

by Johannes.Hofmann＠gmx.de

On Mon, Mar 02, 2009 at 01:06:52PM -0300, Jorge Arellano Cid wrote: > On Sun, Mar 01, 2009 at 12:31:18PM +0100, Hofmann Johannes wrote: > > On Sat, Feb 28, 2009 at 12:22:32AM +0100, Hofmann Johannes wrote: > > > On Fri, Feb 27, 2009 at 07:06:27PM +0100, Hofmann Johannes wrote: > > > > On Fri, Feb 27, 2009 at 03:29:55PM +0000, corvid wrote: > > > > > > On Fri, Feb 27, 2009 at 02:26:03PM +0000, corvid wrote: > > > > > > > > Committed. I hoped to be able to make that code a bit shorter, but > > > > > > > > could not find a reasonable solution... > > > > > > > > > > > > > > Yeah. In any case, it's just going to get worse/rearranged again > > > > > > > for negative numbers... > > > > > > > > > > > > Exactly. I started my simplification attempts, added negative number > > > > > > support - and blew it all up :) > > > > > > I'm really considering a flex based scanner now. What do you think? > > > > > > > > > > I only touched flex very briefly for a class at school long ago, > > > > > and I don't remember anything from the experience, but the > > > > > idea interests me since it has to be less trouble than trying > > > > > to do it by hand... > > > > > > > > > > > > > > > This turned out to be easier than expected. Attached patch adds a flex > > > based scanner for CSS data. > > > It's not optimized or polished but seems to work here. > > > > To make reviewing easier I created a repo for the flex experiment: > > http://freehg.org/u/dillo/flex/ > > I think the use of flex makes the code simpler and more > > maintainable. For some reason rendering is a bit different, e.g. on > > wikipedia.org maybe because negative numbers are supported. > > It would be nice if people could test it regarding the performance > > impact. > > > > What do you think. Is it worth to add the flex dependency? > > I'm a bit worried about the flex dependency. > > It's being a long time since I saw/used flex/bison/yacc & > friends. AFAIR, there was a myriad of slightly incompatible > flavours. I think what the patch uses is pretty standard and automake takes care of the detection and correct parameters. > > It may be safer to generate a C-source parser with the tool, > and to include it as a source file. The problem is that we also need to (statically) link libfl.a, so we actually need lex/flex installed on the build system. > > AFAIU the patch feeds a CSS grammar to libflex, and it acts as > an interpreter that parses. In it's current form only the scanner/tokenizer is replaced by a flex-generated one. The parsing is still done using the hand-written code. > > You should consider the performance, the complexity (against > tunning our current parser), and if the dependency can be > avoided. > Until recently Css_next_token() was small and simple enough so that it was certainly not worth the additional dependency. However even adding support for floats that start with '.' increased it's complexity. Next is negative numbers, urls and maybe more. Does anyone know whether there are still compatibility/integration issues with flex on exotic platforms? Cheers, Johannes

March 2, 2009

[bgreer101@earthlink.net: Dillo2 working great with Crux-PPC!]

by jcid＠dillo.org

-- Cheers Jorge.-

March 2, 2009

Re: css parsing numbers

by jcid＠dillo.org

On Sun, Mar 01, 2009 at 12:31:18PM +0100, Hofmann Johannes wrote: > On Sat, Feb 28, 2009 at 12:22:32AM +0100, Hofmann Johannes wrote: > > On Fri, Feb 27, 2009 at 07:06:27PM +0100, Hofmann Johannes wrote: > > > On Fri, Feb 27, 2009 at 03:29:55PM +0000, corvid wrote: > > > > > On Fri, Feb 27, 2009 at 02:26:03PM +0000, corvid wrote: > > > > > > > Committed. I hoped to be able to make that code a bit shorter, but > > > > > > > could not find a reasonable solution... > > > > > > > > > > > > Yeah. In any case, it's just going to get worse/rearranged again > > > > > > for negative numbers... > > > > > > > > > > Exactly. I started my simplification attempts, added negative number > > > > > support - and blew it all up :) > > > > > I'm really considering a flex based scanner now. What do you think? > > > > > > > > I only touched flex very briefly for a class at school long ago, > > > > and I don't remember anything from the experience, but the > > > > idea interests me since it has to be less trouble than trying > > > > to do it by hand... > > > > > > > > > > > This turned out to be easier than expected. Attached patch adds a flex > > based scanner for CSS data. > > It's not optimized or polished but seems to work here. > > To make reviewing easier I created a repo for the flex experiment: > http://freehg.org/u/dillo/flex/ > I think the use of flex makes the code simpler and more > maintainable. For some reason rendering is a bit different, e.g. on > wikipedia.org maybe because negative numbers are supported. > It would be nice if people could test it regarding the performance > impact. > > What do you think. Is it worth to add the flex dependency? I'm a bit worried about the flex dependency. It's being a long time since I saw/used flex/bison/yacc & friends. AFAIR, there was a myriad of slightly incompatible flavours. It may be safer to generate a C-source parser with the tool, and to include it as a source file. AFAIU the patch feeds a CSS grammar to libflex, and it acts as an interpreter that parses. You should consider the performance, the complexity (against tunning our current parser), and if the dependency can be avoided. -- Cheers Jorge.-

March 2, 2009

[PATCH] Replace the FSF Address in GPL v3 files [try 2]

by jcid＠dillo.org

On Wed, Feb 25, 2009 at 05:39:13PM +0100, Detlef Riekenberg wrote: > The license notice in GPL v3 source files use an outdated FSF Address. > This version use the suggestions from Place (corvid) Good! Committed. > Things to do: > dw/fltkcomplexbutton.hh: GNU Library General Public License v2 (or > later) > dw/fltkcomplexbutton.cc: GNU Library General Public License v3 (or > later) > Such licenses does not exits. > LGPL is GNU Lesser General Public License > > Some Files mention GPL v3, but the license Note is incomplete. > Some Files are missing a copyright / license Note OK, if you can submit a patch that addresses those problems, please do so. -- Cheers Jorge.-

March 2, 2009

[patch]: remove the workaround for FLTK bug #2084.

by jcid＠dillo.org

On Fri, Feb 27, 2009 at 08:29:08PM +0000, Jeremy Henty wrote: > > The dillo source includes a workaround for FLTK bug #2084[1] which was > fixed in revision 6507. We currently recommend[2] revision 6525 or > later. There's no need to include this workaround any more, so let's > remove it. Committed. -- Cheers Jorge.-

March 2, 2009

minor leak with CustShrinkTabPager

by Johannes.Hofmann＠gmx.de

Hi, thanks to Jeremy's valgrind log at http://starurchin.org/dillo/valgrind.html I noticed a minor leak with CustShrinkTabPager(). The TabGroup::pager() method internally clones the passed pager, so it must be deleted afterwards, e.g. like this: diff -r 9b0b8ae9b06a src/uicmd.cc --- a/src/uicmd.ccSun Mar 01 12:34:00 2009 +0100 +++ b/src/uicmd.ccMon Mar 02 08:57:02 2009 +0100 @@ -225,7 +225,9 @@ public: CustTabGroup (int x, int y, int ww, int wh, const char *lbl=0) : TabGroup(x,y,ww,wh,lbl) { - this->pager(new CustShrinkTabPager()); + CustShrinkTabPager *cp = new CustShrinkTabPager(); + this->pager(cp); + delete cp; toolTip = new Tooltip; tooltipEnabled = false; buttonPushed = false; Alternatively we could keep one copy around in a static member. Cheers, Johannes

March 2, 2009

[patch]: fix a memory leak in a_Klist_free()

by jcid＠dillo.org

On Sat, Feb 28, 2009 at 06:54:36PM +0000, Jeremy Henty wrote: > > Subject line speaks for itself! Patch attached. Committed. -- Cheers Jorge.-

March 2, 2009

[patch] Configurable key bindings

by jcid＠dillo.org

Hi there, > [Tim wrote] > It took longer to write this patch than I expected. You will find it > in the attachment but I do not recommend using it before somebody > else has verified it (I am new to C/C++). It's great to see this much needed feature in the process of making its way into the source tree! :) After cosidering the raised issues, I believe it'd be good to make our next release (2.1), with what we have in the repo (plus bug fixes) and to leave the new stuff for 2.2. That allows time to polish and test. Here go my comments. > [Corvid wrote] > - I haven't looked at this in any detail yet -- this is just my > paging-through-the-patch response -- but I notice a_Nav_push_forced > in there. Patches should be kept separate. The smaller they are, > the better for review. > > - Lines should not be longer than 79 chars. Fully agreed. > [Corvid wrote] > I lean toward not adding features to this patch yet. > > (Continuing to add features before getting anything into the > tree caused dillo not to get frames support, and that was > an unfortunate incident all around.) Very good advice. I'd like to have the bare functionality merged first, and to improve on that basis when necessary. BTW, if Corvid agrees, I'd like him to be your adviser on preparing this patch until he sends me something for an inclusion review. > [Tim wrote] > I had to re-implement the parsing routine. This is mainly because a > new line format was introduced which allows you to configure key > bindings in your dillorc. Otherwise we would have needed > unnecessarily a second file. This is the only thing that changes for > the user. The new format looks like this: "set option value" > [Tim wrote] > The downside of this patch is that currently there is an issue with > the line parser, which claims to find syntax errors even though the > lines look alright for me. This needs to be fixed before this patch > can go into mainline. > [Diego wrote] > I disagree about changing the dillorc format: > > - This change is not backwards compatible. Do not read old dillorc > format and it is a big inconvenience for users. > [Diego wrote] > - There are compatible ways to add this preferences (key_save = "...", > key_find = "CTRL + f", ...) > [Tim wrote] > Sure, but options were not initially thought of as being used for > key bindings. When using a special keyword which is only valid for > one preferences type (bindings), it is possible to add further > parameters such as groups (useful when having lots of key bindings). > The other advantage of this implementation is that we could easily > modify it to allow multiple key bindings for one symbol. > [Diego wrote] > - If the number of key bindings increase much another config file can > be convenient. With another config file some example files can be > suplied (vim keys, firefox keys, ...) and end user only need to copy > the prefered file. > [Corvid wrote] > That's a good point. This is an important issue. After giving it some tought, it looks like we can handle it like icewm and others (i.e. with multiple files for different configuration areas). BTW, this is already the case with style.css (CSS configuration file) and cookiesrc. We'd have: dillorc, style.css, keyboardrc and cookiesrc. In the future we may add others (e.g. addblock, read.css). This allows for backwards compatibility, easy separation of tasks, isolation/flexibility of configuration parsers and simple example configuration files for the user. BTW, dillorc has grown a long file to read, and will most probably end being managed by an external app. > [Corvid wrote] > - I'm sure that Jorge isn't going to want exception handling. Good guess! > [Tim wrote] > Why not? Exceptions are more flexible when it comes to longer and > more complicated functions. Of course, we could also return an > integer status code but sometimes it is necessary to return > non-integer values. We're using return codes and for the non-integer return value case we use a pointer argument. > [Johannes wrote] > I'm definately against adding exceptions. > If we would start a new project, we could discuss pros and cons of > using exceptions - I would probabely still vote against using them > in that case... > But as all current dillo code and also fltk does not use exceptions > it would get a real mess if parts of new code would start to > use exceptions for error handling while other code is using > return values. Right. Please modify the patch to use return codes. -- Cheers Jorge.-

March 1, 2009