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

Fri Feb 25 09:36:40 PST 2011

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

--- Comment #27 from Vsevolod Vlasov <vsevik at chromium.org>  2011-02-25 09:36:39 PST ---
(From update of attachment 82640)
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.

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