Hi jmb,
Thanks for the review. I have fixed most of the problems, and
following are my new comments.
>> +
>> + 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.
There are 8 strings together, so I think this don't need a lwc style
compare and it is efficient if I add the len compare before the real
string compare.
>
> The subsequent comparisons should be else if, so that early termination
> occurs.
Done.
>
>> +/**
>> + * 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.
I have changed this. :)
>
>> + if (strncmp(m, "AltGraph", len) == 0) {
>> + *alt_graph = true;
>> + }
>
> It's more efficient to compare the lengths first.
Done.
>> +{
>> + 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.
Fixed and all unref fixed.
>> --- /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.
I have move the _dom_element_hash_lwcstring to utils/hash_table.c. And
the _key and _value functions must be here and each hash table needs
itself's _key and _value, so they can not be shared. The code insides
these two functions are different between different hash tables. :)
>> + 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?
I have removed this comment, and changed it to a "assert(doc !=
NULL);", I can't remember why such a comment and code are here. :)
>> + 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.
Fixed.
>> --- 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.
These are some changes I commit to the turnk/dom yestoday, I don't
understand why they are here.
>> 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?
For the NULL pointer problems, both for string and node. I think it is
better to test a whether a Node/String is NULL in the ref/unref
function, it should not be treated as an error when a NULL passed into
these two functions.
If we don't test the NULL inside these functions, we should write many
code like:
if (p != NULL)
dom_node_(un)ref(p);
If something can't be NULL, we should test it in the right place
instead of making a (un)ref function crashed.
>> + _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?
>
I have added some error handling code in element.c. But, I think we
don't need to do more. There are all three exceptions can be
generated:
UNSPECIFIED_EVENT_TYPE_ERR
DISPATCH_REQUEST_ERR
NOT_SUPPORTED_ERR
For internal generated events, none of these three exceptions can be
raised. So, i think we can assume the _dispatch_ function all return
DOM_NO_ERR.
>> @@ -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?
This is a change from the trunk/dom, I don't understand why it is in
this review. For the problem, when some node is _dom_node_detach, it
is put into the pending_list, so we should remove it from the
pending_list before we attach it.
>
> 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.
Sorry, I forget to check the src/core directory, fix it now. Thanks!
Regards!
Bo
