Review: ns-libcss (round 1)

Daniel Silverstone dsilvers at netsurf-browser.org
Thu Jul 23 15:36:05 BST 2009


On Thu, 2009-07-23 at 01:22 +0100, John-Mark Bell wrote:
> Its runtime status is unknown.

Presumably it is at least functional on GTK?

> Known regressions wrt trunk:
> 
> 1. HTML alignment hints are ignored (this means <center>).
>    The intention is to fix this on trunk after merging. 

What is the reasoning for not fixing this pre-merge?

> 2. Layout relaxation for page-based media has been removed wholesale.
>    The correct solution is to make the layout engine cater for paged media also.
>    For the timebeing, output to page-based media will be suboptimal.

does suboptimal mean worthless, or just not as nice as it could be?

> 1. option_font_min_size is now ignored completely. This was introduced to
>    work around pages using tables with font sizes <100%. Instead, we implement
>    a layout quirk that resets font properties to their defaults when 
>    encountering a table element. See quirks.css for further details.

Firefox still offers a min-font-size option. Would this not be useful in
the case (for example) of users who have high res screens and small
fonts in general use being otherwise boned by people who use <font
size="-3"> type things. Hell, sometimes I wish people wouldn't use
<small> but they do. A 50% font-size will make things illegible to
me :-(

> Index: framebuffer/res/quirks.css
> @@ -0,0 +1,9 @@
> +/* Quirks mode stylesheet for NetSurf */
> +
> +table {

A short comment saying *why* this is here would be useful so we don't
lose the information.

> Index: gtk/res/quirks.css
> Index: !NetSurf/Resources/Quirks,f79

AFAICT these are all the same content. Is it that the diff has gone
wrong, or did you really add the same file in three places rather than
in the !NetSurf dir and then use symlinks?

> Index: css/utils.c
> +css_fixed nscss_len2pt(css_fixed length, css_unit unit)
> +{
> +	/* Length must not be relative */
> +	assert(unit != CSS_UNIT_EM && unit != CSS_UNIT_EX);
> +
> +	switch (unit) {
> +	/* We assume the screen and any other output has the same dpi */

Given this assumption, we presumably can change nscss_screen_dpi, render
a print output, and then change back, should we wish? If so, should it
be nscss_render_dpi ?

> +	case CSS_UNIT_CM: return FMUL(length, FLTTOFIX(28.452756));
> +	case CSS_UNIT_MM: return FMUL(length, FLTTOFIX(2.8452756));

These magic values offend me. Can we have #defines for them with a
comment stating what the numbers were derived from?

> +css_fixed nscss_len2px(css_fixed length, css_unit unit, 
> +		const css_computed_style *style)
> +	case CSS_UNIT_CM: return FDIV(lendpi, FLTTOFIX(2.54));
> +	case CSS_UNIT_MM: return FDIV(lendpi, FLTTOFIX(25.4));

Again, magic numbers which could do with explanation.

> Index: css/dump.c
> +
> +/**
> + * Dump a computed style to the give file handle

Better:

Dump computed style \a style, as valid CSS, to the given file handle \a
stream.

\note This does not dump a selector for the style beforehand. If a
selector is wanted, ensure that it is written to \a stream first.

(however, claiming valid CSS is a touch fraught, this would require the
insertion of some semicolons and/or newlines into the output stream :-)

> +void nscss_dump_computed_style(FILE *stream, const css_computed_style *style)
> +	/* border-bottom-color */
> +	val = css_computed_border_bottom_color(style, &color);

These four are begging to be refactored out into their own
nscss_dump_colour(stream, "border-bottom-color", val, color); type
calls.

> +	/* border-top-style */
> +	val = css_computed_border_top_style(style);

ditto nscss_dump_border_style(....)

> +	/* border-top-width */
> +	val = css_computed_border_top_width(style, &len1, &unit1);

ditto nscss_dump_border_width(....)

> +	/* bottom */
> +	val = css_computed_bottom(style, &len1, &unit1);

bottom, top, left, right, probably all factorable out if only it wasn't
CSS_BOTTOM_AUTO, CSS_TOP_AUTO etc. Also height and width might be able
to be factored similarly. Perhaps this idea might work:

nscss_dump_dimension(stream, "bottom", val == CSS_BOTTOM_AUTO, len1,
unit1);

> +	/* margin-top */
> +	val = css_computed_margin_top(style, &len1, &unit1);

Oh look, a refactoring opportunity like the above. I *bet* padding will
be similar.

> +	/* max-height */
> +	/* max-width */
> +	/* min-height */
> +	/* min-width */

More!

> +	/* padding-top */
> +	/* padding-right */
> +	/* padding-bottom */
> +	/* padding-left */

Yep, these too.

> +/******************************************************************************
> + * Helper functions                                                           *
> + ******************************************************************************/

Helper for what/who? If they're only used in here, they should be
static. If they're exported, can they be nscss_dump_* please?

> +/**
> + * Dump a fixed point value to the stream

...in a textual form.

> +#define ABS(x) (uint32_t)((x) < 0 ? -(x) : (x))

Can you please use something more locally scoped namewise? Just in case
some platform goes ahead and defines ABS. Esp. since your ABS casts too
for fun.

> +	uint32_t uintpart = FIXTOINT(ABS(f));
> +	/* + 500 to ensure round to nearest (division will truncate) */
> +	uint32_t fracpart = ((ABS(f) & 0x3ff) * 1000 + 500) / (1 << 10);

This looks remarkably like we need a FIXTOFRACT type macro in libcss.

> +#undef ABS
> +	size_t flen = 0;
> +	char tmp[20];
> +	size_t tlen = 0;
> +
[snip printout of number]

Any particular reason for not doing fprintf(stream, "%s%d.%03d", (f <
0) ? "-" : "", uintpart, fracpart)

?

(may need a bit of format string or number manipulation frobbery)

> +/**
> + * Dump a dimension to the stream

...in a textual form.

> +void dump_css_unit(FILE *stream, css_fixed val, css_unit unit)

static, or nscss_dump_unit ?

> Index: css/utils.h
> +#ifndef netsurf_css_utils_h_
> +#define netsurf_css_utils_h_

Header guard defines really ought to be in capital letters.

> +static inline colour nscss_color_to_ns(css_color color)

inline? safe for core?

> Index: css/dump.h
> +#ifndef netsurf_css_dump_h_
> +#define netsurf_css_dump_h_

guards in caps please.

> +void nscss_dump_computed_style(FILE *stream, const css_computed_style *style);

That this is the only exported function implies those others really
ought to have been static.

> Index: css/select.c

[snip vast set of static predeclarations]

Do we *really* need these predeclarations, or could things be better
done if you reordered the file to not need them? Predeclarations reduce
ease of maintenance. It's not hard to look for the handler table at the
end of the file. In general, if I see a file with a bunch of static
predeclarations like that, my first thought is "how incestuous is this
code and could it be better rewritten to not be so?".

> +css_error nscss_compute_font_size(void *pw, const css_hint *parent,
> +		css_hint *size)
> +{
> +	static const css_hint_length sizes[] = {
> +		{ FLTTOFIX(0.5625), CSS_UNIT_PT }, /* xx-small */
> +		{ FLTTOFIX(0.6250), CSS_UNIT_PT }, /* x-small */
> +		{ FLTTOFIX(0.8125), CSS_UNIT_PT }, /* small */
> +		{ FLTTOFIX(1.0000), CSS_UNIT_PT }, /* medium */
> +		{ FLTTOFIX(1.1250), CSS_UNIT_PT }, /* large */
> +		{ FLTTOFIX(1.5000), CSS_UNIT_PT }, /* x-large */
> +		{ FLTTOFIX(2.0000), CSS_UNIT_PT }  /* xx-large */

Again, some indication of what those magic numbers are would be nice.

> +bool nscss_parse_colour(const char *data, css_color *result)

Is this not functionality which belongs inside libcss? Or is it HTML
specific?

> +css_error node_name(void *pw, void *node,
> +		lwc_context *dict, lwc_string **name)

> +	lerror = lwc_context_intern(dict, (const char *) n->name,
> +			strlen((const char *) n->name), name);

It would be nice if we could snarf a ref to this name for the node, so
that the next time the node's name is asked for, we don't have to wait
for lwc_context_intern to spot it already has it. Dunno if that
increases the complexity of being able to chuck the libxml node tree
though.

> +css_error named_ancestor_node(void *pw, void *node,
> +		lwc_string *name, void **ancestor)
> +{
> +	xmlNode *n = node;
> +	size_t len = lwc_string_length(name);
> +	const char *data = lwc_string_data(name);
> +
> +	*ancestor = NULL;
> +
> +	for (n = n->parent; n != NULL && n->type == XML_ELEMENT_NODE;
> +			n = n->parent) {
> +		bool match = strlen((const char *) n->name) == len &&
> +				strncasecmp((const char *) n->name,
> +					data, len) == 0;

All of a sudden, the ability to intern the strings in the xmlnode makes
more sense, unless something else is interning them in a duplicated
tree?

> +css_error named_parent_node(void *pw, void *node,
> +		lwc_string *name, void **parent)
> +			strlen((const char *) n->parent->name) == len &&
> +			strncasecmp((const char *) n->parent->name,
> +				data, len) == 0)

Yet more string comparison better done by lwc?

> +css_error named_sibling_node(void *pw, void *node,
> +		lwc_string *name, void **sibling)
> +	if (n->prev != NULL && strlen((const char *) n->prev->name) == len &&
> +			strncasecmp((const char *) n->prev->name,
> +				data, len) == 0)

Ditto?

> +css_error node_has_name(void *pw, void *node,
> +		lwc_string *name, bool *match)
> +{
> +	xmlNode *n = node;
> +	size_t len = lwc_string_length(name);
> +	const char *data = lwc_string_data(name);
> +
> +	/* Element names are case insensitive in HTML */
> +	*match = strlen((const char *) n->name) == len &&
> +			strncasecmp((const char *) n->name, data, len) == 0;

Another case for lwc comparison?

> +css_error node_has_class(void *pw, void *node,
> +		lwc_string *name, bool *match)

I get the feeling this function will become more lwcish when we can use
libdom?

> +css_error node_has_id(void *pw, void *node,
> +		lwc_string *name, bool *match)

Again, interning an lwc_string against the node could help here.

> +css_error node_has_attribute(void *pw, void *node,
> +		lwc_string *name, bool *match)

> +	buf = malloc(lwc_string_length(name) + 1);
> +	if (buf == NULL)
> +		return CSS_NOMEM;
> +
> +	memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> +	buf[lwc_string_length(name)] = '\0';

I wish this copying wasn't necessary. Should we alter lwc to guarantee
null termination (regardless of whether or not the string contains a
NULL anyway)? Lua does that, despite being ptr+length, it guarantees to
NULL terminate the strings it returns to you.

> +css_error node_has_attribute_equal(void *pw, void *node,
> +		lwc_string *name, lwc_string *value,
> +		bool *match)

> +	memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> +	buf[lwc_string_length(name)] = '\0';

Ditto.

> +css_error node_has_attribute_dashmatch(void *pw, void *node,
> +		lwc_string *name, lwc_string *value,
> +		bool *match)

> +
> +	memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> +	buf[lwc_string_length(name)] = '\0';

I'm beginning to think it's less stupid that it might have been.

> +						strncasecmp(start,
> +							lwc_string_data(value),
> +							vlen) == 0) {

*sob* We need something interning values. Roll on libdom.

> +css_error node_has_attribute_includes(void *pw, void *node,
> +		lwc_string *name, lwc_string *value,
> +		bool *match)

> +	buf = malloc(lwc_string_length(name) + 1);
> +	if (buf == NULL)
> +		return CSS_NOMEM;
> +
> +	memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> +	buf[lwc_string_length(name)] = '\0';

Another case for NULL termination in lwc.

> +css_error node_is_hover(void *pw, void *node, bool *match)
> +{
> +	*match = false;

A \todo for this?

> +css_error node_is_active(void *pw, void *node, bool *match)
> +{
> +	*match = false;

Ditto

> +css_error node_is_focus(void *pw, void *node, bool *match)
> +{
> +	*match = false;

Ditto.

> +css_error node_is_lang(void *pw, void *node,
> +		lwc_string *lang, bool *match)
> +{
> +	*match = false;

Ditto.


> +css_error ua_default_for_property(void *pw, uint32_t property, css_hint *hint)

> +	} else if (property == CSS_PROP_FONT_FAMILY) {
> +		hint->data.strings = NULL;
> +		hint->status = CSS_FONT_FAMILY_SANS_SERIF;

Should we have an option for this, like IE and firefox do?

> +int cmp_colour_name(const void *a, const void *b)
> +{
> +	const char *aa = a;
> +	const struct { const char *name; css_color color; } *bb = b;

Where has this struct definition come from? It belongs in a header
somewhere surely?

> +bool parse_named_colour(const char *name, css_color *result)
> +{
> +	static const struct colour_map {
> +		const char *name;
> +		css_color color;
> +	} named_colours[] = {

Are these HTML defined colours? If not, can they belong to libcss? If so, should they belong to hubbub?

> +bool parse_number(const char *data, bool maybe_negative, bool real,
> +		css_fixed *value, size_t *consumed)

This function feels like it ought to be part of libcss. Or am I
misunderstanding where numbers are defined?

> +/**
> + * Determine if a given character is whitespace
> + *
> + * \param c  Character to consider
> + * \return true if character is whitespace, false otherwise
> + */
> +bool isWhitespace(char c)
> +{
> +	return c == ' ' || c == '\t' || c == '\f' || c == '\r' || c == '\n';
> +}

Given you take 'char' not 'int' (and thus cannot be doing unicode
codepoints) what's wrong with ctype's isspace() here?

> Index: css/internal.h
> +#ifndef netsurf_css_internal_h_
> +#define netsurf_css_internal_h_

Guard in caps please.

> Index: css/select.h

> +#ifndef netsurf_css_select_h_
> +#define netsurf_css_select_h_

Guard in caps please.

> Index: beos/res/quirks.css
> Index: amiga/resources/quirks.css

Another two quirks files, or is this more case of diff not being sane in
the face of new symlinks?


> Index: render/box_normalise.c
> ===================================================================
>  struct span_info {
> +	/* Number of rows this cell spans */
>  	unsigned int row_span;
> +	/* The cell in this column spans all rows until the end of the table */
>  	bool auto_row;
> -	bool auto_column;
>  };

If you're going to add comments, please just turn the struct commentary
into a doxygen documentation comment set.

 
>  struct columns {
> +	/* Current column index */
>  	unsigned int current_column;
> -	bool extra;
>  	/* Number of columns in main part of table 1..max columns */
>  	unsigned int num_columns;
> -	/* Information about columns in main table,
> -	  array 0 to num_columns - 1 */
> +	/* Information about columns in main table, array [0, num_columns) */
>  	struct span_info *spans;
> -	/* Number of columns that have cells after a colspan 0 */
> -	unsigned int extra_columns;
>  	/* Number of rows in table */
>  	unsigned int num_rows;
>  };

Ditto.
  
>  	gui_multitask();

This is bare here, should it be in an ifdef riscos?

> @@ -282,38 +321,43 @@ 
> -	if (table->children == 0) {
> +	if (table->children == NULL) {

Thank you for all these particular fixes. Here, have a virtual oatmeal
cookie.

> @@ -596,42 +745,27 @@
>  			assert(0);

*pout* These kinds of assertions make baby Daniel cry.

> Index: render/html.c
> @@ -343,7 +367,8 @@
> @@ -467,7 +492,7 @@
> +	/*box_dump(stderr, c->data.html.layout->children, 0);*/

Is box_dump not conditional on some debug thingy anyway? Or is this a
case of needing a better debugging infrastructure?

> Index: render/html_redraw.c

Out of interest, the css_computed_*() functions -- are they computing
each time, or caching? If they're computing each time, that's gonna hurt
performance, no?

> Index: render/layout.c
> @@ -328,9 +335,10 @@
... 
>  		if (box->style) {

Aww, you missed these throughout the file. I'm taking back that cookie.

> @@ -1183,33 +1224,37 @@
> @@ -1217,72 +1262,91 @@
>  	if (height) {

if height is what?

> @@ -1290,53 +1354,60 @@
>  	if (max_width) {

if max_width is what?

I could go on, but I won't :-)

> @@ -1435,7 +1597,7 @@
> +
> +		if (c->style) {

Oh but I will here... Grrrrr Mister. It's one thing to not fix any you
spot, it's another to *add* one.

> @@ -3567,21 +3842,25 @@
>  				 * ansestor with float_children, rather than

ancestor

Most of the rest of the code I've kinda skipped because I don't
understand the vast majority anyway, especially in +/- littered diff
world. Nothing jumped out at me though.

My brain hurts.

D.





More information about the netsurf-dev mailing list