Bug 60721 – Marker test from ietestcenter fails

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

See the test url, this test also is part of SVG1.12nd edition testsuite.

[Rob Buis]

Created attachment 93330 [details]
Patch

[Geoffrey Garen]

Your ChangeLog is missing an explanation of why this change is correct.

[Dirk Schulze]

Comment on attachment 93330 [details]
Patch

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

> Source/WebCore/svg/SVGMarkerElement.cpp:251
> +    return style->display() != NONE || (parentNode() && parentNode()->renderer() && parentNode()->renderer()->isSVGHiddenContainer());

What about visibility?

[WebKit Review Bot]

Comment on attachment 93330 [details]
Patch

Attachment 93330 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8684889

New failing tests:
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg

[WebKit Review Bot]

Created attachment 93336 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 93344 [details]
Patch

[Dirk Schulze]

Comment on attachment 93344 [details]
Patch

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

I'd like to see at least one more test, where you use inherit for display <marker style="display:inherit" ...

r- because of the new test and the changelog.

> Source/WebCore/ChangeLog:9
> +
> +        Allow <marker> to be referenced in case the display:none is set on it
> +        or any of the ancestors.

You're doing more now. Marker gets a Renderer, always. Also not sure what you mean with referenced in the combination with display:none. Please add a more detailed description what you changed, why you did it, Why it fixes the problem. Add a link to the specification text.

[Rob Buis]

Created attachment 93349 [details]
Patch

[Rob Buis]

Created attachment 93353 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93353 [details]
Patch

Attachment 93353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8689847

New failing tests:
svg/W3C-SVG-1.1-SE/types-dom-05-b.svg
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg

[WebKit Review Bot]

Created attachment 93361 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 93353 [details]
Patch

Looks great, r=me.

[Rob Buis]

Committed r86791: <http://trac.webkit.org/changeset/86791>

[Ryosuke Niwa]

It seems like we need a Windows rebaseline:
http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28783

[Ryosuke Niwa]

Windows 7:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r86809%20(13005)/results.html

[Rob Buis]

Hi Ryosuke,

(In reply to comment #14)
> It seems like we need a Windows rebaseline:
> http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28783

Agreed! I want to make clear as well that the new result is an improvement.
Cheers,

Rob.