On Wed, 2009-08-12 at 15:34 +0100, John-Mark Bell wrote:
> Index: include/dom/events/event.h
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ include/dom/events/event.h 2009-08-12 15:12:46.000000000 +0100
> @@ -0,0 +1,93 @@
> +/*
> + * This file is part of libdom.
> + * Licensed under the MIT License,
> + * http://www.opensource.org/licenses/mit-license.php
> + * Copyright 2009 Bo Yang <[email protected]>
> + */
> +
> +#ifndef dom_events_event_h_
> +#define dom_events_event_h_
> +
> +#include <stdbool.h>
> +#include <dom/core/exceptions.h>
> +#include <dom/events/event_target.h>
> +
> +struct dom_string;
> +
> +typedef enum {
> + DOM_CAPTURING_PHASE = 1,
> + DOM_AT_TARGET = 2,
> + DOM_BUBBLING_PHASE = 3
> +} dom_event_flow_phase;
Whitespace: use spaces here, not tabs.
> Index: src/events/mouse_event.h
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/mouse_event.h 2009-08-12 15:12:50.000000000 +0100
> +/**
> + * The mouse event
> + */
> +struct dom_mouse_event {
> + struct dom_ui_event base; /**< Base class */
> +
> + long sx; /**< ScreenX */
> + long sy; /**< ScreenY */
> + long cx; /**< ClientX */
> + long cy; /**< ClientY */
> +
> + bool ctrl; /**< Whether the Control key is down */
> + bool meta; /**< Whether the Meta key is down */
> + bool shift; /**< Whether the shift key is down */
> + bool alt; /**< Whether the Alt key is down */
> +
> + bool alt_graph; /**< Whether AltGraph key is down */
> + bool caps_lock; /**< Whether CapsLock key is down */
> + bool num_lock; /**< Whether NumLock key is down */
> + bool scroll; /**< Whether Scroll key is down */
The above would be better as:
enum {
MOUSE_MOD_CTRL = (1<<0),
MOUSE_MOD_META = (1<<1),
MOUSE_MOD_SHIFT = (1<<2),
/* etc */
};
uint32_t modifier_state;
> Index: src/events/mutation_name_event.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/mutation_name_event.c 2009-08-12 15:12:50.000000000 +0100
> @@ -0,0 +1,157 @@
> +/*
> + * This file is part of libdom.
> + * Licensed under the MIT License,
> + * http://www.opensource.org/licenses/mit-license.php
> + * Copyright 2009 Bo Yang <[email protected]>
> + */
> +
> +#include "events/mutation_name_event.h"
> +#include "core/document.h"
> +
> +#include "utils/utils.h"
> +
> +static struct dom_event_private_vtable _event_vtable = {
> + __dom_mutation_name_event_destory
> +};
"destroy".
> Index: src/events/mutation_event.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/mutation_event.c 2009-08-12 15:12:50.000000000 +0100
> @@ -0,0 +1,224 @@
> +/*
> + * This file is part of libdom.
> + * Licensed under the MIT License,
> + * http://www.opensource.org/licenses/mit-license.php
> + * Copyright 2009 Bo Yang <[email protected]>
> + */
> +
> +#include "events/mutation_event.h"
> +#include "core/document.h"
> +
> +static struct dom_event_private_vtable _event_vtable = {
> + __dom_mutation_event_destory
> +};
Ditto. I'll not highlight any more of these.
Note also that identifiers beginning __ are reserved in C, so this needs
to be changed.
> Index: src/events/keyboard_event.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/keyboard_event.c 2009-08-12 15:12:50.000000000 +0100
> +/**
> + * Query the state of a modifier using a key identifier
> + *
> + * \param evt The event object
> + * \param ml The modifier identifier, such as "Alt", "Control", "Meta",
> + * "AltGraph", "CapsLock", "NumLock", "Scroll", "Shift".
> + * \param state Whether the modifier key is pressed
> + * \return DOM_NO_ERR on success, appropriate dom_exception on failure.
> + *
> + * @note: If an application wishes to distinguish between right and left
> + * modifiers, this information could be deduced using keyboard events and
> + * KeyboardEvent.keyLocation.
> + */
> +dom_exception _dom_keyboard_event_get_modifier_state(dom_keyboard_event *evt,
> + struct dom_string *m, bool *state)
> +{
> + if (m == NULL) {
> + *state = false;
> + return DOM_NO_ERR;
> + }
> +
> + const char *data = _dom_string_data(m);
> + size_t len = _dom_string_length(m);
> +
> + if (strncmp(data, "AltGraph", len) == 0) {
> + *state = evt->alt_graph;
> + }
The point of interning strings is that you don't need to call strncmp to
compare them.
The subsequent comparisons should be else if, so that early termination
occurs.
> +/**
> + * Parse the modifier list string to corresponding bool variable state
> + *
> + * \param modifier_list The modifier list string
> + * \param alt_graph Whether AltGraph is in the modifier list
> + * \param alt Whether Alt is in the modifier list
> + * \param caps_lock Whether CapsLock is in the modifier list
> + * \param ctrl Whether Control is in the modifier list
> + * \param meta Whether Meta is in the modifier list
> + * \param num_lock Whether NumLock is in the modifier list
> + * \param scroll Whether Scroll is in the modifier list
> + * \param shift Whether Shift is in the modifier list
> + * \return DOM_NO_ERR on success, appropriate dom_exception on failure.
> + */
> +dom_exception _dom_parse_modifier_list(struct dom_string *modifier_list,
> + bool *alt_graph, bool *alt, bool *caps_lock, bool *ctrl,
> + bool *meta, bool *num_lock, bool *scroll, bool *shift)
> +{
> + *alt_graph = false;
> + *alt = false;
> + *caps_lock = false;
> + *ctrl = false;
> + *meta = false;
> + *num_lock = false;
> + *scroll = false;
> + *shift = false;
> +
> + if (modifier_list == NULL)
> + return DOM_NO_ERR;
> +
> + char *data = _dom_string_data(modifier_list);
> + char *m = NULL;
> +
> + m = strtok(data, " ");
I'd rather not see strtok used, ever. It's simple enough to do this kind
of tokenisation manually.
> + while (m != NULL) {
> + size_t len = strlen(m);
This is redundant if you do it manually; just keep a pointer to the
start of the current token and subtract it from the current pointer to
retrieve the token length.
> + if (strncmp(m, "AltGraph", len) == 0) {
> + *alt_graph = true;
> + }
It's more efficient to compare the lengths first.
> + if (strncmp(m, "Alt", len) == 0) {
> + *alt = true;
> + }
These should be else if.
> Index: src/events/keyboard_event.h
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/keyboard_event.h 2009-08-12 15:12:50.000000000 +0100
> +/**
> + * The keyboard event
> + */
> +struct dom_keyboard_event {
> + struct dom_ui_event base; /**< The base class */
> +
> + struct dom_string *key_ident; /**< The identifier of the key in this
> + * event, please refer:
> + *
> http://www.w3.org/TR/DOM-Level-3-Events/keyset.html#KeySet-Set
> + * for detail
> + */
> +
> + dom_key_location key_loc; /**< Indicate the location of the key on
> + * the keyboard
> + */
> +
> + bool ctrl; /**< Whether the Control key is down */
> + bool meta; /**< Whether the Meta key is down */
> + bool shift; /**< Whether the Shift key is down */
> + bool alt; /**< Whether the Alt key is down */
> +
> + bool alt_graph; /**< Whether AltGraph key is down */
> + bool caps_lock; /**< Whether CapsLock key is down */
> + bool num_lock; /**< Whether NumLock key is down */
> + bool scroll; /**< Whether Scroll key is down */
These, too, can be a bitfield.
> Index: src/events/event.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/event.c 2009-08-12 15:12:50.000000000 +0100
> +/**
> + * Claim a reference on this event object
> + *
> + * \param evt The Event object
> + */
> +void _dom_event_ref(dom_event *evt)
> +{
> + evt->refcnt ++;
> +}
Lose the space.
> +/**
> + * Release a reference on this event object
> + *
> + * \param evt The Event object
> + */
> +void _dom_event_unref(dom_event *evt)
> +{
> + if (evt->refcnt > 0) {
> + evt->refcnt --;
> + return ;
> + }
> + dom_event_destroy(evt);
> +}
This is wrong. The object should be destroyed when the reference count
reaches zero, not -1.
> Index: src/events/event_listener.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/event_listener.c 2009-08-12 15:12:50.000000000 +0100
> +/**
> + * Claim a new reference on the listener object
> + *
> + * \param listener The EventListener object
> + */
> +void dom_event_listener_ref(dom_event_listener *listener)
> +{
> + listener->refcnt ++;
> +}
Whitespace.
> +/**
> + * Release a reference on the listener object
> + *
> + * \param listener The EventListener object
> + */
> +void dom_event_listener_unref(dom_event_listener *listener)
> +{
> + if (listener->refcnt > 0) {
> + listener->refcnt --;
Ditto.
> + return;
> + }
> +
> + _dom_document_alloc(listener->doc, listener,
> + sizeof(dom_event_listener));
> +}
Should destroy when refcnt reaches zero.
> Index: src/events/event_target.c
> ===================================================================
> --- /dev/null 2009-04-16 19:17:07.000000000 +0100
> +++ src/events/event_target.c 2009-08-12 15:12:50.000000000 +0100
> +
> +/* Hash key/value functions */
> +static void *_key(void *key, void *key_pw, dom_alloc alloc, void *pw,
> + bool clone);
> +static void *_value(void *value, void *value_pw, dom_alloc alloc,
> + void *pw, bool clone);
> +static unsigned int _dom_element_hash_lwcstring(void *key);
This needs renaming. More importantly, can it be put somewhere central
-- in src/utils, perhaps? I'd like to avoid duplicating this kind of
functionality.
> +/**
> + * Add an EventListener to the EventTarget
> + *
> + * \param et The EventTarget object
> + * \param type The event type which this event listener listens for
> + * \param listener The event listener object
> + * \param capture Whether add this listener in the capturing phase
> + * \return DOM_NO_ERR on success, appropriate dom_exception on failure.
> + */
> +dom_exception _dom_event_target_add_event_listener(dom_event_target *et,
> + struct dom_string *type, struct dom_event_listener *listener,
> + bool capture)
> +{
> + struct listener_entry *le = NULL;
> + struct dom_document *doc = dom_node_get_owner(et);
> + if (doc == NULL) {
> + /* TODO: In the progress of parsing, many Nodes in the DTD
> + * has no document at all, do nothing for this kind of node */
> + return DOM_NO_ERR;
> + }
This seems odd. Can you explain why it's needed in more detail?
> +/**
> + * Dispatch a DOMNodeInserted/DOMNodeRemoved event
> + *
> + * \param doc The document object
> + * \param et The EventTarget object
> + * \param type "DOMNodeInserted" or "DOMNodeRemoved"
> + * \param related The parent of the removed/inserted node
> + * \param success Whether this event's default action get called
> + * \return DOM_NO_ERR on success, appropriate dom_exception on failure.
> + */
> +dom_exception _dom_dispatch_node_change_event(struct dom_document *doc,
> + dom_event_target *et, dom_event_target *related,
> + dom_mutation_type change, bool *success)
> +{
> + struct dom_mutation_event *evt;
> + dom_exception err;
> +
> + err = _dom_mutation_event_create(doc, &evt);
> + if (err != DOM_NO_ERR)
> + return err;
> +
> + lwc_string *type = NULL;
> + if (change == DOM_MUTATION_ADDITION) {
> + err = _dom_document_create_lwcstring(doc,
> + (const uint8_t *) "DOMNodeInserted",
> + SLEN("DOMNodeInserted"), &type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> + } else if (change == DOM_MUTATION_REMOVAL) {
> + err = _dom_document_create_lwcstring(doc,
> + (const uint8_t *) "DOMNodeRemoval",
> + SLEN("DOMNodeRemoved"), &type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> + } else {
> + assert("Should never be here" == NULL);
> + }
> +
> + dom_string *t = NULL;
> + err = _dom_document_create_string_from_lwcstring(doc, type, &t);
> + _dom_document_unref_lwcstring(doc, type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> +
> + /* Initiliase the event with corresponding parameters */
> + err = dom_mutation_event_init(evt, t, true, false, related, NULL, NULL,
> + NULL, change);
> + dom_string_unref(t);
> + if (err != DOM_NO_ERR) {
> + goto cleanup;
> + }
> +
> + err = dom_event_target_dispatch_event(et, evt, success);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> +
> + /* Finalise the evt, and reuse it */
> + _dom_mutation_event_finalise(doc, evt);
> + /* Dispatch the DOMNodeInsertedIntoDocument/DOMNodeRemovedFromDocument
> + * event */
> + if (change == DOM_MUTATION_ADDITION) {
> + err = _dom_document_create_lwcstring(doc,
> + (const uint8_t *)
> + "DOMNodeInsertedIntoDocument",
> + SLEN("DOMNodeInsertedIntoDocument"), &type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> + } else if (change == DOM_MUTATION_REMOVAL) {
> + err = _dom_document_create_lwcstring(doc,
> + (const uint8_t *) "DOMNodeRemovalFromDocument",
> + SLEN("DOMNodeRemovedFromDocument"), &type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> + } else {
> + assert("Should never be here" == NULL);
> + }
> +
> + err = _dom_document_create_string_from_lwcstring(doc, type, &t);
> + _dom_document_unref_lwcstring(doc, type);
> + if (err != DOM_NO_ERR)
> + goto cleanup;
> +
> + dom_event_target *target = et;
> + while (target != NULL) {
> + err = dom_mutation_event_init(evt, t, true, false, NULL,
> + NULL, NULL, NULL, change);
> + dom_string_unref(t);
You're unreffing 't' for every iteration of this loop. That seems wrong.
> +/**
> + * Query the state of a modifier using a key identifier
> + *
> + * \param evt The event object
> + * \param ml The modifier identifier, such as "Alt", "Control", "Meta",
> + * "AltGraph", "CapsLock", "NumLock", "Scroll", "Shift".
> + * \param state Whether the modifier key is pressed
> + * \return DOM_NO_ERR on success, appropriate dom_exception on failure.
> + *
> + * @note: If an application wishes to distinguish between right and left
> + * modifiers, this information could be deduced using keyboard events and
> + * KeyboardEvent.keyLocation.
> + */
> +dom_exception _dom_mouse_event_get_modifier_state(dom_mouse_event *evt,
> + struct dom_string *m, bool *state)
> +{
> + if (m == NULL) {
> + *state = false;
> + return DOM_NO_ERR;
> + }
> +
> + const char *data = _dom_string_data(m);
> + size_t len = _dom_string_length(m);
> +
> + if (strncmp(data, "AltGraph", len) == 0) {
> + *state = evt->alt_graph;
> + }
Same commentary applies here as before.
> Index: src/utils/namespace.c
> ===================================================================
> --- src/utils/namespace.c (revision 9213)
> +++ src/utils/namespace.c (working copy)
> @@ -151,13 +151,6 @@
>
> if (colon == (uint32_t) -1) {
> /* No prefix */
> - /* If namespace URI is for xmlns, ensure qname == "xmlns" */
> - if (namespace != NULL &&
> - dom_string_cmp(namespace,
> - dom_namespaces[DOM_NAMESPACE_XMLNS]) == 0 &&
> - dom_string_cmp(qname, xmlns) != 0) {
> - return DOM_NAMESPACE_ERR;
> - }
> /* If qname == "xmlns", ensure namespace URI is for xmlns */
> if (namespace != NULL &&
> dom_string_cmp(qname, xmlns) == 0 &&
It turns out that I got the merge wrong here. Ignore this change.
> Index: src/core/string.c
> ===================================================================
> --- src/core/string.c (revision 9213)
> +++ src/core/string.c (working copy)
> @@ -56,7 +56,8 @@
> */
> void dom_string_ref(struct dom_string *str)
> {
> - str->refcnt++;
> + if (str != NULL)
> + str->refcnt++;
> }
Surely it's a bug if str is ever NULL here?
> Index: src/core/characterdata.c
> ===================================================================
> --- src/core/characterdata.c (revision 9213)
> +++ src/core/characterdata.c (working copy)
> @@ -10,11 +10,13 @@
>
> #include <dom/core/characterdata.h>
> #include <dom/core/string.h>
> +#include <dom/events/events.h>
>
> #include "core/characterdata.h"
> #include "core/document.h"
> #include "core/node.h"
> #include "utils/utils.h"
> +#include "events/mutation_event.h"
>
> /* The virtual functions for dom_characterdata, we make this vtable
> * public to each child class */
> @@ -128,6 +130,11 @@
> return DOM_NO_MODIFICATION_ALLOWED_ERR;
> }
>
> + /* Dispatch a DOMCharacterDataModified event */
> + struct dom_document *doc = dom_node_get_owner(cdata);
> + bool success = true;
> + _dom_dispatch_characterdata_modified_event(doc, c, c->value, data,
> &success);
> +
Do we care if the dispatch failed? If so, how should it be handled?
> if (c->value != NULL) {
> dom_string_unref(c->value);
> }
> @@ -135,6 +142,9 @@
> dom_string_ref(data);
> c->value = data;
>
> + success = true;
> + _dom_dispatch_subtree_modified_event(doc, c->parent, &success);
> +
Ditto (and, indeed, everywhere else).
> Index: src/core/node.c
> ===================================================================
> --- src/core/node.c (revision 9213)
> +++ src/core/node.c (working copy)
> @@ -351,7 +359,8 @@
> */
> void _dom_node_ref(dom_node_internal *node)
> {
> - node->refcnt++;
> + if (node != NULL)
> + node->refcnt++;
> }
Similarly, how can node be NULL here?
>
> @@ -1007,10 +1016,6 @@
> _dom_node_detach(new_child);
> }
>
> - /* When a Node is attached, it should be removed from the pending
> - * list */
> - dom_node_remove_pending(new_child);
> -
Where does it get removed from the pending list now?
Other than the above, this looks fine. Please ensure it's wrapped to 80
columns -- there's still a bunch of cases where it isn't.
J.
