A bunch of property parsers.

Split out !important parsing into a separate function.
Support for dumping bytecode to a file handle in some kind of human-readable format.
Strings can be represented in the bytecode as a pointer, length pair rather than embedding the string data into the bytecode -- all strings are interned by the core syntax parser.
Add todo relating to early destruction of parser object (it shouldn't be needed once parsing is complete). Note documented issue surrounding interned string dictionary, however.
In general, it seems wasteful to create a new dictionary containing string representations of keywords for every single parser instance. It would be better to have one central (statically allocated?) dictionary for this and then each parser instance can have a smaller dictionary containing any unknown strings contained within the stylesheet being parsed (e.g. string constants or URLs).

svn path=/trunk/libcss/; revision=5627

7 files changed, 558 insertions, 55 deletions

diff --git a/docs/Bytecode b/docs/Bytecode
index d999c48..e7bb2fa 100644
--- a/docs/Bytecode
+++ b/docs/Bytecode
@@ -32,9 +32,9 @@ Floats are stored in their natural format.
 Integers wider than 32 bits are not supported.
 Doubles are not supported.
 
-Strings are stored unterminated UTF8, preceded by 32bits of byte length, and 
-followed by 0-3 bytes of padding for alignment. The padding bytes must be 
-zero. The padding is not included in the byte length.
+Strings are stored as <pointer,length> pairs, as per css_string.
+Pointer's width is the native width of a pointer on the platform.
+Length's width is the native width of a size_t on the platform.
 
 CSS dimensions are stored as two 32bit values: <length, units>. 
 Length is a 32bit integer and unit is as follows:
diff --git a/src/bytecode/Makefile b/src/bytecode/Makefile
new file mode 100644
index 0000000..db4d922
--- /dev/null
+++ b/src/bytecode/Makefile
@@ -0,0 +1,49 @@
+# Child makefile fragment
+#
+# Toolchain is provided by top-level makefile
+#
+# Variables provided by top-level makefile
+#
+# COMPONENT		The name of the component
+# EXPORT		The location of the export directory
+# TOP			The location of the source tree root
+# RELEASEDIR		The place to put release objects
+# DEBUGDIR		The place to put debug objects
+#
+# do_include		Canned command sequence to include a child makefile
+#
+# Variables provided by parent makefile:
+#
+# DIR			The name of the directory we're in, relative to $(TOP)
+#
+# Variables we can manipulate:
+#
+# ITEMS_CLEAN		The list of items to remove for "make clean"
+# ITEMS_DISTCLEAN	The list of items to remove for "make distclean"
+# TARGET_TESTS		The list of target names to run for "make test"
+#
+# SOURCES		The list of sources to build for $(COMPONENT)
+#
+# Plus anything from the toolchain
+
+# Push parent directory onto the directory stack
+sp             := $(sp).x
+dirstack_$(sp) := $(d)
+d              := $(DIR)
+
+# Manipulate include paths
+CFLAGS := $(CFLAGS) -I$(d)
+
+# Sources
+SRCS_$(d) := dump.c
+
+# Append to sources for component
+SOURCES += $(addprefix $(d), $(SRCS_$(d)))
+
+# Now include any children we may have
+MAKE_INCLUDES := $(wildcard $(d)*/Makefile)
+$(eval $(foreach INC, $(MAKE_INCLUDES), $(call do_include,$(INC))))
+
+# Finally, pop off the directory stack
+d  := $(dirstack_$(sp))
+sp := $(basename $(sp))
diff --git a/src/bytecode/bytecode.h b/src/bytecode/bytecode.h
index 2db3138..86019d1 100644
--- a/src/bytecode/bytecode.h
+++ b/src/bytecode/bytecode.h
@@ -9,6 +9,7 @@
 #define css_bytecode_bytecode_h_
 
 #include <inttypes.h>
+#include <stdio.h>
 
 #include <libcss/types.h>
 
@@ -163,6 +164,8 @@ static inline bool isInherit(uint32_t OPV)
 	return getFlags(OPV) & 0x2;
 }
 
+void css_bytecode_dump(void *bytecode, uint32_t length, FILE *fp);
+
 #endif
 
 
diff --git a/src/bytecode/dump.c b/src/bytecode/dump.c
new file mode 100644
index 0000000..05213ee
--- /dev/null
+++ b/src/bytecode/dump.c
@@ -0,0 +1,229 @@
+/*
+ * This file is part of LibCSS.
+ * Licensed under the MIT License,
+ *                http://www.opensource.org/licenses/mit-license.php
+ * Copyright 2008 John-Mark Bell <jmb@netsurf-browser.org>
+ */
+
+#include "bytecode/bytecode.h"
+#include "bytecode/opcodes.h"
+
+/**
+ * Opcode names, indexed by opcode
+ */
+static const char *opcode_names[] = {
+	"azimuth",
+	"background-attachment",
+	"background-color",
+	"background-image",
+	"background-position",
+	"background-repeat",
+	"border-collapse",
+	"border-spacing",
+	"border-trbl-color",
+	"border-trbl-style",
+	"border-trbl-width",
+	"bottom",
+	"caption-side",
+	"clear",
+	"clip",
+	"color",
+	"content",
+	"counter-increment",
+	"counter-reset",
+	"cue-after",
+	"cue-before",
+	"cursor",
+	"direction",
+	"display",
+	"elevation",
+	"empty-cells",
+	"float",
+	"font-family",
+	"font-size",
+	"font-style",
+	"font-variant",
+	"font-weight",
+	"height",
+	"left",
+	"letter-spacing",
+	"line-height",
+	"list-style-image",
+	"list-style-position",
+	"list-style-type",
+	"margin-trbl",
+	"max-height",
+	"max-width",
+	"min-height",
+	"min-width",
+	"orphans",
+	"outline-color",
+	"outline-style",
+	"outline-width",
+	"overflow",
+	"padding-trbl",
+	"page-break-after",
+	"page-break-before",
+	"page-break-inside",
+	"pause-after",
+	"pause-before",
+	"pitch-range",
+	"pitch",
+	"play-during",
+	"position",
+	"quotes",
+	"richness",
+	"right",
+	"speak-header",
+	"speak-numeral",
+	"speak-punctuation",
+	"speak",
+	"speech-rate",
+	"stress",
+	"table-layout",
+	"text-align",
+	"text-decoration",
+	"text-indent",
+	"text-transform",
+	"top",
+	"unicode-bidi",
+	"vertical-align",
+	"visibility",
+	"voice-family",
+	"volume",
+	"white-space",
+	"widows",
+	"width",
+	"word-spacing",
+	"z-index",
+};
+
+/**
+ * Dump a CSS bytecode stream to the given file handle
+ * 
+ * \param bytecode  The stream to dump
+ * \param length    Length, in bytes, of bytecode
+ * \param fp        File handle to output to
+ */
+void css_bytecode_dump(void *bytecode, uint32_t length, FILE *fp)
+{
+	uint32_t offset = 0;
+
+#define ADVANCE(n) do {					\
+	offset += (n);					\
+	bytecode = ((uint8_t *) bytecode) + (n);	\
+} while(0)
+
+	while (offset < length) {
+		opcode op;
+		uint8_t flags;
+		uint16_t value;
+		uint32_t opv = *((uint32_t *) bytecode);
+
+		ADVANCE(sizeof(opv));
+
+		op = getOpcode(opv);
+
+		fprintf(fp, "%s: ", opcode_names[op]);
+
+		flags = getFlags(opv);
+
+		if (flags & FLAG_INHERIT) {
+			fprintf(fp, "inherit");
+		} else { 
+			value = getValue(opv);
+
+			switch (op) {
+			case OP_BACKGROUND_ATTACHMENT:
+				switch (value) {
+				case BACKGROUND_ATTACHMENT_FIXED:
+					fprintf(fp, "fixed");
+					break;
+				case BACKGROUND_ATTACHMENT_SCROLL:
+					fprintf(fp, "scroll");
+					break;
+				}
+				break;
+			case OP_BACKGROUND_COLOR:
+				switch (value) {
+				case BACKGROUND_COLOR_TRANSPARENT:
+					fprintf(fp, "transparent");
+					break;
+				case BACKGROUND_COLOR_SET:
+				{
+					uint32_t colour = 
+						*((uint32_t *) bytecode);
+					ADVANCE(sizeof(colour));
+					fprintf(fp, "#%08x", colour);
+				}	
+					break;
+				}
+				break;
+			case OP_BACKGROUND_IMAGE:
+				switch (value) {
+				case BACKGROUND_IMAGE_NONE:
+					fprintf(fp, "none");
+					break;
+				case BACKGROUND_IMAGE_URI:
+				{
+					uint8_t *ptr = 
+						*((uint8_t **) bytecode);
+					ADVANCE(sizeof(ptr));
+					size_t len =
+						*((size_t *) bytecode);
+					ADVANCE(sizeof(len));
+					fprintf(fp, "url('%.*s')", (int) len, 
+							(char *) ptr);
+				}
+					break;
+				}
+				break;
+			case OP_BACKGROUND_REPEAT:
+				switch (value) {
+				case BACKGROUND_REPEAT_NO_REPEAT:
+					fprintf(fp, "no-repeat");
+					break;
+				case BACKGROUND_REPEAT_REPEAT_X:
+					fprintf(fp, "repeat-x");
+					break;
+				case BACKGROUND_REPEAT_REPEAT_Y:
+					fprintf(fp, "repeat-y");
+					break;
+				case BACKGROUND_REPEAT_REPEAT:
+					fprintf(fp, "repeat");
+					break;
+				}
+				break;
+			case OP_CLEAR:
+				switch (value) {
+				case CLEAR_NONE:
+					fprintf(fp, "none");
+					break;
+				case CLEAR_LEFT:
+					fprintf(fp, "left");
+					break;
+				case CLEAR_RIGHT:
+					fprintf(fp, "right");
+					break;
+				case CLEAR_BOTH:
+					fprintf(fp, "both");
+					break;
+				}
+				break;
+			default:
+				fprintf(fp, "Unknown opcode %x", op);
+				break;
+			}
+		}
+
+		if (flags & FLAG_IMPORTANT)
+			fprintf(fp, " !important");
+
+		fprintf(fp, "; ");
+	}
+
+#undef ADVANCE
+
+}
+
+
diff --git a/src/parse/css21.c b/src/parse/css21.c
index 7f16c8d..cee929a 100644
--- a/src/parse/css21.c
+++ b/src/parse/css21.c
@@ -19,8 +19,10 @@
 #include "utils/utils.h"
 
 enum {
+	/* At-rules */
 	CHARSET, IMPORT, MEDIA, PAGE,
 
+	/* Properties */
 	FIRST_PROP,
 
 	AZIMUTH = FIRST_PROP, BACKGROUND_ATTACHMENT, BACKGROUND_COLOR, 
@@ -45,13 +47,20 @@ enum {
 	TOP, UNICODE_BIDI, VERTICAL_ALIGN, VISIBILITY, VOICE_FAMILY, VOLUME, 
 	WHITE_SPACE, WIDOWS, WIDTH, WORD_SPACING, Z_INDEX,
 
+	LAST_PROP = Z_INDEX,
+
+	/* Other keywords */
+	INHERIT, IMPORTANT, NONE, BOTH, FIXED, SCROLL, TRANSPARENT,
+	NO_REPEAT, REPEAT_X, REPEAT_Y, REPEAT,
+
 	LAST_KNOWN
 };
 
+/* Must be synchronised with above enum */
 static struct {
 	const char *ptr;
 	size_t len;
-} stringmap[] = {
+} stringmap[LAST_KNOWN] = {
 	{ "charset", SLEN("charset") },
 	{ "import", SLEN("import") },
 	{ "media", SLEN("media") },
@@ -155,7 +164,19 @@ static struct {
 	{ "widows", SLEN("widows") },
 	{ "width", SLEN("width") },
 	{ "word-spacing", SLEN("word-spacing") },
-	{ "z-index", SLEN("z-index") }
+	{ "z-index", SLEN("z-index") },
+
+	{ "inherit", SLEN("inherit") },
+	{ "important", SLEN("important") },
+	{ "none", SLEN("none") },
+	{ "both", SLEN("both") },
+	{ "fixed", SLEN("fixed") },
+	{ "scroll", SLEN("scroll") },
+	{ "transparent", SLEN("transparent") },
+	{ "no-repeat", SLEN("no-repeat") },
+	{ "repeat-x", SLEN("repeat-x") },
+	{ "repeat-y", SLEN("repeat-y") },
+	{ "repeat", SLEN("repeat") },
 };
 
 typedef struct context_entry {
@@ -1071,11 +1092,11 @@ css_error parseProperty(css_css21 *c, const css_token *property,
 
 	/* Find property index */
 	/** \todo improve on this linear search */
-	for (i = FIRST_PROP; i < LAST_KNOWN; i++) {
+	for (i = FIRST_PROP; i <= LAST_PROP; i++) {
 		if (property->lower.ptr == c->strings[i])
 			break;
 	}
-	if (i == LAST_KNOWN)
+	if (i == LAST_PROP + 1)
 		return CSS_INVALID;
 
 	/* Get handler */
diff --git a/src/parse/css21props.c b/src/parse/css21props.c
index c47ef99..09eb889 100644
--- a/src/parse/css21props.c
+++ b/src/parse/css21props.c
@@ -309,6 +309,13 @@ static css_error parse_z_index(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result);
 
+static inline css_error parse_important(css_css21 *c,
+		const parserutils_vector *vector, int *ctx,
+		uint8_t *result);
+static inline css_error parse_colour_specifier(css_css21 *c,
+		const parserutils_vector *vector, int *ctx,
+		uint32_t *result);
+
 /**
  * Type of property handler function
  */
@@ -319,7 +326,7 @@ typedef css_error (*css_prop_handler)(css_css21 *c,
 /**
  * Dispatch table of property handlers, indexed by property enum
  */
-static const css_prop_handler property_handlers[LAST_KNOWN - FIRST_PROP] =
+static const css_prop_handler property_handlers[LAST_PROP + 1 - FIRST_PROP] =
 {
 	parse_azimuth,
 	parse_background_attachment,
@@ -426,6 +433,7 @@ css_error parse_azimuth(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
+	/** \todo azimuth */
 	UNUSED(c);
 	UNUSED(vector);
 	UNUSED(ctx);
@@ -438,10 +446,39 @@ css_error parse_background_attachment(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
-	UNUSED(c);
-	UNUSED(vector);
-	UNUSED(ctx);
-	UNUSED(result);
+	css_error error;
+	const css_token *ident;
+	uint8_t flags = 0;
+	uint16_t value = 0;
+	uint32_t opv;
+
+	/* IDENT (fixed, scroll, inherit) */
+	ident = parserutils_vector_iterate(vector, ctx);
+	if (ident == NULL || ident->type != CSS_TOKEN_IDENT)
+		return CSS_INVALID;
+
+	error = parse_important(c, vector, ctx, &flags);
+	if (error != CSS_OK)
+		return error;
+
+	if (ident->lower.ptr == c->strings[INHERIT]) {
+		flags |= FLAG_INHERIT;
+	} else if (ident->lower.ptr == c->strings[FIXED]) {
+		value = BACKGROUND_ATTACHMENT_FIXED;
+	} else if (ident->lower.ptr == c->strings[SCROLL]) {
+		value = BACKGROUND_ATTACHMENT_SCROLL;
+	} else
+		return CSS_INVALID;
+
+	opv = buildOPV(OP_BACKGROUND_ATTACHMENT, flags, value);
+
+	/* Allocate result */
+	*result = css_stylesheet_style_create(c->sheet, sizeof(opv));
+	if (*result == NULL)
+		return CSS_NOMEM;
+
+	/* Copy the bytecode to it */
+	memcpy((*result)->bytecode, &opv, sizeof(opv));
 
 	return CSS_OK;
 }
@@ -450,10 +487,56 @@ css_error parse_background_color(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
-	UNUSED(c);
-	UNUSED(vector);
-	UNUSED(ctx);
-	UNUSED(result);
+	css_error error;
+	const css_token *token;
+	uint8_t flags = 0;
+	uint16_t value = 0;
+	uint32_t opv;
+	uint32_t colour = 0;
+	uint32_t required_size;
+
+	/* colour | IDENT (transparent, inherit) */
+	token= parserutils_vector_peek(vector, *ctx);
+	if (token == NULL)
+		return CSS_INVALID;
+
+	if (token->type == CSS_TOKEN_IDENT && 
+			token->lower.ptr == c->strings[INHERIT]) {
+		parserutils_vector_iterate(vector, ctx);
+		flags |= FLAG_INHERIT;
+	} else if (token->type == CSS_TOKEN_IDENT &&
+			token->lower.ptr == c->strings[TRANSPARENT]) {
+		parserutils_vector_iterate(vector, ctx);
+		value = BACKGROUND_COLOR_TRANSPARENT;
+	} else {
+		error = parse_colour_specifier(c, vector, ctx, &colour);
+		if (error != CSS_OK)
+			return CSS_INVALID;
+
+		value = BACKGROUND_COLOR_SET;
+	}
+
+	error = parse_important(c, vector, ctx, &flags);
+	if (error != CSS_OK)
+		return error;
+
+	opv = buildOPV(OP_BACKGROUND_COLOR, flags, value);
+
+	required_size = sizeof(opv);
+	if (value == BACKGROUND_COLOR_SET)
+		required_size += sizeof(colour);
+
+	/* Allocate result */
+	*result = css_stylesheet_style_create(c->sheet, required_size);
+	if (*result == NULL)
+		return CSS_NOMEM;
+
+	/* Copy the bytecode to it */
+	memcpy((*result)->bytecode, &opv, sizeof(opv));
+	if (value == BACKGROUND_COLOR_SET) {
+		memcpy(((uint8_t *) (*result)->bytecode) + sizeof(opv),
+				&colour, sizeof(colour));
+	}
 
 	return CSS_OK;
 }
@@ -462,10 +545,54 @@ css_error parse_background_image(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
-	UNUSED(c);
-	UNUSED(vector);
-	UNUSED(ctx);
-	UNUSED(result);
+	css_error error;
+	const css_token *token;
+	uint8_t flags = 0;
+	uint16_t value = 0;
+	uint32_t opv;
+	uint32_t required_size;
+
+	/* URI | IDENT (none, inherit) */
+	token = parserutils_vector_iterate(vector, ctx);
+	if (token == NULL || (token->type != CSS_TOKEN_IDENT &&
+			token->type != CSS_TOKEN_URI))
+		return CSS_INVALID;
+
+	error = parse_important(c, vector, ctx, &flags);
+	if (error != CSS_OK)
+		return error;
+
+	if (token->type == CSS_TOKEN_IDENT && 
+			token->lower.ptr == c->strings[INHERIT]) {
+		flags |= FLAG_INHERIT;
+	} else if (token->type == CSS_TOKEN_IDENT && 
+			token->lower.ptr == c->strings[NONE]) {
+		value = BACKGROUND_IMAGE_NONE;
+	} else if (token->type == CSS_TOKEN_URI) {
+		value = BACKGROUND_IMAGE_URI;
+	} else
+		return CSS_INVALID;
+
+	opv = buildOPV(OP_BACKGROUND_IMAGE, flags, value);
+
+	required_size = sizeof(opv);
+	if (value == BACKGROUND_IMAGE_URI)
+		required_size += sizeof(uint8_t *) + sizeof(size_t);
+
+	/* Allocate result */
+	*result = css_stylesheet_style_create(c->sheet, required_size);
+	if (*result == NULL)
+		return CSS_NOMEM;
+
+	/* Copy the bytecode to it */
+	memcpy((*result)->bytecode, &opv, sizeof(opv));
+	if (value == BACKGROUND_IMAGE_URI) {
+		memcpy((uint8_t *) (*result)->bytecode + sizeof(opv),
+				&token->data.ptr, sizeof(uint8_t *));
+		memcpy((uint8_t *) (*result)->bytecode + sizeof(opv) + 
+					sizeof(uint8_t *),
+				&token->data.len, sizeof(size_t));
+	}
 
 	return CSS_OK;
 }
@@ -474,6 +601,7 @@ css_error parse_background_position(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
+	/** \todo background-position */
 	UNUSED(c);
 	UNUSED(vector);
 	UNUSED(ctx);
@@ -486,10 +614,43 @@ css_error parse_background_repeat(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
-	UNUSED(c);
-	UNUSED(vector);
-	UNUSED(ctx);
-	UNUSED(result);
+	css_error error;
+	const css_token *ident;
+	uint8_t flags = 0;
+	uint16_t value = 0;
+	uint32_t opv;
+
+	/* IDENT (no-repeat, repeat-x, repeat-y, repeat, inherit) */
+	ident = parserutils_vector_iterate(vector, ctx);
+	if (ident == NULL || ident->type != CSS_TOKEN_IDENT)
+		return CSS_INVALID;
+
+	error = parse_important(c, vector, ctx, &flags);
+	if (error != CSS_OK)
+		return error;
+
+	if (ident->lower.ptr == c->strings[INHERIT]) {
+		flags |= FLAG_INHERIT;
+	} else if (ident->lower.ptr == c->strings[NO_REPEAT]) {
+		value = BACKGROUND_REPEAT_NO_REPEAT;
+	} else if (ident->lower.ptr == c->strings[REPEAT_X]) {
+		value = BACKGROUND_REPEAT_REPEAT_X;
+	} else if (ident->lower.ptr == c->strings[REPEAT_Y]) {
+		value = BACKGROUND_REPEAT_REPEAT_Y;
+	} else if (ident->lower.ptr == c->strings[REPEAT]) {
+		value = BACKGROUND_REPEAT_REPEAT;
+	} else
+		return CSS_INVALID;
+
+	opv = buildOPV(OP_BACKGROUND_REPEAT, flags, value);
+
+	/* Allocate result */
+	*result = css_stylesheet_style_create(c->sheet, sizeof(opv));
+	if (*result == NULL)
+		return CSS_NOMEM;
+
+	/* Copy the bytecode to it */
+	memcpy((*result)->bytecode, &opv, sizeof(opv));
 
 	return CSS_OK;
 }
@@ -690,7 +851,8 @@ css_error parse_clear(css_css21 *c,
 		const parserutils_vector *vector, int *ctx, 
 		css_style **result)
 {
-	const css_token *token, *ident;
+	css_error error;
+	const css_token *ident;
 	uint8_t flags = 0;
 	uint16_t value = 0;
 	uint32_t opv;
@@ -700,41 +862,19 @@ css_error parse_clear(css_css21 *c,
 	if (ident == NULL || ident->type != CSS_TOKEN_IDENT)
 		return CSS_INVALID;
 
-	/** \todo break this !important stuff into a utility function */
-	consumeWhitespace(vector, ctx);
-
-	token = parserutils_vector_iterate(vector, ctx);
-	if (token != NULL && tokenIsChar(token, '!')) {
-		consumeWhitespace(vector, ctx);
+	error = parse_important(c, vector, ctx, &flags);
+	if (error != CSS_OK)
+		return error;
 
-		token = parserutils_vector_iterate(vector, ctx);
-		if (token == NULL || token->type != CSS_TOKEN_IDENT)
-			return CSS_INVALID;
-
-		/** \todo compare pointer to interned version. */
-		if (token->lower.len == 9 &&
-				strncmp((char *) token->lower.ptr, 
-					"important", 9) == 0)
-			flags |= FLAG_IMPORTANT;
-	} else if (token != NULL)
-		return CSS_INVALID;
-
-
-	/** \todo ugh. compare pointers to interned versions, already */
-	if (ident->lower.len == 7 &&
-			strncmp((char *) ident->lower.ptr, "inherit", 7) == 0) {
+	if (ident->lower.ptr == c->strings[INHERIT]) {
 		flags |= FLAG_INHERIT;
-	} else if (ident->lower.len == 5 && 
-			strncmp((char *) ident->lower.ptr, "right", 5) == 0) {
+	} else if (ident->lower.ptr == c->strings[RIGHT]) {
 		value = CLEAR_RIGHT;
-	} else if (ident->lower.len == 4 &&
-			strncmp((char *) ident->lower.ptr, "left", 4) == 0) {
+	} else if (ident->lower.ptr == c->strings[LEFT]) {
 		value = CLEAR_LEFT;
-	} else if (ident->lower.len == 4 &&
-			strncmp((char *) ident->lower.ptr, "both", 4) == 0) {
+	} else if (ident->lower.ptr == c->strings[BOTH]) {
 		value = CLEAR_BOTH;
-	} else if (ident->lower.len == 4 &&
-			strncmp((char *) ident->lower.ptr, "none", 4) == 0) {
+	} else if (ident->lower.ptr == c->strings[NONE]) {
 		value = CLEAR_NONE;
 	} else
 		return CSS_INVALID;
@@ -1664,4 +1804,48 @@ css_error parse_z_index(css_css21 *c,
 	return CSS_OK;
 }
 
+css_error parse_important(css_css21 *c,
+		const parserutils_vector *vector, int *ctx,
+		uint8_t *result)
+{
+	const css_token *token;
+
+	consumeWhitespace(vector, ctx);
+
+	token = parserutils_vector_iterate(vector, ctx);
+	if (token != NULL && tokenIsChar(token, '!')) {
+		consumeWhitespace(vector, ctx);
+
+		token = parserutils_vector_iterate(vector, ctx);
+		if (token == NULL || token->type != CSS_TOKEN_IDENT)
+			return CSS_INVALID;
+
+		if (token->lower.ptr == c->strings[IMPORTANT])
+			*result |= FLAG_IMPORTANT;
+	} else if (token != NULL)
+		return CSS_INVALID;
+
+	return CSS_OK;
+}
+
+css_error parse_colour_specifier(css_css21 *c,
+		const parserutils_vector *vector, int *ctx,
+		uint32_t *result)
+{
+	const css_token *token;
+
+	UNUSED(c);
+	UNUSED(result);
+
+	/** \todo Parse colours */
+
+	/* For now, consume everything up to the end of the declaration or !, 
+ 	 * whichever comes first */
+	while ((token = parserutils_vector_peek(vector, *ctx)) != NULL &&
+			tokenIsChar(token, '!') == false)
+		parserutils_vector_iterate(vector, ctx);
+
+	return CSS_OK;
+}
+
 #endif
diff --git a/src/stylesheet.c b/src/stylesheet.c
index 7a093f9..7608b16 100644
--- a/src/stylesheet.c
+++ b/src/stylesheet.c
@@ -8,6 +8,7 @@
 #include <string.h>
 
 #include "stylesheet.h"
+#include "bytecode/bytecode.h"
 #include "parse/css21.h"
 #include "utils/utils.h"
 
@@ -158,6 +159,13 @@ css_error css_stylesheet_data_done(css_stylesheet *sheet)
 		return CSS_BADPARM;
 
 	return css_parser_completed(sheet->parser);
+
+	/** \todo We can destroy the parser here as it won't be needed
+	 * Note, however, that, if we do so, then the dictionary of
+	 * strings created by the parser *must* be preserved (and, ideally,
+	 * created by us in the first place) because our stylesheet 
+	 * datastructures contain pointers to strings stored in this
+	 * dictionary. */
 }
 
 /**
@@ -531,6 +539,9 @@ css_error css_stylesheet_rule_append_style(css_stylesheet *sheet,
 		if (temp == NULL)
 			return CSS_NOMEM;
 
+		/* Ensure bytecode pointer is correct */
+		temp->bytecode = ((uint8_t *) temp + sizeof(css_style));
+
 		/** \todo Can we optimise the bytecode here? */
 		memcpy((uint8_t *) temp->bytecode + temp->length, 
 				style->bytecode, style->length);
@@ -666,6 +677,12 @@ void css_stylesheet_dump_rule(css_rule *rule, FILE *target)
 			if (i != rule->data.selector.selector_count - 1)
 				fprintf(target, ", ");
 		}
+		fprintf(target, " { ");
+		if (rule->data.selector.style != NULL) {
+			css_bytecode_dump(rule->data.selector.style->bytecode,
+				rule->data.selector.style->length, target);
+		}
+		fprintf(target, "}");
 		break;
 	case CSS_RULE_CHARSET:
 	case CSS_RULE_IMPORT: