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.