[webkit-dev] Re: [webkit-changes]  trunk/WebCore
lars at trolltech.com
Fri Jul 27 11:36:28 PDT 2007
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.
More information about the webkit-dev