[webkit-reviews] review denied: [Bug 77549] Alternate xml-stylesheets with no title are loaded, in violation of the CSSOM draft : [Attachment 130198] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Dave Tharp
<dtharp at codeaurora.org>'s request for review:
Bug 77549: Alternate xml-stylesheets with no title are loaded, in violation of
the CSSOM draft
https://bugs.webkit.org/show_bug.cgi?id=77549

Attachment 130198: Patch
https://bugs.webkit.org/attachment.cgi?id=130198&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130198&action=review


> IE shows the same behavior as FF, in other words, IE respects the draft spec.


How did you test IE?

> Source/WebCore/dom/ProcessingInstruction.cpp:139
> +	   // see
http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing-the-xml
-stylesheet-processing-instruction
> +	   // The rules state that if this is an alternate stylesheet with no
title, 
> +	   // then this stylesheet should not be loaded.

No need for this comment. Implementing the spec while also following other
browsers' behavior is the norm, not an exception that needs explanation.

> LayoutTests/ChangeLog:13
> +	   * fast/css/xml-stylesheet-alternate-no-title-expected.txt: Added.
> +	   * fast/css/xml-stylesheet-alternate-no-title.xhtml: Added.

This should be a text-only test. Please start with a template created by
make-new-script-test, and use shouldBe with computed style to check that the
text is green. Also, please check disabled state of the stylesheet object.

> LayoutTests/fast/css/xml-stylesheet-alternate-no-title.xhtml:8
> +	 if (document.styleSheets.length > 1)
> +	   document.styleSheets[1].disabled = false;

Why are you disabling the stylesheet here?