Thanks for the second review, jmb!
>
> These all want a '_' at the start.
>
I am sorry, but I really get a little confused about which kind of
functions should start with a "_", which kind not...
For example, the _dom_string_intern should start with '_', but the
dom_document_alloc should not. Is there any rules for this?
>> +/* Get the internal lwc_string */
>> +dom_exception dom_string_get_intern(struct dom_string *str,
>> + struct lwc_context_s **ctx, struct lwc_string_s **lwcstr);
>> +/* Map the lwc_error to dom_exception */
>> +dom_exception dom_exception_from_lwc_error(lwc_error err);
>> +
>
> Does this really belong here?
>
All our API return a dom_string to our client, if the client (such as
the libcss) want to extract the lwc_string inside, this two functions
are necessary.
>> +
>> +/* TODO: Make the "id" definition temporarily here, in future, it should
>> + * be obtained from the language bindings.
>> + */
>> +#define ID_ATTRIBUTE "id"
>
> I'd like to see this fixed before merge.
>
So, let the parser binding to set a string of ID to the document. I
mean, to provide a function like:
dom_document_set_id_string(dom_document *doc, dom_string *id);
and the parser binding call this after it has completed parsing the
doc and know what is the ID string is.
How do you think about this?
>> +static unsigned int hash_lwcstring(void *key);
>
> _dom_element? Alternatively, move this to the hashtable implementation.
>
Changed to _dom_element_hash_*.
>> +bool attributes_equal(void *p1, void *p2)
>> +{
>> + return p1 == p2;
>
> Is this comparing lwc_strings? If so, it should use
> lwc_context_string_isequal(ctx, p1, p2); It would appear that you've no
> way of retrieving the internment context here. That seems like a bug.
I am afraid, no. The two pointers are two dom_element.
>> +#define DOM_EF_PROTECT_VTABLE \
>> + _dom_ef_destroy, \
>> + _dom_ef_alloc, \
>> + _dom_ef_copy
>
> What is "EF"?
I mean entity_reference, but sorry for the typo, I have changed this
to DOM_ER_PROTECT_VTABLE as well as all ef to er. :)
>> static struct dom_string *__nodenames_utf8[DOM_NODE_TYPE_COUNT + 1];
>
> Is this actually needed any more?
Yes, it is. The create_text_node/create_comment and many other
function need the strings inside it.
>
> The rest looks fine (excepting the comments I've stopped pointing out).
>
I just delete all the empty comment, just leave the /*-------*/ as the
code section separator, thanks!
Regards!
Bo
