Bug 60721 - Marker test from ietestcenter fails
: Marker test from ietestcenter fails
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Rob Buis
http://samples.msdn.microsoft.com/iet...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-12 13:25 PDT by Rob Buis
Modified: 2011-05-19 07:46 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.07 KB, patch)
2011-05-12 13:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.24 MB, application/zip)
2011-05-12 14:10 PDT, WebKit Review Bot
no flags Details
Patch (23.49 KB, patch)
2011-05-12 14:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (55.38 KB, patch)
2011-05-12 14:56 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (41.27 KB, patch)
2011-05-12 15:13 PDT, Rob Buis
zimmermann: review+
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.24 MB, application/zip)
2011-05-12 15:54 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2011-05-12 13:25:01 PDT
See the test url, this test also is part of SVG1.12nd edition testsuite.
Comment 1 Rob Buis 2011-05-12 13:46:47 PDT
Created attachment 93330 [details]
Patch
Comment 2 Geoffrey Garen 2011-05-12 13:50:33 PDT
Your ChangeLog is missing an explanation of why this change is correct.
Comment 3 Dirk Schulze 2011-05-12 13:57:50 PDT
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?
Comment 4 WebKit Review Bot 2011-05-12 14:10:10 PDT
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
Comment 5 WebKit Review Bot 2011-05-12 14:10:16 PDT
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
Comment 6 Rob Buis 2011-05-12 14:28:03 PDT
Created attachment 93344 [details]
Patch
Comment 7 Dirk Schulze 2011-05-12 14:37:03 PDT
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.
Comment 8 Rob Buis 2011-05-12 14:56:58 PDT
Created attachment 93349 [details]
Patch
Comment 9 Rob Buis 2011-05-12 15:13:55 PDT
Created attachment 93353 [details]
Patch
Comment 10 WebKit Review Bot 2011-05-12 15:54:26 PDT
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
Comment 11 WebKit Review Bot 2011-05-12 15:54:31 PDT
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
Comment 12 Nikolas Zimmermann 2011-05-18 07:26:47 PDT
Comment on attachment 93353 [details]
Patch

Looks great, r=me.
Comment 13 Rob Buis 2011-05-18 14:40:36 PDT
Committed r86791: <http://trac.webkit.org/changeset/86791>
Comment 14 Ryosuke Niwa 2011-05-18 18:55:21 PDT
It seems like we need a Windows rebaseline:
http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28783
Comment 16 Rob Buis 2011-05-19 07:46:59 PDT
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.