[webkit-dev] Re: [webkit-changes] [24723] trunk/WebCore

[Lars Knoll]

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