[Webkit-unassigned] [Bug 13807] XML without style should render as syntax-highlighted source

Fri Feb 18 12:47:58 PST 2011

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

--- Comment #26 from Alexey Proskuryakov <ap at webkit.org>  2011-02-18 12:47:56 PST ---
(From update of attachment 82640)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.