[PATCH v2 2/5] d2d1: Partially implement RegisterEffectFromStream().
Ziqing Hui
zhui at codeweavers.com
Mon Jun 13 05:47:51 CDT 2022
On 6/13/22 3:38 PM, Nikolay Sivov wrote:
>
>
> On 6/13/22 09:46, Ziqing Hui wrote:
>>
>> On 6/12/22 4:30 PM, Nikolay Sivov wrote:
>>>
>>> On 6/12/22 08:31, Ziqing Hui wrote:
>>>> + /* Loop inside effect node */
>>>> + end_node_found = FALSE;
>>>> + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK)
>>>> + {
>>>> + if (node_type == XmlNodeType_Element)
>>>> + {
>>>> + if (!wcscmp(node_name, L"Property"))
>>>> + hr = parse_property(xml_reader, reg);
>>>> + else if (!wcscmp(node_name, L"Inputs"))
>>>> + hr = parse_inputs(xml_reader, reg);
>>>> + else
>>>> + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
>>>> +
>>>> + if (FAILED(hr))
>>>> + goto done;
>>>> + }
>>>> + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect"))
>>>> + {
>>>> + end_node_found = TRUE;
>>>> + break;
>>>> + }
>>>> + }
>>>> + hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
>>> I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
>>>
>>> Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
>>>
>> I'm thinking of failure case. The tests in the PATCH 1 show that if we meet element that are not expected, the function will return a error code (mostly HRESULT_FROM_WIN32(ERROR_NOT_FOUND)).
>> So I think we should not skip elements we don't interested in, we should return error for them.
>>
>> Checking EndElement name is for the same reason.
>>
>> Nikolay, what do you think?
>>
> What I mean is that xmllite will check that for you, if closing tag does not match last open tag it will be a parser error. My suggestion was to make helpers like parse_property() consume entire <Property>...</Property> element, including end node, so the only possibility for EndElement at top level would be </Effect> one.
>
> Regarding errors, does that mean the order of elements is important too? E.g. can you have some property elements before Inputs and some after, or it's stricter than that? I don't think we should care too much about that, we can introduce additional checks later, but for now it's fine to handle that as a general xml document.
According to my test, order of elements is not important.
OK, I think we can ignore these error check now and focus on the important part.
More information about the wine-devel
mailing list