On Wed, May 04, 2011 at 10:33:08PM +0100, John-Mark Bell wrote:
>
> Precis:
>
> This changeset introduces a content factory with which content handlers may
> be dynamically registered.
> Additionally, it inverts the inheritance model of contents such that each
> content handler becomes self-contained.
schweeet!
as usual my review will be inline and I shall elide any bits I have no
comment on
> Index: render/html_internal.h
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ render/html_internal.h 2011-05-04 22:29:08.000000000 +0100
> @@ -0,0 +1,103 @@
> +/*
> + * Copyright 2004 James Bursa <bu...@users.sourceforge.net>
really? I think its 2011 and you added this file ;-) maybe its a move tho?
> +
> +/** Data specific to CONTENT_HTML. */
> +typedef struct html_content {
> + struct content base;
> +
> + void *parser_binding;
> + xmlDoc *document;
> + binding_quirks_mode quirks; /**< Quirkyness of document */
> +
> + char *encoding; /**< Encoding of source, 0 if unknown. */
> + binding_encoding_source encoding_source;
> + /**< Source of encoding information. */
> +
> + char *base_url; /**< Base URL (may be a copy of content->url). */
> + char *base_target; /**< Base target */
> +
> + struct box *layout; /**< Box tree, or 0. */
> + colour background_colour; /**< Document background colour. */
> + const struct font_functions *font_func;
> +
> + /** Number of entries in stylesheet_content. */
> + unsigned int stylesheet_count;
> + /** Stylesheets. Each may be 0. */
> + struct html_stylesheet *stylesheets;
> + /**< Style selection context */
> + css_select_ctx *select_ctx;
> +
> + /** Number of entries in object_list. */
> + unsigned int num_objects;
> + /** List of objects. */
> + struct content_html_object *object_list;
> + /** Forms, in reverse order to document. */
> + struct form *forms;
> + /** Hash table of imagemaps. */
> + struct imagemap **imagemaps;
> +
> + /** Browser window containing this document, or 0 if not open. */
> + struct browser_window *bw;
> +
> + /** Frameset information */
> + struct content_html_frames *frameset;
> +
> + /** Inline frame information */
> + struct content_html_iframe *iframe;
> +
> + /** Content of type CONTENT_HTML containing this, or 0 if not an object
> + * within a page. */
> + struct html_content *page;
> + /** Box containing this, or 0 if not an object. */
> + struct box *box;
> +} html_content;
gah make a decision on before or after doccomments within one struct
;-) and are we saying 0 or NULL for empty ponters?
> +
> +
> +bool html_fetch_object(html_content *c, const char *url, struct box *box,
> + content_type permitted_types,
> + int available_width, int available_height,
> + bool background);
> +
> +void html_set_status(html_content *c, const char *extra);
> +
> +/* in render/html_redraw.c */
> +bool html_redraw(struct content *c, int x, int y,
> + int width, int height, const struct rect *clip,
> + float scale, colour background_colour);
> +
> +/* in render/html_interaction.c */
> +void html_mouse_track(struct content *c, struct browser_window *bw,
> + browser_mouse_state mouse, int x, int y);
> +void html_mouse_action(struct content *c, struct browser_window *bw,
> + browser_mouse_state mouse, int x, int y);
> +void html_overflow_scroll_callback(void *client_data,
> + struct scroll_msg_data *scroll_data);
> +
> +#endif
no doccomments?
> Index: image/image.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ image/image.c 2011-05-04 22:29:39.000000000 +0100
> +
> +nserror image_init(void)
> +{
comment? or at least a ref to the header?
> +
> +void image_fini(void)
ditto
> Index: content/content_factory.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ content/content_factory.c 2011-05-04 22:30:17.000000000 +0100
file comment?
> +
> +typedef struct content_handler_entry {
> + struct content_handler_entry *next;
> +
> + lwc_string *mime_type;
> + const content_handler *handler;
> +} content_handler_entry;
whassat for then? comment teh struct a bit? tbh i shall shut up about
comments now, its boring
> +
> +static content_handler_entry *content_handlers;
> +
> +nserror content_factory_register_handler(lwc_string *mime_type,
> + const content_handler *handler)
> +{
> + content_handler_entry *e;
e? really? perhaps entry might have been a little less brief?
> +
> +content_type content_factory_type_from_mime_type(const char *mime_type)
> +{
> + const content_handler *handler;
> + lwc_string *imime_type;
> + lwc_error lerror;
> + content_type type;
> +
> + lerror = lwc_intern_string(mime_type, strlen(mime_type), &imime_type);
> + if (lerror != lwc_error_ok)
> + return CONTENT_NONE;
> +
> + handler = content_lookup(imime_type);
> + if (handler == NULL) {
> + lwc_string_unref(imime_type);
> + return CONTENT_NONE;
> + }
> +
> + type = handler->type(imime_type);
> +
> + lwc_string_unref(imime_type);
> +
> + return type;
> +}
maybe? better as:
content_type type = CONTENT_NONE;
lerror = lwc_intern_string(mime_type, strlen(mime_type), &imime_type);
if (lerror != lwc_error_ok)
return CONTENT_NONE;
handler = content_lookup(imime_type);
if (handler != NULL) {
type = handler->type(imime_type);
}
lwc_string_unref(imime_type);
return type;
>
> Changed files
>
>
> 94 files changed, 4614 insertions(+), 3043 deletions(-)
yikes!
> Index: render/html.c
> ===================================================================
> --- render/html.c (revision 12276)
> +++ render/html.c (working copy)
>
> +static const content_handler html_content_handler = {
> + html_create,
> + html_process_data,
> + html_convert,
> + html_reformat,
> + html_destroy,
> + html_stop,
> + html_mouse_track,
> + html_mouse_action,
> + html_redraw,
> + NULL,
> + html_open,
> + html_close,
> + html_clone,
> + NULL,
> + html_content_type,
> + true
> +};
I was gonna ask about using named initialisors but i ust remembered
why now...grrrr
>
> error:
> if (error == BINDING_BADENCODING) {
> - LOG(("Bad encoding: %s", html->encoding ? html->encoding : ""));
> + LOG(("Bad encoding: %s", c->encoding ? c->encoding : ""));
> msg_data.error = messages_get("ParsingFail");
> } else
> msg_data.error = messages_get("NoMemory");
>
> - content_broadcast(c, CONTENT_MSG_ERROR, msg_data);
> - return false;
> + content_broadcast(&c->base, CONTENT_MSG_ERROR, msg_data);
> +
> + return NSERROR_NOMEM;
always no memory?
>
> @@ -1176,26 +1270,22 @@
> nserror html_convert_css_callback(hlcache_handle *css,
> const hlcache_event *event, void *pw)
> {
> - struct content *parent = pw;
> + html_content *parent = pw;
> unsigned int i;
gahhhh not blooody fortran
>
> /**
> @@ -2174,12 +2209,11 @@
> */
> hlcache_handle *html_get_favicon(hlcache_handle *h)
kept the API even though the rest of favicon went?
Overall:
wonderful simplification, many #if chains removed. Loved the
consolidation of the image stuff and simplification of content
handling.
I think is ok to merge
--
Regards Vincent
http://www.kyllikki.org/
