Bug 13807 – XML without style should render as syntax-highlighted source

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

XML with no style information (and no HTML/SVG) should render the source tree instead with syntax highlighting.  WinIE and Firefox do something like this.

[Dave Hyatt]

Created attachment 14647 [details]
Patch that uses view source mode.

This is an experiment.  I don't think it's necessarily the way to go.  XBL seems like it would be a better technology for this, since the original DOM could be preserved more readily.

[Ian 'Hixie' Hickson]

If we implement this we have to be careful to avoid doing it for any document that matches any of the following conditions:

 - Has an <?xml-stylesheet?> PI
 - Has XHTML anywhere in the document (including attributes)
 - Has SVG anywhere in the document (including attributes)
 - Has an HTTP Link header (to link in a stylesheet)
 - Has XLink anywhere in the document
 - Was created by script
 - Is modified by script
 - Contains an element or pseudo-element that has style applied to it by any
   rule in the UA stylesheets

If an XML document has no author stylesheet [1], no author script [2], and 
contains no elements or attributes in namespaces given in the UA style sheets, then we should display it as if someone had asked for the document's source instead of laying it out per CSS.

My recommendation would be that once the document is shown, we listen for mutation events to (a) update the source view and (b) switch out of the source view if any of the conditions listed above stop applying (e.g., a stylesheet PI is added to the document).

[1] There are various ways to add author stylesheets: style attributes in 
various namespaces, style elements in various namespaces, various different 
link elements in various namespaces, HTTP headers, presentational attributes 
on elements in certain namespaces, etc. I don't know how to test for this 
properly. For example, one way to add styles is to dynamically bind the root
element to an XBL binding that links to a CSS stylesheet. We have to be able
to cope with this case correctly.

[2] Similarly, scripts can be added in many different ways. The XBL case is 
possible here too, but that's not the only way. Adding event handlers via the
DOM is another possibility. Some elements and attributes in various namespaces
add script to the document, etc.

XML test cases:
   http://www.hixie.ch/tests/evil/xml/001.xml
   http://www.hixie.ch/tests/evil/xml/002.xml
   http://www.hixie.ch/tests/evil/xml/003.xml
   http://www.hixie.ch/tests/evil/xml/004.xml
   http://www.hixie.ch/tests/evil/xml/005.xml
   http://www.hixie.ch/tests/evil/xml/006.xml
   http://www.hixie.ch/tests/evil/xml/007.xml
   http://www.hixie.ch/tests/evil/xml/008.xml
   http://www.hixie.ch/tests/evil/xml/009.xml
   http://www.hixie.ch/tests/evil/xml/010.xml
   http://www.hixie.ch/tests/evil/xml/011.xml
   http://www.hixie.ch/tests/adhoc/xml/styling/

HTML test cases:
   http://www.hixie.ch/tests/evil/xml/001.html
   http://www.hixie.ch/tests/evil/xml/004.html

[Dave Hyatt]

Yeah this patch was just to get my feet wet with the idea.   I don't think I like the "transform" approach, since XBL would be a much better way of doing this.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Blake Sening]

This is being tracked in Chromium as
http://code.google.com/p/chromium/issues/detail?id=434

[David Kilzer (:ddkilzer)]

Resetting to default assignee.

[David Kilzer (:ddkilzer)]

<rdar://problem/3006226>

[Vsevolod Vlasov]

Created attachment 80435 [details]
Patch

[Vsevolod Vlasov]

I implemented this XML viewer with the following approach:

1) Rendering is done using XSLT stylesheet with the following features
  - Formatted xml output
  - Syntax highlighting (uses the same view-source.css as View Source Mode for colors)
  - Expand/collapse buttons on complex tags (similar to Firefox approach), expand all, collapse all buttons.

2) XML viewer is not shown when one of the following conditions is met:
  - Current frame has parent frame defined (since parent frame can execute scripts on current frame).
  - Current window has opener defined (since opener window can execute scripts on current frame).
    The side effect of this condition is that XML viewer is not shown for XML 
    documents opened with <a href="www.example.com/no-style.xml" target="_blank">Anchor text</a>.
  - Current document has any XML parsing errors.
  - Current document has <?xml-stylesheet ...?> processing instruction.
  - Current document has any element or attribute in one of the 'known' namespaces:
    - XHTML
    - SVG (if it is enabled)
    - WML (if it is enabled)
    - MATHML (if it is enabled)
    - XLINK
    - XUL

3) Layout Tests provided to test when and how XML viewer is shown. 
   Since I could not use javascript in most of the tests (they should not have xhtml namespace intentionally),
   I put them in http folder/tests/xmlviewer folder with .htaccess to serve all of them with application/xml mime type.
   At the same time I changed DumpRenderTree to always dump application/xml as text.
   That doesn't cause any problems with older tests (by default .xml files are served as text/xml)

My implementation seems to pass all of the Hixie's tests that were passed before patch.

So far I made DumpRenderTree changes only for chromium. WML and MathML are not working in chromium build (at least in default), so these tests do not have '*-expected.txt' files yet.

As a result of my changes these tests do not pass anymore
  fast/css/xml-stylesheet-pi-not-in-prolog.xml = IMAGE+TEXT =============== Has PI not in prolog
  fast/encoding/utf-16-no-bom.xml = IMAGE+TEXT  =========================== XSLT applied, strange file, strange symbols
  fast/invalid/junk-data.xml = IMAGE+TEXT  ================================ XML WITH ERROR -> fails after XSLT (only '>' in xml)
  svg/hixie/error/004.xml = IMAGE+TEXT ==================================== SVG WITHOUT NAMESPACES
  svg/hixie/error/005.xml = IMAGE+TEXT ==================================== SVG WITHOUT NAMESPACES
They are testing the behaviour with various incorrect xml files, so they are failing after applying XMLViewer.xsl transformation.

Could you give me any comments on this implementation?

[Alexey Proskuryakov]

I probably won't be the reviewing this patch, but I'm curious. Could you attach a screenshot of how the end result looks?

FWIW, my understanding is that we're now much closer to having something like XBL than in 2007.

[Vsevolod Vlasov]

Created attachment 80455 [details]
XML viewer screenshot

Here is a simple example

[Ismail Donmez]

(In reply to comment #12)
> Created an attachment (id=80455) [details]
> XML viewer screenshot
> 
> Here is a simple example

Looks very nice!

[Eric Seidel (no email)]

Comment on attachment 80435 [details]
Patch

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

> Source/WebCore/dom/ProcessingInstruction.cpp:63
> +: ContainerNode(document)
> +, m_target(target)
> +, m_data(data)
> +, m_sheetStr(sheetStr)
> +, m_cachedSheet(0)
> +, m_loading(false)
> +, m_alternate(false)
> +, m_createdByParser(false)
> +#if ENABLE(XSLT)
> +, m_isXSL(false)

WE indent these one level (4spaces)

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1501
> +    int exception = 0;

ExceptionCode?

> Source/WebCore/xml/XMLViewer.xsl:33
> +<xsl:stylesheet version='1.0' xmlns:xsl='http://www.w3.org/1999/XSL/Transform'>

XSL transforms are dog-slow.

[Eric Seidel (no email)]

I would like to see perf numbers from one of the PerformanceTests/Parser which show that this does not slow down load of XML pages (which have style).  Maybe I'm being paranoid.

[Alexey Proskuryakov]

Someone else will need to decide if we should accept an XSL-based implementation at all. I just have some nitpicks about the looks.
1) This is not localization friendly - the header has several English strings that are embedded in a huge stylesheet.
2) I'm not quite sure what [Expand All] and [Collapse All] are good for, but they should be actual buttons, not Lynx-style links.
3) I don't see any reason to use a monospace font.

[Maciej Stachowiak]

Comment on attachment 80435 [details]
Patch

Now that we have a syntax highlighted tree view in the Web Inspector, what is the point of having a special one only for the case of unstyled XML? At the very least this should reuse the Web Inspector's DOM viewer code instead of being implemented in a completely different way.

[Vsevolod Vlasov]

(In reply to comment #17)
> (From update of attachment 80435 [details])
> Now that we have a syntax highlighted tree view in the Web Inspector, what is the point of having a special one only for the case of unstyled XML? At the very least this should reuse the Web Inspector's DOM viewer code instead of being implemented in a completely different way.

This patch was developed in close collaboration with Pavel Feldman. Our intention is to provide users with error highlighting which makes this tree view quite different from the Web Inspector's one. 

Current approach is to render XML to the first error and report this first error in the header.
There are three cases:

1. XML without XSLT but with some other style information (e.g. CSS or SVG)
 - document is rendered up to the first error with styles applied
 - we do not currently intend to change this behavior

2. XML with XSLT
 - if there is any error in XML (or XSLT) then nothing is rendered at all (white screen!)
 - we intend to show source XML in this case and highlight errors

3. XML without any style information
 - document is rendered (text nodes only) up to the first error
 - we intend to show source XML in this case and highlight errors

Reusing Web Inspector's logic is also unreasonably complex because it is designed to work in another process and uses platform specific code. Pavel can elaborate on that if needed.
For consistency we can use the same styles as much as possible.

[Alexey Proskuryakov]

It seems counter-intuitive to add advanced debugging functionality (like highlighting errors) outside of Web Inspector.

[Vsevolod Vlasov]

(In reply to comment #19)
> It seems counter-intuitive to add advanced debugging functionality (like highlighting errors) outside of Web Inspector.

It is not about adding this functionality because errors are already reported when rendering XML, it is more about improving it.

[Vsevolod Vlasov]

(In reply to comment #16)
> Someone else will need to decide if we should accept an XSL-based implementation at all. I just have some nitpicks about the looks.
> 1) This is not localization friendly - the header has several English strings that are embedded in a huge stylesheet.
> 2) I'm not quite sure what [Expand All] and [Collapse All] are good for, but they should be actual buttons, not Lynx-style links.
> 3) I don't see any reason to use a monospace font.

1,2 - agreed
3 - monospace font is used for consistency with View Source and Web Inspector

[Ismail Donmez]

(In reply to comment #19)
> It seems counter-intuitive to add advanced debugging functionality (like highlighting errors) outside of Web Inspector.

This is a very useful functionality (which also IE implements), unless it slows down code it should be provided.

[Alexey Proskuryakov]

Just to make it clear, I also want unstyled XML to show as tree in document window, at least when loaded from a local file. I'm not sure if there is a practical use case to display remote context as tree outside of Web Inspector, but maybe that should be done nonetheless for consistency.

[Vsevolod Vlasov]

Created attachment 82640 [details]
Patch

Here is a new patch with everything discussed taken into account.

1) Developer Extras Flag:

As discussed XML viewer is shown only when developer extras flag is set.

2) Reusing treeoutline.js:

Implementing XML viewer using treeoutline.js is unreasonably complex.
We would need to pass text nodes content in the javascript methods, so we would need to escape these texts.
This escaping for javascript in XSLT stylesheet would be not only complex but error-prone.
Instead of that I reused viewsource.css styles and made xml viewer look similar to inspector by reusing arrow images.

3) Performance tests:

This patch does not slow down PerformanceTests/parser/xml-parser.htnl tests mentioned above.
In fact all checks are done using boolean fields set during parsing, so there is no any time-consuming tasks.

4) Localization:

Currently errors in XML / XSLT are reported without localization.
Thus this patch does not make it worse. 
The message itself is passed to XSLT as a parameter, so these could be easily localized together as a next step.

5) Hixie's tests:

Some of the Hixie's tests are not passed:

- XUL and XLink attributes in XML document are not processed currently, so their presence does not disable xml viewer.
   http://www.hixie.ch/tests/adhoc/xml/styling/004.xml - XUL
   http://www.hixie.ch/tests/evil/xml/009.xml - XLink

- Styles passed in HTTP headers are not processed in XML document currently, so their presence does not disable xml viewer.
   http://www.hixie.ch/tests/evil/xml/006.xml

- MathML test is passed when MathML is enabled
   http://localhost/xml/hixie/styling/003.xml

- http://www.hixie.ch/tests/adhoc/xml/styling/pi/
   These tests give the same results with and without this change.

6) Layout Tests

Several layout tests are introduced to test XML viewer layout and behavior on XML documents having style information.

To force dumping XML documents without style information as text DumpRenderTree is changed to dump as text all tests having "dumpAsText/" in their path. This change is currently made only in Chromium, Mac and GTK ports.
Also resetting of frame opener is added in resetTestController-like functions to avoid side effects of running some naigation tests together with XML viewer ones. This change is currently made for Chromium only.

Some older layout tests are not passed now since they check behavior on some XML documents without style. Their expected results are still to be changed.

[Vsevolod Vlasov]

Created attachment 82642 [details]
XML viewer screenshot

[Alexey Proskuryakov]

Comment on attachment 82640 [details]
Patch

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

I have a number of nitpicks, and some design concerns for which I don't have better suggestions from the top of my head. Would be good to address those if possible though.

> LayoutTests/http/tests/xmlviewer/dumpAsText/css-stylesheet.xml:8
>  \ No newline at end of file

Please add a newline.

> LayoutTests/http/tests/xmlviewer/dumpAsText/resources/css-stylesheet.css:2
>  \ No newline at end of file

Please add a newline.

> LayoutTests/http/tests/xmlviewer/dumpAsText/resources/frames-helper.xml:6
>  \ No newline at end of file

Please add a newline.

> LayoutTests/http/tests/xmlviewer/dumpAsText/xmlviewer.xml:40
> +                And now some long text in root element.

Do we have any test coverage for non-ASCII content?

> Source/WebCore/DerivedSources.make:651
> +all : XMLViewerXSL.h XMLViewerDownArrowPNG.h XMLViewerRightArrowPNG.h

I guess I just don't know how we usually do it, but generating source files for resources seems strange. Generally speaking, all images should come from resources for localization (it's fairly unlikely that these particular images need to be localized, but one can never be sure... how about reversing arrow direction for RTL languages? or changing color for cultures that have different connotations for colors?)

> Source/WebCore/GNUmakefile.am:3630
> +	Source/WebCore/xml/XMLViewerHelper.cpp \
> +	Source/WebCore/xml/XMLViewerHelper.h \

"Helper" doesn't mean anything. Pavel told me that this class is encapsulating logic related to the feature - it really needs a speaking name.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25133
> +<<<<<<< HEAD
>  				B8DBDB4B130B0F8A00F5CDB1 /* SetSelectionCommand.cpp in Sources */,
>  				B8DBDB4D130B0F8A00F5CDB1 /* SpellingCorrectionCommand.cpp in Sources */,
> +=======
> +				5905ADBF1302F3CE00F116DF /* XMLViewerHelper.cpp in Sources */,
> +>>>>>>> XML without style should render as syntax-highlighted source.

Bad merge.

> Source/WebCore/dom/Document.cpp:411
> +    , m_hasElementsInKnownNamespaces(false)

This member variable is quite misleading. For one thing, it doesn't track element removal. And the concept of "having" an element is not formal.

This is part of a heuristic for deciding when to display the document as tree, and ideally, it shouldn't be even tracked inside Document itself. Maybe you need to keep it inside Document for performance, as calling into another class wouldn't be great.

I also have concerns about long term validity of this heuristic. How would we do incremental rendering of XML if there is a check that depends on all elements in the document? Even if we never implement incremental rendering, it would be good to have common tree view behavior with browsers that do.

> Source/WebCore/dom/ProcessingInstruction.h:94
> +    bool m_isCSS;

Now that you are adding this data member, you should update the rest of the code to use it. For example, ProcessingInstruction::setCSSStyleSheet() should ASSERT(m_isCSS).

Might make sense to do that in a separate patch before this one.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1336
> +    bool xmlViewerMode = !m_sawError && !m_sawCSS && !m_sawXSLTransform && xmlViewerHelper.hasNoStyleInformation();

As mentioned above, we should consider the possibility of expressing the heuristic in a way that works with incremental parsing (not necessarily coding it that way).

> Source/WebCore/xml/XMLViewer.xsl:19
> +   - THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS

This is a yet different license header.

> Source/WebCore/xml/XMLViewerHelper.cpp:17
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

Please use a correct license header - "Apple Computer, Inc." and "Apple" are both wrong names to use. Official project license is available at <http://www.webkit.org/coding/bsd-license.html>.

> Source/WebCore/xml/XMLViewerHelper.cpp:99
> +    processor->setParameter("", "xml_has_no_style_message", "This XML file does not have any style information. The document tree is shown.");

Is "is shown" proper English? Again, you should really ask for a localizable string, so that could be vetted by language experts in each company that ships WebKit.

> Source/WebCore/xml/XMLViewerHelper.h:17
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

License header again.

> Source/WebCore/xml/XMLViewerHelper.h:57
> +#endif // ENABLE(XSLT)
> +#endif /* XMLViewerHelper_h */

Why a C-style comment?

> Source/WebCore/xml/XSLStyleSheet.h:67
> +    static PassRefPtr<XSLStyleSheet> createFromString(Document* document, String sheetString)

I don't know if this is worth adding a function for. In any case, please use const String&.

> Source/WebCore/xml/XSLStyleSheet.h:69
> +        RefPtr<XSLStyleSheet> sheet = adoptRef(new XSLStyleSheet(document, String(), KURL(), false));

What are the URL arguments used for, and is it safe to have them null?

> Source/WebCore/xml/XSLStyleSheet.h:72
> +        return sheet;

Should be "return sheet.release()" to avoid refcount churn.

> Tools/DumpRenderTree/chromium/TestShell.cpp:230
> +    webView()->mainFrame()->resetOpener();

There is no explanation of this change in ChangeLog. Should it be made in a separate patch?

[Vsevolod Vlasov]

Comment on attachment 82640 [details]
Patch

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

>> LayoutTests/http/tests/xmlviewer/dumpAsText/css-stylesheet.xml:8
>>  \ No newline at end of file
> 
> Please add a newline.

Done.

>> LayoutTests/http/tests/xmlviewer/dumpAsText/resources/css-stylesheet.css:2
>>  \ No newline at end of file
> 
> Please add a newline.

Done.

>> LayoutTests/http/tests/xmlviewer/dumpAsText/resources/frames-helper.xml:6
>>  \ No newline at end of file
> 
> Please add a newline.

Done.

>> LayoutTests/http/tests/xmlviewer/dumpAsText/xmlviewer.xml:40
>> +                And now some long text in root element.
> 
> Do we have any test coverage for non-ASCII content?

Added tests for cyrillic in utf8 and cp1251.

>> Source/WebCore/DerivedSources.make:651
>> +all : XMLViewerXSL.h XMLViewerDownArrowPNG.h XMLViewerRightArrowPNG.h
> 
> I guess I just don't know how we usually do it, but generating source files for resources seems strange. Generally speaking, all images should come from resources for localization (it's fairly unlikely that these particular images need to be localized, but one can never be sure... how about reversing arrow direction for RTL languages? or changing color for cultures that have different connotations for colors?)

Done using CSS canvas. Now if need is to make these localized, it could be done by passing parameters from resources for localization.

>> Source/WebCore/GNUmakefile.am:3630
>> +	Source/WebCore/xml/XMLViewerHelper.h \
> 
> "Helper" doesn't mean anything. Pavel told me that this class is encapsulating logic related to the feature - it really needs a speaking name.

Renamed to XMLTreeViewer. This names seems speaking to me.

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25133
>> +>>>>>>> XML without style should render as syntax-highlighted source.
> 
> Bad merge.

Done.

>> Source/WebCore/dom/Document.cpp:411
>> +    , m_hasElementsInKnownNamespaces(false)
> 
> This member variable is quite misleading. For one thing, it doesn't track element removal. And the concept of "having" an element is not formal.
> 
> This is part of a heuristic for deciding when to display the document as tree, and ideally, it shouldn't be even tracked inside Document itself. Maybe you need to keep it inside Document for performance, as calling into another class wouldn't be great.
> 
> I also have concerns about long term validity of this heuristic. How would we do incremental rendering of XML if there is a check that depends on all elements in the document? Even if we never implement incremental rendering, it would be good to have common tree view behavior with browsers that do.

As discussed, this behavior is in agreement with other browsers.
The member variable is renamed to m_sawElementsInKnownNamespaces. 
It could be moved out from here but should stay in Document for performance.

>> Source/WebCore/dom/ProcessingInstruction.h:94
>> +    bool m_isCSS;
> 
> Now that you are adding this data member, you should update the rest of the code to use it. For example, ProcessingInstruction::setCSSStyleSheet() should ASSERT(m_isCSS).
> 
> Might make sense to do that in a separate patch before this one.

Done in separate patch.

>> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1336
>> +    bool xmlViewerMode = !m_sawError && !m_sawCSS && !m_sawXSLTransform && xmlViewerHelper.hasNoStyleInformation();
> 
> As mentioned above, we should consider the possibility of expressing the heuristic in a way that works with incremental parsing (not necessarily coding it that way).

See above.

>> Source/WebCore/xml/XMLViewer.xsl:19
>> +   - THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> 
> This is a yet different license header.

Done.

>> Source/WebCore/xml/XMLViewerHelper.cpp:17
>> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> 
> Please use a correct license header - "Apple Computer, Inc." and "Apple" are both wrong names to use. Official project license is available at <http://www.webkit.org/coding/bsd-license.html>.

Done.

>> Source/WebCore/xml/XMLViewerHelper.cpp:99
>> +    processor->setParameter("", "xml_has_no_style_message", "This XML file does not have any style information. The document tree is shown.");
> 
> Is "is shown" proper English? Again, you should really ask for a localizable string, so that could be vetted by language experts in each company that ships WebKit.

As discussed, this doesn't make any regression, copied English message from FF for now with localization patch follow up.

>> Source/WebCore/xml/XMLViewerHelper.h:17
>> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> 
> License header again.

Done.

>> Source/WebCore/xml/XMLViewerHelper.h:57
>> +#endif /* XMLViewerHelper_h */
> 
> Why a C-style comment?

Done.

>> Source/WebCore/xml/XSLStyleSheet.h:67
>> +    static PassRefPtr<XSLStyleSheet> createFromString(Document* document, String sheetString)
> 
> I don't know if this is worth adding a function for. In any case, please use const String&.

Done.

>> Source/WebCore/xml/XSLStyleSheet.h:69
>> +        RefPtr<XSLStyleSheet> sheet = adoptRef(new XSLStyleSheet(document, String(), KURL(), false));
> 
> What are the URL arguments used for, and is it safe to have them null?

These urls are used for loading linked resources, checking security origins, caching these resources.
In this case they will not be used and are safe to be empty strings.

>> Source/WebCore/xml/XSLStyleSheet.h:72
>> +        return sheet;
> 
> Should be "return sheet.release()" to avoid refcount churn.

Done.

>> Tools/DumpRenderTree/chromium/TestShell.cpp:230
>> +    webView()->mainFrame()->resetOpener();
> 
> There is no explanation of this change in ChangeLog. Should it be made in a separate patch?

Done in separate patch.

[Vsevolod Vlasov]

Created attachment 83826 [details]
Patch with fixes

[Alexey Proskuryakov]

Comment on attachment 83826 [details]
Patch with fixes

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

Looks good to me.

> LayoutTests/http/tests/xmlviewer/dumpAsText/xmlviewer-charset-utf8.xml:4
> +  <message>Text in the next node should be in Russian</message>
> +  <utf8>ШиÑÐ¾ÐºÐ°Ñ ÑлекÑÑиÑикаÑÐ¸Ñ ÑжнÑÑ Ð³ÑбеÑний даÑÑ Ð¼Ð¾ÑнÑй ÑолÑок подÑÑÐ¼Ñ ÑелÑÑкого ÑозÑйÑÑва.</utf8>

One trick I often use is to have "SUCCESS" spelled with Cyrillic "CCE". That way, even a person who doesn't know Russian will be able to see if the results are correct.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1342
> +            document()->setParsing(false); // Make the doc think it's done, so it will apply xsl sheets.

Please don't abbreviate "document", and spell "XSL stylesheets".

> Source/WebCore/xml/XMLTreeViewer.cpp:104
> +    // FIXME: We should introduce error handling

Is this something worth filing a bug for?

> Source/WebCore/xml/XSLStyleSheet.h:28
> +#include "Document.h"

Is this #include necessary?

> Source/WebCore/xml/XSLStyleSheet.h:73
> +    static PassRefPtr<XSLStyleSheet> createFromString(Document* document, const String& sheetString)
> +    {
> +        RefPtr<XSLStyleSheet> sheet = adoptRef(new XSLStyleSheet(document, String(), KURL(), false));
> +        sheet->parseString(sheetString);
> +
> +        return sheet.release();
> +    }

Please consider giving this function a less generic name, so that it's less likely to be misused later. Maybe createFromStringWithNullURL? Or createForXMLTreeViewer(), matching the existing createForXSLTProcessor()?

[Vsevolod Vlasov]

Created attachment 83838 [details]
Patch with some more fixes

[Vsevolod Vlasov]

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

>> LayoutTests/http/tests/xmlviewer/dumpAsText/xmlviewer-charset-utf8.xml:4
>> +  <utf8>ШиÑÐ¾ÐºÐ°Ñ ÑлекÑÑиÑикаÑÐ¸Ñ ÑжнÑÑ Ð³ÑбеÑний даÑÑ Ð¼Ð¾ÑнÑй ÑолÑок подÑÑÐ¼Ñ ÑелÑÑкого ÑозÑйÑÑва.</utf8>
> 
> One trick I often use is to have "SUCCESS" spelled with Cyrillic "CCE". That way, even a person who doesn't know Russian will be able to see if the results are correct.

Nice trick. Done.

>> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1342
>> +            document()->setParsing(false); // Make the doc think it's done, so it will apply xsl sheets.
> 
> Please don't abbreviate "document", and spell "XSL stylesheets".

Not my code actually, but done :)

>> Source/WebCore/xml/XMLTreeViewer.cpp:104
>> +    // FIXME: We should introduce error handling
> 
> Is this something worth filing a bug for?

https://bugs.webkit.org/show_bug.cgi?id=55233

>> Source/WebCore/xml/XSLStyleSheet.h:28
>> +#include "Document.h"
> 
> Is this #include necessary?

Removed, see below.

>> Source/WebCore/xml/XSLStyleSheet.h:73
>> +    }
> 
> Please consider giving this function a less generic name, so that it's less likely to be misused later. Maybe createFromStringWithNullURL? Or createForXMLTreeViewer(), matching the existing createForXSLTProcessor()?

createForXMLTreeViewer() would be better. Document.h was needed as this method was taking Document* argument, not a Node* one, to decrease a chance of misuse. But since the name is clearer now, I guess Node will be fine.

[Alexey Proskuryakov]

> Document.h was needed as this method was taking Document* argument, not a Node* one, to decrease a chance of misuse. But since the name is clearer now, I guess Node will be fine.

If this method is only supposed to be called with a Document argument, then it's better to have the more specific type. There is no need to include Document.h for that, a forward declaration would suffice.

Generally, we try hard to not include headers that are not absolutely needed to make rebuild times shorter. Using forward declarations is the most common technique.

[Pavel Feldman]

Comment on attachment 83838 [details]
Patch with some more fixes

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

Looks great. Please fix the JavaScript nits and we can land this.

> Source/WebCore/xml/XMLViewer.xsl:268
> +                function onload() {

should be on the next line. Can be fixed while landing.

> Source/WebCore/xml/XMLViewer.xsl:269
> +                  drawArrows();

Four spaces indent. Ditto.

> Source/WebCore/xml/XMLViewer.xsl:301
> +                function getElementsByClassName(classname, node, recursive)  {

Too many spaces before { (that should be on the next line)

> Source/WebCore/xml/XMLViewer.xsl:305
> +                    if (recursive) {

No need for { and } for one-liners.

> Source/WebCore/xml/XMLViewer.xsl:311
> +                        if(re.test(els[i].className)) {

Ditto

> Source/WebCore/xml/XMLViewer.xsl:319
> +                    getElementsByClassName('expanded', button.parentNode.parentNode.parentNode, false)[0].className = 'expanded';

what is button.parentNode.parentNode.parentNode? You should not walk DOM structure like that - it is better to store a link to the node you need into the button object itself.
Please use document.querySelectorAll() instead of this getElementsByClassName

> Source/WebCore/xml/XMLViewer.xsl:324
> +                    getElementsByClassName('collapsed', button.parentNode.parentNode.parentNode, false)[0].className = 'collapsed';

ditto

[Vsevolod Vlasov]

Created attachment 83932 [details]
Patch with javascript fixes.

Fixed these.

[Pavel Feldman]

Comment on attachment 83932 [details]
Patch with javascript fixes.

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

> Source/WebCore/xml/XMLViewer.xsl:241
> +        <span class="button expand-button" onmousedown="return buttonMouseDown();">

return buttonMouseDown does not sound right, what are you trying to achieve? But I think we can address it in a follow up patch.
Setting cq- since is likely to require careful landing.

[Pavel Feldman]

Committed r79799: <http://trac.webkit.org/changeset/79799>

[Pavel Feldman]

Rolled out as r79805 for breaking XML tests on Snow Leopard. See 
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r79799%20(26001)/results.html for more details.

fast/css/xml-stylesheet-pi-not-in-prolog.xml
fast/encoding/utf-16-no-bom.xml
fast/invalid/junk-data.xml
svg/hixie/error/004.xml
svg/hixie/error/005.xml

Sounds like we are not handling XML resources containing errors properly.

Also, I had to manually fix CMakeLists (see 79804) as a follow up.

[Vsevolod Vlasov]

Created attachment 84050 [details]
Patch with failing tests fixes.

XML Viewer tests for QT disabled.

Failing tests fixed.
Some tests were moved to "/dumpAsText" to ensure dumping as text.
Their expected results for QT should be checked.

[Pavel Feldman]

Committed r79861: <http://trac.webkit.org/changeset/79861>

[Eric Seidel (no email)]

Looks like this is failing on Snow Leopard.

[Pavel Feldman]

(In reply to comment #40)
> Looks like this is failing on Snow Leopard.

Yes, I thought I fixed it in http://trac.webkit.org/changeset/79874 & 79879. Give us 12 hours to fix this.

[Eric Seidel (no email)]

You do realize that leaving a broken test in for 12 hours, means that no one can use the commit-queue for those 12 hours. :)

[Pavel Feldman]

(In reply to comment #42)
> You do realize that leaving a broken test in for 12 hours, means that no one can use the commit-queue for those 12 hours. :)

Heh. Should be fixed.

I noticed the new native XML rendering in Chrome version 11.0.696.3 dev. I see a few issues with the implementation:

1. You're using an XSL stylesheet to render the output. This proves problematic in both rendering times on large-ish XML documents and getting declared namespaces to display accurately.

2. The results don't include declared namespaces. See comments about #1.

I maintain the XML Tree extension for Chrome and initially it used an XSL stylesheet to render the output. However, due to long transformation times on large XML documents I had to swap the majority of the rendering work to javascript. This reduced the rendering time of XML document tremendously. 

Another issue I ran into is trying to get declared namespaces to display accurately. I spent many hours trying to get this right with an XSL stylesheet. I probably got about 95% of the way there, but in the end it's just too much of a hassle with XSL. Using javascript (SAX parsing) for the rendering, the XML Tree extension can accurately display declared namespaces where as the native rendering attributed to this bug doesn't display any. I know both IE and Firefox don't display namespaces in their rendering, so you are on par with those, but they also don't have half the functionality of most XML extensions available for Chrome have (so far Safari gets no XML love).

One problem I have with this implementation is that there isn't a way programmatically to not have the native rendering occur. For a Chrome user with one of the XML extensions installed, this means the native rendering is applied and then the extension's rendering is applied to the native renderings output for XML documents without namespaces.

For XML documents with namespaces, because namespaces are not included in the native rendering, the Chrome javascript DOM parser barfs when an extension attempts to render the native rendered XML because it is not valid XML as the prefixes are applied to the elements and attributes, but the namespace URIs are nowhere to be found.  
For instance, if you open an XSL stylesheet in Chrome without an XML rendering extension installed, you'll get the native rendering but no where is the "http://www.w3.org/1999/XSL/Transform" namespace declared anywhere. Great that webkit provides native rendering of XML documents, but not so great it fails badly with namespaces and displays invalid XML documents.

I applaud the work to natively render XML documents in webkit, but I think you are doing a disservice by not displaying well-formed and valid XML. As a Safari user I'd be happy to have any XML rendering, but as a Chrome user this is a real step down from what the three XML rendering extensions provide for functionality.

[Adam Barth]

> I applaud the work to natively render XML documents in webkit, but I think you are doing a disservice by not displaying well-formed and valid XML. As a Safari user I'd be happy to have any XML rendering, but as a Chrome user this is a real step down from what the three XML rendering extensions provide for functionality.

Thanks for your detailed feedback.  Would it make sense to incorporate your extension natively into WebKit as the XML renderer (i.e., as an improvement over the rendering provided by this patch)?

[Alexey Proskuryakov]

It would be best to discuss further improvements in a new bug.

I think incorporating, or a derivation of, the extension's model would make for better end results.

I would definitely be willing to provide any assistance I can. In the short term, the most help I can give is to suggest you check out the source of Chrome's XML Tree extension in the way it parses the DOM via javascript as a guide (tree.render function). 

Due to the extension being a collaboration of multiple developers' work, licensing issues are problematic. You'll have to install the extension to view the source as I don't feel I can fully release the source without the other's permission. Anything I've written, I'd be happy to release, but I can't speak for the others.

Alexey - If a new bug is to be created for this, I'd like to request I be added to the CC list.

[Pavel Feldman]

> 1. You're using an XSL stylesheet to render the output. This proves problematic in both rendering times on large-ish XML documents and getting declared namespaces to display accurately.

Could you point to a test case (public large XML)? I'd like to find out why large XML documents result in long rendering / why XSL is slow.

> 2. The results don't include declared namespaces. See comments about #1.

A test case would also help, ideally in a separate bug claiming that namespaces are not rendered property. A side question: do they work fine in Web Inspector?

> One problem I have with this implementation is that there isn't a way programmatically to not have the native rendering occur. For a Chrome user with one of the XML extensions installed, this means the native rendering is applied and then the extension's rendering is applied to the native renderings output for XML documents without namespaces.

That's a good point, thanks for reporting it. Native rendering will apply only in case there are no namespaces known to WebKit + there is no page opener. As a workaround, extension could reload the page with the opener set and do its rendering.

> 
> I applaud the work to natively render XML documents in webkit, but I think you are doing a disservice by not displaying well-formed and valid XML. As a Safari user I'd be happy to have any XML rendering, but as a Chrome user this is a real step down from what the three XML rendering extensions provide for functionality.

We will make sure we don't regress Chrome experience, please help us with test cases.

(In reply to comment #45)
> 
> Thanks for your detailed feedback.  Would it make sense to incorporate your extension natively into WebKit as the XML renderer (i.e., as an improvement over the rendering provided by this patch)?

I agree that namespaces must be fixed, but we need to be careful wrt bloating WebKit code. There already is a full blown support for XML trees in the Web Inspector. This native XML rendering was more of a convenience / feature parity matter for us, users wanted it badly.

[Adam Barth]

> Due to the extension being a collaboration of multiple developers' work, licensing issues are problematic. You'll have to install the extension to view the source as I don't feel I can fully release the source without the other's permission. Anything I've written, I'd be happy to release, but I can't speak for the others.

If there are licensing issues, it's probably better for us not to look at the source code.  If you'd be willing to file a new bug explaining what you'd like improved about the built-in XML rendering, that's probably the best way forward.

If you'd like to have the option of replacing the built-in XML rendering with an extension, that's more of an issue for the Chromium project than the WebKit project.  Please feel free to file a bug at http://new.crbug.com/.  For that, we'll want to touch base with the extensions team.

[Adam Barth]

Looks like pfeldman filed https://bugs.webkit.org/show_bug.cgi?id=56262

[Pavel Feldman]

(In reply to comment #50)
> Looks like pfeldman filed https://bugs.webkit.org/show_bug.cgi?id=56262

and https://bugs.webkit.org/show_bug.cgi?id=56263 for extensions' opt-out.

Feel free to file a performance-related one for large XML documents (with a test case / pointer).

It appears that the long rendering times issue that I encountered with javascript transformations in previous versions of Chrome have either been fixed altogether or do not occur with the way native rendering is accomplished. That's good news and sorry about the false alarm on this part.