Bug 76820

Summary: [Qt] Replace QtXmlPatterns usage with libxslt dependency
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: PlatformAssignee: Andras Piroska <pandras>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, kbalazs, laszlo.gombos, loki, ossy, pandras, tonikitoo, vestbo, zherczeg
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76773    
Attachments:
Description Flags
Replace QtXmlPatterns usage with libxslt dependency
hausmann: review-
Replace-QtXmlPatterns
none
Replace-QtXmlPatterns
none
Replace QtXmlPatterns usage with libxslt dependency none

Description Simon Hausmann 2012-01-23 04:07:36 PST
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
Comment 1 Andras Piroska 2012-02-01 01:34:34 PST
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.
Comment 2 Andras Piroska 2012-02-01 03:39:55 PST
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.
Comment 3 Simon Hausmann 2012-02-01 06:18:32 PST
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.
Comment 4 Andras Piroska 2012-02-02 03:25:56 PST
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.
Comment 5 Laszlo Gombos 2012-02-08 20:02:53 PST
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.
Comment 6 Andras Piroska 2012-02-22 06:02:11 PST
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.
Comment 7 Simon Hausmann 2012-02-22 11:52:49 PST
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?
Comment 8 Andras Piroska 2012-02-23 02:27:24 PST
Created attachment 128442 [details]
Replace-QtXmlPatterns

I think make this config-test same as the existing configtest (fontconfig),
so I removed the "!mac" condition.
Comment 9 Andras Piroska 2012-03-13 05:03:29 PDT
Created attachment 131592 [details]
Replace QtXmlPatterns usage with libxslt dependency
Comment 10 Balazs Kelemen 2012-03-27 09:29:49 PDT
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.
Comment 11 Eric Seidel (no email) 2012-03-27 10:30:10 PDT
Yay!  Hopefully this means eventual further simplification of our XML parsing.  (By removing QXMLParser.) :)
Comment 12 Simon Hausmann 2012-03-29 07:32:01 PDT
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)
Comment 13 Simon Hausmann 2012-04-01 23:10:03 PDT
(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.
Comment 14 Balazs Kelemen 2012-04-16 04:47:19 PDT
Landed in r114240 - with my cleanups :).
Comment 15 Csaba Osztrogonác 2012-04-16 05:55:46 PDT
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.
Comment 16 Csaba Osztrogonác 2012-04-16 05:58:30 PDT
I skipped them until fix - http://trac.webkit.org/changeset/114245
Comment 17 Simon Hausmann 2012-06-02 00:24:32 PDT
(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? :)
Comment 18 Csaba Osztrogonác 2012-06-02 08:10:55 PDT
I'm on it.
Comment 19 Csaba Osztrogonác 2012-06-02 09:58:49 PDT
(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.
Comment 20 Simon Hausmann 2012-06-02 12:06:03 PDT
Thanks a lot Ossy!