[2/2] msxml3: Support xmlns[:*] attribute nodes intelligently.

[Ulrik Dickow]

Den 12-06-2012 13:21, Nikolay Sivov skrev:
> On 6/12/2012 11:55, Ulrik Dickow wrote:
>> Patch 2 of 2 from http://bugs.winehq.org/show_bug.cgi?id=26226 .
>>
>>
>>
> 
>> +    /* Emit appropriate fixme or warning in case of unsupported or invalid namespace.
>> +     * The W3C spec would like us to exit earlier for attempts to redefine the namespace for
>> +     * the reserved "xmlns" attribute or prefix (http://www.w3.org/TR/xml-names/#ns-decl),
>> +     * like http://gdome2.cs.unibo.it/gtk-doc/gdome2-gdomeelement.html#GDOME-EL-SETATTRIBUTENS,
>> +     * but since native msxml3 accepts anything here, it's safest to just continue with warning.
>> +     */
> The question is more like what libxml2 thinks about that. If it follows
> w3c here which is likely the question will be - won't it break somewhere
> internally in libxml2 code if we hack it that way?

I don't think so.  We have already seen that libxml2 doesn't consider
the attribute "xmlns" to have any special meaning in relation to
namespaces, when setting an attribute.  It hasn't given any problems in
my tests.

But maybe you are right that it is safer to return E_INVALIDARG, as I
did in an earlier version, to guard against future increased
intelligence in libxml2.  Both of the applications in bug 26226 use the
correct W3C namespace, so they don't care.  I'll change it back.

>> +    if(namespaceURI && namespaceURI[0] && node_type != NODE_ELEMENT)
>> +    {
>> +        if(node_type == NODE_ATTRIBUTE)
>> +            switch((!strcmpW(name, xmlnsW) ||
>> +                    !strncmpW(name, xmlnscW, sizeof(xmlnscW)/sizeof(WCHAR))) +
>> +                   !strcmpW(namespaceURI, w3xmlns))
> Could you please simplify that, it's a bit hard to read.

I guess you mean I simplified it too much.  But yes, it can be hard to
read/understand.  I'll offer an expanded version using logical variables
and maybe several if-then-else instead of this overly simple switch.

BTW, as a funny side-note, at first I had added a "default:" clause with
a FIXME (or similar) stating that this should be impossible.  'gcc -O2'
was intelligent enough to know that for logical variables a and b in C,

	a + b

can only be 0 (both are false), 1 (exactly one is true) or 2 (both are
true), so it completely optimized away the FIXME-string/statement from
the binary.

>> +     * Note: We would be more in line with the native msxml if we only detected a
>> +     * redundancy/conflict for prefixes existing on the element itself (in element->ns
>> +     * or in the linked list element->nsDef).  But as long as our *_get_xml() doesn't
>> +     * eliminate redundant namespace definitions in children, this behaviour may be
>> +     * an advantage in some cases.  As disadvantage is that we incorrectly regard the
>> +     * redefinition in element 'b' in '<a xmlns="x"><b xmlns="y"/></a>' as a conflict.
>> +     * Libxml2 currently doesn't have a function or option to only search for namespaces
>> +     * on the current node; but it would be easy to make a reduced version of xmlSearchNs
>> +     * (currently defined in http://git.gnome.org/browse/libxml2/tree/tree.c#n5871).
>> +     * However, most apps probably set the ns to y on node b creation, so no conflict here.
>> +     *
> I don't think it's better to potentially break one thing while fixing
> the other.

In current wine, attributes named "xmlns" don't work at all in
setAttributeNode (an error code is returned instead of S_OK, and no
attribute is added, no matter whether a namespace is already present or
not at this level).

So fixing this issue doesn't break anything that worked before.  I
consider it a step-wise improvement.  Something better, nothing worse.

I wanted to keep that patch as small and simple as possible.
setAttribute also has this "dumb" use of xmlSearchNs.

> And I guess I like a thought on making xmlSearchNs version
> that will do what we want from it.

Ok, actually it'll only be about 10 lines of extra code, roughly, so
I'll add that -- and shrink the comment, less worries, more code :).

> Just to clarify - is that right that libxml2 problem here is a lack of some checks
> that lead to duplication and conflicts later? (that are visible in doc
> dumps too)

Well, yes, in a way: it doesn't check whether we try to set attributes
named "xmlns" or use prefix "xmlns" or use the namespaceURI
"http://www.w3.org/2000/xmlns/".  But maybe it shouldn't.  It's a matter
of policy -- giving higher layers the freedom to enforce restrictions
themselves.

It turns out that the native msxml3 doesn't enforce restrictions.  As
the doc dumps show, it too can give

  1) duplication when setAttribute is used (but not when
setAttributeNode is used)

  2) conflicts between the namespaces in visible XML (get_xml) versus
what get_namespaceURI tells us, both for setAttribute and setAttributeNode

The most problematic lack in libxml2 is not about treating (input)
attributes specially, but rather the whole concept of treating the ns
fields as "directives to print an xmlns attribute".  It doesn't map well
with the native msxml concept of treating namespaces as just information
that this element _belongs_ to this namespace.  Whether or not an xmlns
attribute should be printed (returned from get_xml), depends on what
part of the tree the user wants to look at.  If the user asks for XML
for a parent, a child in the same namespace doesn't have the xmlns
attribute repeated at its level (due to ns fields).

It would be nice to have XML-generation functions in libxml2 that
suppressed child xmlns output similarly.  Fortunately most applications
probably don't care about this extra XML (BridgeCentral doesn't).

>> +/* Convenient helper function for creating an attribute node in a given namespace.
>> + * Note: CHECK_HR() should not be here, but in caller, to give optimal line number info.
>> + */
> You mean EXPECT_HR here I guess.

Yup, sorry.

>> +static HRESULT create_attribute_ns(
>> +    IXMLDOMDocument *doc,
>> +    const char *attr_name,
>> +    const char *nsURI,
>> +    IXMLDOMAttribute **attr_ptr)
>> +{
>> +    HRESULT hr;
>> +    VARIANT var;
>> +    IXMLDOMNode *node;
>> +
>> +    V_VT(&var) = VT_I1;
>> +    V_I1(&var) = NODE_ATTRIBUTE;
>> +
>> +    hr = IXMLDOMDocument_createNode(doc, var, _bstr_(attr_name), _bstr_(nsURI), &node);
>> +    if (hr == S_OK)
>> +    {
>> +        IXMLDOMNode_QueryInterface(node, &IID_IXMLDOMAttribute, (void**) attr_ptr);
>> +        IXMLDOMNode_Release(node);
>> +    }
>> +    return hr;
>> +}
> Please null out pointer in case of failure, or better always before
> createNode().

Ok.