On Friday 27 July 2007 16:50:01 Darin Adler wrote: > I'm not happy with the code organization here. XMLTokenizer now has > tons of ifdefs and two separate implementations. It's fine to have a > QXmlStream implementation, but the two implementations should be in > separate files, side by side, as we do in the platform layer.
I agree that the #ifdef's are not optimal, and we can move parts of the code out. But currently a lot of code is still shared between the QXmlStream and the libxml based implementations, so we have two choices: 1. just add an XMLTokenizerQt.cpp and duplicate lots of code. 2. add a qt/XMLTokenizerQt.cpp and a libxml/XMLTokenizerLibXml.cpp and keep a common XMLTokenizer.cpp for code that is used in both. 3. Have some ifdef's in the one file. The first option is no good, as we'd have to duplicate lot's of shared code leading to maintenance issues in the long term. The second option is better, but also here we'd have quite a bit more duplicated code than required as long as we don't do some bigger refactoring to abstract out the common parts. This is something we wanted to avoid at the moment. So we went for option 3. > Or the XML parser should be abstracted out so we have a single > XMLTokenizer built on a single parser abstraction. That will be IMO almost impossible to do in an efficient way as the two parsers work very differently. > I think this is also the type of patch that should be reviewed by at > least one person other than the folks concentrating on the Qt port. We've had other people look at most patches. The QXmlStream changes are required, and I think the only issue to discuss here could be separating implementations. This is something I'd like to do in the longer term, but as you're trying to stabilize currently I didn't feel that this should have been done right now. > With the huge number of check-ins today, I'm sure there are other > interesting things like this that I missed. I think we did a thorough job reviewing our patches and testing them on all platforms (Mac, Windows, gtk and Qt). We also kept most changes rather small and atomic to make reviewing easier. Lars _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
