[webkit-reviews] review denied: [Bug 25072] CSS21 attribute selectors not dynamic for xml : [Attachment 29313] An attempt to fix the bug including test case

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Kai Brüning
<kai at granus.net>'s request for review:
Bug 25072: CSS21 attribute selectors not dynamic for xml
https://bugs.webkit.org/show_bug.cgi?id=25072

Attachment 29313: An attempt to fix the bug including test case
https://bugs.webkit.org/attachment.cgi?id=29313&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Good evening Kai,

a quick glance over your patch shows some style violations and smaller
problems, that I'm going to comment on. NOTE that I did not verify wheter your
approach is the best, an expert in that area like Hyatt or Maciej would need to
review it. I'm just highlighting some misc. problems:

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 42265)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,16 @@
> +2009-04-07  Kai Brüning  <kai at granus.net>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +		
> +	- Fixed bug 25072: CSS21 attribute selectors not dynamic for xml.
> +		  Copied the relevant part of the
WebCore::StyleElement::attributeChanged to WebCore::Element::attributeChanged,
> +		  taking care to not change the behavior when called from
WebCore::StyleElement::attributeChanged.
> +
> +	   Test: fast/css/attribute-selector-dynamic.xml

Your ChangeLog contains tabs. Please remove them. We usually include the bug
url in the ChangeLog, not the number:
https://bugs.webkit.org/show_bug.cgi?id=25072.
Your name is also misspelled :-)


> ===================================================================
> --- WebCore/dom/Element.cpp	(revision 42261)
> +++ WebCore/dom/Element.cpp	(working copy)
> @@ -559,6 +559,13 @@ PassRefPtr<Attribute> Element::createAtt
>  
>  void Element::attributeChanged(Attribute* attr, bool)
>  {
> +    // Fix for bug 25072: CSS21 attribute selectors not dynamic for xml
> +    // Mapped attributes are handled by StyledElement::attributeChanged()
> +    if (   ! (attr->isMappedAttribute() && isStyledElement())
> +	   && ownerDocument()->attached()
> +	   &&
ownerDocument()->styleSelector()->hasSelectorForAttribute(attr->name().localNam
e()))
> +	   setChanged();

You do want to use "document()" instead of "ownerDocument()" here, because it's
guaranteed to be non-NULL
(except for document type nodes, that don't live in the document itself, but
that's unrelated here).
ownerDocument() would be NULL for a Document object itself, so it's use should
be avoided.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 42265)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> +2009-04-07  Kai Brüning  <kai at granus.net>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +		
> +		- test that CSS21 attribute selectors take effect when the
attribute is
> +		  dynamically changed in an xml dom.

Again tabs here. You also want to add a pixel test result here, generated using
"run-webkit-tests --pixel-tests your/test.xhtml".

> Index: LayoutTests/fast/css/attribute-selector-dynamic.xml
> ===================================================================
> --- LayoutTests/fast/css/attribute-selector-dynamic.xml	(revision 0)
> +++ LayoutTests/fast/css/attribute-selector-dynamic.xml	(revision 0)
> @@ -0,0 +1,28 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<book xmlns="http://docbook.org/ns/docbook"
xmlns:xhtml="http://www.w3.org/1999/xhtml" version="5.0"> 
> +	<xhtml:style>
> +		title { background-color: red; display:inline; }
> +		*[test="0"] { background-color: green; }
> +	</xhtml:style>
> +
> +   <title test="1">Should have green background</title>
> +
> +   <xhtml:script>
> +	 
> +	 function change() {
> +	    var element = document.getElementsByTagName('title')[0];
> +	    element.attributes.test.value = 0;
> +		if (window.layoutTestController)
> +		layoutTestController.notifyDone();
> +	 }
> +	 
> +	 window.onload = function () {
> +		if (window.layoutTestController)
> +			layoutTestController.waitUntilDone();
> +	    // Trigger an attribute change after all load processing is done.
Doing the change
> +	    // here immediately does not exhibit the bug.
> +	    setTimeout("change();", 100);
> +	 }
> +
> +   </xhtml:script>
> +</book>
Again tabs here, they need to be removed.
Does a 0ms timer work here, instead of forcing a 100ms wait? Timing dependant
tests should be avoided, if possible.

r- because of the issues above. NOTE again that this needs review from an
expert, I'm not sure this is the best approach.