Bug 76820

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.

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

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.

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.

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

Created attachment 128442 [details] Replace-QtXmlPatterns I think make this config-test same as the existing configtest (fontconfig), so I removed the "!mac" condition.

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

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.

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

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)

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

Landed in r114240 - with my cleanups :).

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.

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

(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? :)

I'm on it.

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

Thanks a lot Ossy!