https://bugs.winehq.org/show_bug.cgi?id=7372
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian@fds-team.de
--- Comment #62 from Sebastian Lackner sebastian@fds-team.de --- Hi,
I looked at this patch, and the basic idea seems correct. Nevertheless while reviewing I noticed some issues:
try 2 (with libxml):
* When copying part of a table or enumeration, then the HTML snippet will look like "<tr><td>...</td></tr>". This confuses libxml, and no <HTML><BODY> tag will be added. The code will later error out with a message that no body tag was found. Not sure if its caused by my specific version of libxml, but it doesn't seem to be safe to assume, that libxml will do the whole job.
try 3 (manual adding tags):
* In the example above only a <HTML><BODY> tag is added, but no <table> tag. To create meaningful HTML the code should also add other tags, where it is possible to guess in which context they are used.
* The code to detect "startOfMarkup" is wrong. It is of course also possible that the HTML code begins with text, not with an HTML tag. In this situation the tag will be inserted at the wrong position. I have also no idea what the memchr(...) check is for?!
* The code for startOfMarkup is executed even when its not required. Since this code is relatively slow a better method is to skip over it, when all header fields are already present.
* It looks a bit unnecessary complicated that the whole memory block is copied several times. First of all one time for the UTF-16 decoding, then another time for adding the tags, and a third time to add the headers. I am not sure yet if there is an easy method, but we should probably try to get rid of at least one copy step.
* The <BODY> tag is searched, although the position is already known in some cases. By merging both functions its probably possible to get rid of that.
I have fixed the most critical issue (incorrect calculation of startOfMarkup) and added a fixed version of the patch to our staging tree.
https://github.com/wine-compholio/wine-staging/blob/master/patches/winex11-C...
@Damjan: Not sure if you're still working on it, but if you want some feedback on your next attempt before sending it on the mailing lists just contact me. ;)
Regards, Sebastian