Bug 28303 – [Qt] XSLT support with QtXmlPatterns

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Jakub Wieczorek]

Since the 4.5 release, QtXmlPatterns has supported XSLT 2.0 so it seems to be a good idea for QtWebKit to use that rather than libxslt (which in fact doesn't build now with the Qt port).

http://doc.trolltech.com/4.6-snapshot/qtxmlpatterns.html#xslt-2-0
Here is an overview of how much complete the XSLT 2.0 support in QtXmlPatterns is. It's lacking some important features but making QtWebKit use that should help improve it.

There is some libxml2/libxslt specific code laying around, not only in the implementations of XSLTProcessor and XSLStyleSheet but also in e.g. Document. For the former, we should provide our own separate implementations, for the latter we would need some #ifdefs.

The other files/classes the Qt port doesn't need at all:
- XSLImportRule: QtXmlPatterns currently doesn't support xsl:import/xsl:include but even when it does, it will most likely import documents internally (of course we would need to hook that so that we can take care of security and caching), so the XSLImportRule class we don't need.
- XSLTExtensions - this is completely a libxslt specific thing, we can't add extra functionality to the QtXmlPatterns module so we don't need it either.
- XSLTUnicodeSort - as above, we can't extend QXmlQuery with our own sorting functionality. If we wanted to add missing bits to the xsl:import implementation, we would have to patch QtXmlPatterns.

The Qtish implementation of XSLStyleSheet would be pretty basic, we don't support child stylesheets and pre-compiling stylesheets so really there is not much what XSLStyleSheet could do apart from actually holding a stylesheet source.

Currently I managed to achieve a result of ~35% passing layout tests. The rest can be skipped with notes about why they fail so they can be investigated in QtXmlPatterns. I'll post the exact results when submitting the patches.

[Jakub Wieczorek]

Created attachment 34848 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

[Jakub Wieczorek]

Created attachment 34849 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

This one actually fixes the whitespace errors.

[Jakub Wieczorek]

Created attachment 34852 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

[Jakub Wieczorek]

Created attachment 34854 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

[Jakub Wieczorek]

Created attachment 34855 [details]
[Qt] Add expected result for fast/xsl/document-function.xml.

[Jakub Wieczorek]

Created attachment 34858 [details]
[Qt] Update expected result for fast/xsl/xslt_unicode.xml.

[Jakub Wieczorek]

Created attachment 34860 [details]
Remove the encoding attribute from xsl:stylesheet element in
resources/unicode.xsl.

This one will have no effect on other ports, it is needed to make Qtish XSLT processor happy: currently it stops evaluating the stylesheet after encountering the misplaced attribute.

[Jakub Wieczorek]

Created attachment 34861 [details]
[Qt] Update the Skipped list.

The numbers:

13 test cases (37%) succeeded
19 test cases (54%) had incorrect layout
2 test cases (5%) were new
1 test case (2%) crashed

We're passing 13 test cases within the fast/xsl directory and 5 more outside it. They all have been unskipped and the rest skipped with notes about why they're failing.

[Eric Seidel (no email)]

Comment on attachment 34849 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

This looks like an OK first step.  Thank you for breaking this into pieces.

[Eric Seidel (no email)]

Comment on attachment 34855 [details]
[Qt] Add expected result for fast/xsl/document-function.xml.

This seems unrelated, but looks fine.  the commit-queue can't handle this due to dependancies on other patches.

[Eric Seidel (no email)]

Comment on attachment 34858 [details]
[Qt] Update expected result for fast/xsl/xslt_unicode.xml.

Also unrelated to the rest.  ChangeLog is malformed, so will need manual landing.

[Eric Seidel (no email)]

Comment on attachment 34860 [details]
Remove the encoding attribute from xsl:stylesheet element in
resources/unicode.xsl.

Why?  Do we need a test to make sure that we fail with the encoding attribute?  If so, this needs a test.  We don't really want some versions of WebKit to behave differently  with XSLT content.

[Eric Seidel (no email)]

Comment on attachment 34849 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Rejecting patch 34849 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34849 from bug 28303 failed to download and apply.

[Eric Seidel (no email)]

Comment on attachment 34861 [details]
[Qt] Update the Skipped list.

Looks OK.  Depends on others, so cq-

[Eric Seidel (no email)]

Comment on attachment 34854 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

Seems we could have a better abstraction here:
#if ENABLE(XSLT)
 310 #if ENABLE(XSLT) && !USE(QXMLQUERY)
311311     , m_transformSource(0)
312312 #endif

Style.  Likewise, we need a better abstraction:
 #if USE(QXMLQUERY)
 3757 void Document::setTransformSource(const String &source)
 3758 {
 3759     m_transformSource = source;
 3760 }
 3761 #else

If that means having a TransformSource class which is opaque to document, that's fine.

Abstraction/cleanup needed:
 #if ENABLE(XSLT)
 183 #if ENABLE(XSLT) && !USE(QXMLQUERY)
184184 void* xmlDocPtrForString(DocLoader*, const String& source, const String& url);
185185 #endif

Yeah, basically all of the:
 44 #if ENABLE(XSLT) && !USE(QXMLQUERY)
look wrong.  Or at least they could be done better with just a little bit of abstraction work.

[Eric Seidel (no email)]

cq failure:
Hunk #5 FAILED at 111.
1 out of 8 hunks FAILED -- saving rejects to file WebCore/xml/XSLTProcessor.cpp.rej
patch -p0 "WebCore/xml/XSLTProcessor.cpp" returned 1.  Pass --force to ignore patch failures.
Logging in as [e-mail]...

Again, generally we try to avoid so many patches on a bug, as the comment threads get confusing. :)

[Jakub Wieczorek]

Created attachment 35024 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Rebased against ToT. It should apply cleanly now.

[Eric Seidel (no email)]

Comment on attachment 35024 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Looks OK.

[Eric Seidel (no email)]

Comment on attachment 35024 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Rejecting patch 35024 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=35024 from bug 28303 failed to download and apply.

[Jakub Wieczorek]

Created attachment 35027 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Sorry for that, whitespace differences got messed up when editing the patch by hand. This one should hopefully be fine.

Also noticed that I'm missing an update to the xcode project file. Is there any guideline on how to update that file by hand?

[Jakub Wieczorek]

(In reply to comment #12)
> (From update of attachment 34860 [details])
> Why?  Do we need a test to make sure that we fail with the encoding attribute? 
> If so, this needs a test.  We don't really want some versions of WebKit to
> behave differently  with XSLT content.

Well, for now QtWebKit _will_ behave differently comparing to libxslt-based equivalents, as QtXmlPatterns' processor is quite strict, doesn't have XSLT 1.0 compatibility mode yet and still lacks some features. So while some sites may not work properly in QtWebKit, it should help us track down the relevant issues and get them fixed upstream.

On the other hand you're right, this layout test can be left as is.

[Eric Seidel (no email)]

I am super-entertained that you found that 6-year-expired address of mine. :)

[Jakub Wieczorek]

Created attachment 35058 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Removed some redundant #includes.

[Jakub Wieczorek]

Created attachment 35060 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

Fixed one typo in GNUMakefile.am.

[Jakub Wieczorek]

Created attachment 35061 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

I got rid of most of the #ifdefs:
- Qt doesn't use the XMLTokenizerScope class so I excluded it from the Qt build.
- I made a TransformSource typedef in Document, which for Qt is String and for others xmlDocPtr. This simplified the code a bit but we still need to free m_transformSource for libxslt.

Not sure what else I could do. I was thinking of moving the definition of XSLTProcessor::MessageHandler to the XSLTProcessorQt.cpp file but was not sure if that is okay with the coding guidelines.

[Jakub Wieczorek]

Created attachment 35107 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

[Jakub Wieczorek]

Created attachment 35108 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

Removed an accidental hunk.

[Eric Seidel (no email)]

Comment on attachment 35107 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Looks OK.

You don't generally want to fill in the requestee field.  That just tends to make reviews go slower. ;)

[Eric Seidel (no email)]

Comment on attachment 35108 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

You missed the point.  I meant making TransformSource a real class with two separate implementations.  Not just a typedef.  That way all the logic can be kept in the qt vs. libxml files instead of in Document.

[Eric Seidel (no email)]

(My last comment was not intended to sound as rude as it may have.)

[Eric Seidel (no email)]

Comment on attachment 35060 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

It seems that we will need an XSLStyleSheet.h file anyway.  There is code to be shared here.

-bool XSLStyleSheet::isLoading()
certainly should be shared.

As should:
-void XSLStyleSheet::checkLoaded()

Possibly:
-void XSLStyleSheet::clearDocuments()

So before you add hte qt version, I think we need to add back an XSLStyleSheet.h and .cpp file and move a bunch of this code into a shared baseclass.

The ChangeLog diff is bad, so this will have to be applied manually.  I don't trust svn-apply to get this right.

[Jakub Wieczorek]

(In reply to comment #31)
> (From update of attachment 35060 [details])
> It seems that we will need an XSLStyleSheet.h file anyway.  There is code to be
> shared here.
> 
> -bool XSLStyleSheet::isLoading()
> certainly should be shared.
> 
> As should:
> -void XSLStyleSheet::checkLoaded()
> 
> Possibly:
> -void XSLStyleSheet::clearDocuments()

Unfortunately none of these can be shared. Qtish XSLTProcessor doesn't make use of the XSLImportRule class as it does not support xsl:import and xsl:include. At some point it will but even then the whole logic would most likely take place in QXmlQuery (with necessary caching and security bits in WebKit).

[Eric Seidel (no email)]

Why would Qt want to invent their own XSLT processor?  And why would a qt-based webkit want to use a less-functioning XSLT processor?  I take it qt webkit doesn't currently build with any xslt support?

[Eric Seidel (no email)]

Comment on attachment 35107 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Rejecting patch 35107 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Jakub Wieczorek]

(In reply to comment #33)
> Why would Qt want to invent their own XSLT processor?  And why would a qt-based
> webkit want to use a less-functioning XSLT processor?  I take it qt webkit
> doesn't currently build with any xslt support?

Yes, QtWebKit currently does not build with the libxslt based implementation.

I agree that our XSLT processor is currently less-functioning but the main reason behind using it in QtWebKit is to change that. :) When it's utilised in QtWebKit, we will be getting more user input and thus can get things fixed upstream sooner.

Another point is that libxslt is just another dependency for Qt (if it was to ship QtWebKit with libxslt enabled by default). :) That is also why Qt has its own XMLTokenizer implementation using QXmlStreamReader rather than libxml2, I guess.

[Jakub Wieczorek]

Created attachment 39198 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

This one should hopefully build fine on mac. I checked the libxslt-based build with the gtk port and updated the xcode project.

[Jakub Wieczorek]

Created attachment 39207 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

Updated the xcode project file.

[Jakub Wieczorek]

Created attachment 39384 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

- Add the TransformSource class.
- Fix build with Qt < 4.5 (disable xslt by default for older versions).

[Kenneth Rohde Christiansen]

(In reply to comment #38)
> Created an attachment (id=39384) [details]
> [Qt] Implement XSLT support with QtXmlPatterns.
> 
> - Add the TransformSource class.
> - Fix build with Qt < 4.5 (disable xslt by default for older versions).

Hi Jakub, as far as I know QtXmlPatterns module is only build when Qt is built with exception support. How does this patch affect QtWebKit inside Qt, when Qt is compiled without exception support?

[Jakub Wieczorek]

Created attachment 39427 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

(In reply to comment #39)
> (In reply to comment #38)
> > Created an attachment (id=39384) [details] [details]
> > [Qt] Implement XSLT support with QtXmlPatterns.
> > 
> > - Add the TransformSource class.
> > - Fix build with Qt < 4.5 (disable xslt by default for older versions).
> 
> Hi Jakub, as far as I know QtXmlPatterns module is only build when Qt is built
> with exception support. How does this patch affect QtWebKit inside Qt, when Qt
> is compiled without exception support?

Indeed, the patch assumed that Qt has been built with the QtXmlPatterns module.
This one fixes that by enabling XSLT support only with Qt >= 4.5 _and_ when QtXmlPatterns is available.

Apart from that, fixed libxslt-based build breakage and some compiler warnings.

[Eric Seidel (no email)]

Can we do this in small pieces on individual bugs?  Trying to do 6 patches at once just tends to overwhelm the process...

[Jakub Wieczorek]

Created attachment 39887 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

According to the suggestions on IRC, I prepared an svn version of the patch so that it will properly preserve the file history when landing in the repository.

[Jakub Wieczorek]

Created attachment 39888 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

A patch generated on an svn checkout.

[Jakub Wieczorek]

(In reply to comment #41)
> Can we do this in small pieces on individual bugs?  Trying to do 6 patches at
> once just tends to overwhelm the process...

Ok, fine. I'll submit the unrelated patches (the Skipped list update and the test results) in seperate bugs once the main patches land, especially that the Skipped patch does no longer apply against ToT and one of the results has outdated metrics.

The rest should be easy to go through in this bug, as two of them have already been r+'ed, they just didn't compile on mac. I fixed them afterwards and Tor Arne checked that they are okay now.

[Simon Hausmann]

Comment on attachment 39888 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

r=me, but when landing I think the XSLStyleSheet.h change should not be landed. Even though the current coding guidlines on the web say it should be indented, the recent discussion on webkit-dev indicates a change of this policy in the near future.

[Simon Hausmann]

Comment on attachment 39887 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

r=me, but see also my comment earlier regarding indentation (affects XSLTProcessor.h hunk).

[Simon Hausmann]

Comment on attachment 39427 [details]
[Qt] Implement XSLT support with QtXmlPatterns.


From what I can see this is very good, except for a tiny TransformSource buglet:

> -void Document::setTransformSource(void* doc)
> +void Document::setTransformSource(TransformSource* source)
>  {
> -    if (doc == m_transformSource)
> -        return;
> -
> -    xmlFreeDoc((xmlDocPtr)m_transformSource);
> -    m_transformSource = doc;
> +    m_transformSource = source;
>  }

Technically this function was protected against self-assignment and isn't anymore. One could
argue it's a bug in OwnPtr perhaps?

> +    void setTransformSource(TransformSource*);
> +    TransformSource* transformSource() const { return m_transformSource.get(); }
>  #endif

I believe the setter should take a PassOwnPtr<TransformSource>, to make it safe and clear that ownership is passed here.

[Simon Hausmann]

Comment on attachment 39887 [details]
Move the libxslt specific part of XSLTProcessor to a separate file.

Clearing review after landing http://trac.webkit.org/changeset/48812 and http://trac.webkit.org/changeset/48814 (oops :)

[Simon Hausmann]

Comment on attachment 39888 [details]
Rename XSLStyleSheet.cpp to XSLStyleSheetLibxslt.cpp.

Clearing review after landing http://trac.webkit.org/changeset/48815

[Jakub Wieczorek]

Created attachment 40244 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

(In reply to comment #47)
> (From update of attachment 39427 [details])
> 
> From what I can see this is very good, except for a tiny TransformSource
> buglet:
> 
> > -void Document::setTransformSource(void* doc)
> > +void Document::setTransformSource(TransformSource* source)
> >  {
> > -    if (doc == m_transformSource)
> > -        return;
> > -
> > -    xmlFreeDoc((xmlDocPtr)m_transformSource);
> > -    m_transformSource = doc;
> > +    m_transformSource = source;
> >  }
> 
> Technically this function was protected against self-assignment and isn't
> anymore. One could
> argue it's a bug in OwnPtr perhaps?
> 
> > +    void setTransformSource(TransformSource*);
> > +    TransformSource* transformSource() const { return m_transformSource.get(); }
> >  #endif
> 
> I believe the setter should take a PassOwnPtr<TransformSource>, to make it safe
> and clear that ownership is passed here.

Done.

[Simon Hausmann]

Comment on attachment 40244 [details]
[Qt] Implement XSLT support with QtXmlPatterns.

r=me

[Simon Hausmann]

Committed r48826: <http://trac.webkit.org/changeset/48826>

[Tor Arne Vestbø]

*** Bug 29882 has been marked as a duplicate of this bug. ***