On Fri, 2009-07-31 at 12:15 +0800, Bo Yang wrote:
> 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?
Yes. If it's public API, then it begins dom_. If it's library-internal
API, then it begins _dom_.
> >> +/* 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.
Sure. I was just questioning whether the mapping from lwc_error to
dom_exception needs to be public at all. Is there any situation in which
libdom returns a lwc_error? If not, then I don't think this should be
public. If it does, why?
> >> +
> >> +/* 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?
Sounds fine.
> >> +static unsigned int hash_lwcstring(void *key);
> >
> > _dom_element? Alternatively, move this to the hashtable implementation.
> >
>
> Changed to _dom_element_hash_*.
Ok.
> >> +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.
Ok. Please document this, it's not clear.
> >> +#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. :)
Ta.
> >> 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.
They use the interned strings in the document, right? Why do we have
this table at all, then?
J.
