Bug 76820 – [Qt] Replace QtXmlPatterns usage with libxslt dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Simon Hausmann]

We should use libxslt for XSLT support if it's available. If it is the case, then we should also use libxml for XML parsing.
If libxslt is not available, we should disable XSLT support and fall back to Qt's built-in XML parser (QXmlStreamReader) for XML parsing.

See also https://lists.webkit.org/pipermail/webkit-qt/2011-November/002166.html

[Andras Piroska]

I found some references to Qt specific XML/XSLT files (XSLStyleSheetQt.cpp,etc.) in Target.gypi and WebCore.gypi project files. In my opinion we should not remove these code files, only change the default build mode to use libxml, libxslt libraries.

[Andras Piroska]

Created attachment 124923 [details]
Replace QtXmlPatterns usage with libxslt dependency

The Target.pri and WebCore.pri files was modified, in order to use libxml and libxslt by default.
The Qt's XML parser haven't been remove, it still used in the .gypi build files.

[Simon Hausmann]

Comment on attachment 124923 [details]
Replace QtXmlPatterns usage with libxslt dependency

View in context: https://bugs.webkit.org/attachment.cgi?id=124923&action=review

This doesn't seem enough though. I see fallbacking handling missing in this patch, i.e. disabling xslt support if libxslt is not present in the system. We should be using a Qt configure test for that.

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +        The Target.pri and WebCore.pri files was modified, in order to use libxml and libxslt by default.

Usually there's an empty line between "Reviewed by" and the body of the change log.

[Andras Piroska]

Oh sorry, I misunderstood my task, I tried remove the xslt support from the source code instead of
make a switch that check (before the build) the libxslt support is available or not.

[Laszlo Gombos]

Similar change is available here - http://gitorious.org/+mob-team/webkit/mob-team-qtwebkit2/commit/a165ea69a25b7c162f3da9dca16f2855902c5cf7. It seems that you will need to clone the repo to be able to see the change.

[Andras Piroska]

Created attachment 128192 [details]
Replace-QtXmlPatterns

Replace QtXmlPatterns with libxslt and/or libxml.
This patch works with config-test to determine the availability of these libs.

[Simon Hausmann]

Comment on attachment 128192 [details]
Replace-QtXmlPatterns

View in context: https://bugs.webkit.org/attachment.cgi?id=128192&action=review

The tests are looking good. I need to give this a spin myself :).

A small question below.

> Tools/qmake/mkspecs/features/features.prf:57
> +haveQt(5):contains(config_test_libxml2, yes):!mac {

Why is this disabled for the mac?

[Andras Piroska]

Created attachment 128442 [details]
Replace-QtXmlPatterns

I think make this config-test same as the existing configtest (fontconfig),
so I removed the "!mac" condition.

[Andras Piroska]

Created attachment 131592 [details]
Replace QtXmlPatterns usage with libxslt dependency

[Balazs Kelemen]

Comment on attachment 131592 [details]
Replace QtXmlPatterns usage with libxslt dependency

View in context: https://bugs.webkit.org/attachment.cgi?id=131592&action=review

Informal review.
The patch seems sane to me.

> Tools/qmake/mkspecs/features/features.prf:57
> +# We need libxml2 config test to determine to use libxml2 or not

It's evident, so you should better remove the comment.

> Tools/qmake/mkspecs/features/features.prf:64
> +haveQt(5):contains(config_test_libxml2, yes) {
> +    DEFINES += WTF_USE_LIBXML2=1
> +    DEFINES -= WTF_USE_LIBXML2=0
> +} else {
> +    DEFINES += WTF_USE_LIBXML2=0
> +    DEFINES -= WTF_USE_LIBXML2=1
> +}

IMHO you should not change these flags if they are not set explicitly. It means, you should also check this condition:
"!contains(WTF_USE_LIBXML2=.)". You can also remove the -= lines.
Furthermore, you should not bother with setting it to 0, it's the same as when it is not defined.

> Tools/qmake/mkspecs/features/features.prf:74
> +# We need libxslt config test to determine to use libxslt or not
> +haveQt(5):contains(config_test_libxslt, yes) {
> +    DEFINES += ENABLE_XSLT=1
> +    DEFINES -= ENABLE_XSLT=0
> +} else {
> +    DEFINES += ENABLE_XSLT=0
> +    DEFINES -= ENABLE_XSLT=1
> +}
> +

Ditto and ditto.

[Eric Seidel (no email)]

Yay!  Hopefully this means eventual further simplification of our XML parsing.  (By removing QXMLParser.) :)

[Simon Hausmann]

Comment on attachment 131592 [details]
Replace QtXmlPatterns usage with libxslt dependency

I agree with Balazs' comments, this could be cleaned up a bit before landing. But in principle this patch is correct I believe :) (sorry, took me a while to check the conditions we have all there, in WebCore/Target.pri and wtf/Platform.h)

[Simon Hausmann]

(In reply to comment #11)
> Yay!  Hopefully this means eventual further simplification of our XML parsing.  (By removing QXMLParser.) :)

That might become an option in the long term, if the libxml dependency is not too much of an issue for people using Qt. In the short term one thing that we can definitely remove is the XsltProcessorQt code.

[Balazs Kelemen]

Landed in r114240 - with my cleanups :).

[Csaba Osztrogonác]

Reopen, because it made 35 tests fail on Qt5:
http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20-%20Qt5-WebKit1/r114242%20%286155%29/results.html

It needs more investigation to check them.

[Csaba Osztrogonác]

I skipped them until fix - http://trac.webkit.org/changeset/114245

[Simon Hausmann]

(In reply to comment #15)
> Reopen, because it made 35 tests fail on Qt5:
> http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20-%20Qt5-WebKit1/r114242%20%286155%29/results.html
> 
> It needs more investigation to check them.

The diff suggest that our expected results are wrong.

---- platform/qt/fast/xsl/xslt-recursion-expected.txt:
CONSOLE MESSAGE: line 1: <html xmlns='http://www.w3.org/1999/xhtml/'><body><p>Running an XSL-T 1.0 stylesheet with a 2.0 processor.</p></body></html>
Success!
----

---- fast/xsl/xslt-recursion-expected.txt:
Success!
----

and http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20-%20Qt5-WebKit1/r114242%20%286155%29/fast/xsl/xslt-recursion-diff.txt shows that we now produce the latter instead of the former.

This looks like a really simple cleanup (getting rid of our platform specific results and unskipping the test). Volunteers? :)

[Csaba Osztrogonác]

I'm on it.

[Csaba Osztrogonác]

(In reply to comment #17)
> The diff suggest that our expected results are wrong.
...
> This looks like a really simple cleanup (getting rid of our platform specific results and unskipping the test). Volunteers? :)

You're right, former expected files were wrong (== adjusted to QtXmlPatterns).
I moved them to qt-4.8, because 4.8 still uses QtXmlPatterns. And Qt 5 is happy
with platform independent expected files. We only need Qt specific expected file for 2 tests because of additional console messages. (http/tests/security/xss-DENIED-xsl-document-redirect-expected.txt and http/tests/security/xss-DENIED-xsl-external-entity-redirect-expected.txt)

And ther http/tests/xmlviewer/dumpAsText/mathml.xml still fails, I filed a new bug report for it - https://bugs.webkit.org/show_bug.cgi?id=88169

Cleanup patch landed in r119323.

[Simon Hausmann]

Thanks a lot Ossy!