[netsurf-dev] Re: Pull request: CSS support for libsvgtiny

On 2024-01-21 15:45:09, Michael Drake wrote:
> Hi Michael,
>
> I'm sorry it's taken so long to get back to you.

No problem, I wasn't expecting it to be a quick review. I've pushed a
few more commits to my branch that fix all of these issues (except
-pedantic, see below).


> When I tried building I got a lot of `-Werror=pedantic` warnings due to the use
> of a libwapcaplet macro....
>
> So we'd need to drop the -pedantic in the Makefile.

I actually don't see these with gcc-13.2.1? Thankfully this is the
easiest / least-important of the issues. I can still drop the flag if
you want me to do it -- I'd just feel better if I could see the errors
disappear as a result of my change.


> I also had warnings about `fill_opacity_type` and `stroke_opacity_type` being
> set but unused in `svgtiny_parse_styles`. I simply called those computed style
> getters without assigning their return value.

Fixed, thanks. These slipped through because I was accidentally
clobbering my WARNFLAGS.


> In the function `node_has_name`
>
> https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blob;f=src/svgtiny_css.c;h=7e203a2b70f09b3127108aca03f18e6269a9d582;hb=refs/heads/libcss#l643
>
> It's doing a `lwc_string_isequal` between `qname->name` (Wapcapplet string) and
> `state->interned_universal` (DOM string), which won't work. I changed
> it to a call to
> `dom_string_lwc_isequal` locally.

Fixed.


> Also I added a `break` before the `default` case in this `switch`, to
> stop it asserting when built with `make VARIANT=debug`:
>
> https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blob;f=src/svgtiny_css.c;h=7e203a2b70f09b3127108aca03f18e6269a9d582;hb=refs/heads/libcss#l1941

Fixed.


> With those changes I tried running it but found it was unstable in NetSurf.
>
> I haven't done very much testing but I could see issues running the libsvgtiny
> test runner binary under valgrind:

I was using lwc_string_destroy() where I should have been using
lwc_string_unref(). I fixed that one, and re-ran the other tests under
valgrind. They all come up clean now. Hopefully that was the cause of
the NetSurf instability.