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

[Maciej Stachowiak]

On Jul 27, 2007, at 11:36 AM, Lars Knoll wrote:

> 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.

Other organizations have requested the ability to use other XML  
parsers as well, such as expat. Seems like in the long run we want a  
different approach than just ifdefs in the XMLTokenizer.cpp file. It  
seems like the best would be some abstraction layer on top of the  
parser library, but if that is difficult then your option #2 sounds  
like a docent long-run approach. I would have expected just about  
every XML parsing library to have a SAX-like API, which shouldn't be  
too hard to abstract, but perhaps QXml works differently.

Regards,
Maciej


>
>
>> 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 at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev