Bug 15495

Summary: SVGViewSpec DOM bindings aka SVGSVGElement.currentView is unimplemented
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, gustavo, haraken, japhet, krit, ojan, rakuco, vestbo, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on: 15504    
Bug Blocks:    
Attachments:
Description Flags
test "standard" behavior (download to LayoutTests/svg/dom)
none
Several tests for LayoutTests/svg/dom (some are incomplete)
none
incomplete patch
none
Patch
none
Patch v2
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch v3 rwlbuis: review+

Description Eric Seidel (no email) 2007-10-13 22:56:15 PDT
SVGViewSpec DOM bindings aka SVGSVGElement.currentView is unimplemented

Unfortunately it's so under-specified in the spec that it's hard to implement.  I've raised:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=5191
with the SVG working group.  Hopefully more information will come of that.

I'm attaching my partial patch and a couple test cases and moving on to other things.  We'll come back to this someday.
Comment 1 Eric Seidel (no email) 2007-10-13 22:56:57 PDT
Created attachment 16655 [details]
test "standard" behavior (download to LayoutTests/svg/dom)
Comment 2 Eric Seidel (no email) 2007-10-13 22:59:07 PDT
Created attachment 16656 [details]
Several tests for LayoutTests/svg/dom (some are incomplete)
Comment 3 Eric Seidel (no email) 2007-10-13 22:59:49 PDT
Created attachment 16657 [details]
incomplete patch
Comment 4 Eric Seidel (no email) 2007-10-18 00:41:36 PDT
A chunk of this was landed as part of bug 15504 (the fix to the parser to prevent crashing).
Comment 5 Nikolas Zimmermann 2012-05-19 17:21:24 PDT
*** Bug 15503 has been marked as a duplicate of this bug. ***
Comment 6 Nikolas Zimmermann 2012-05-25 03:35:24 PDT
Created attachment 144033 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2012-05-25 03:50:53 PDT
Comment on attachment 144033 [details]
Patch

Attachment 144033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12791497
Comment 8 Nikolas Zimmermann 2012-05-25 04:37:23 PDT
Created attachment 144045 [details]
Patch v2
Comment 9 WebKit Review Bot 2012-05-25 06:25:21 PDT
Comment on attachment 144045 [details]
Patch v2

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

New failing tests:
svg/dom/viewspec-parser.html
Comment 10 WebKit Review Bot 2012-05-25 06:25:27 PDT
Created attachment 144059 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 11 Rob Buis 2012-05-25 07:49:30 PDT
Comment on attachment 144045 [details]
Patch v2

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

It is unfortunate that the patch is so big, but according to Nikolas it is impractical to split it up. Code looks good, can the failing test be fixed for non OS X?

> Source/WebCore/ChangeLog:9
> +        - SVGViewSpec and all of its concents should be read-only. Enforce that and test it.

contents

> Source/WebCore/ChangeLog:10
> +          Add a new enum to SVGPropertyInfo so that each SVGAnimatedProperty knows if its content is suppoed to be read-write or read-only.

supposed

> LayoutTests/svg/dom/resources/viewspec-parser.svg:4
> +</svg>

Is this a good file name?

> LayoutTests/svg/dom/viewspec-parser.html:196
> +</html>

What changed in this file?
Comment 12 Nikolas Zimmermann 2012-05-26 02:45:00 PDT
Created attachment 144200 [details]
Patch v3
Comment 13 Nikolas Zimmermann 2012-05-26 02:49:04 PDT
Fixed all typos. Changed the viewspec-parser.html to avoid the printf() differences, regarding \0 termination - I hope the new version passes chromium as well.

> It is unfortunate that the patch is so big, but according to Nikolas it is impractical to split it up. Code looks good, can the failing test be fixed for non OS X?
Exposing SVGViewSpec requires all the svg/properties changes - but they can't be done separated, as the current form of SVGViewSpec wouldn't compile then :( I hope this is okay.

> > LayoutTests/svg/dom/resources/viewspec-parser.svg:4
> > +</svg>
> 
> Is this a good file name?
Renamed to viewspec-target.svg, there's no parsing involved.
 
> > LayoutTests/svg/dom/viewspec-parser.html:196
> > +</html>
> 
> What changed in this file?
Best to compare with two views, manually, sorry. webkit-patch upload produces this style of patch.
Comment 14 Rob Buis 2012-05-26 14:10:05 PDT
Comment on attachment 144200 [details]
Patch v3

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

Looks good.

> LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:23
>        RenderSVGRect {rect} at (0,0) size 480x360 [stroke={[type=SOLID] [color=#000000]}] [x=1.00] [y=1.00] [width=478.00] [height=358.00]

I wonder, can this test be made a reftest? Would save some rebaselining. I leave it up to you if you want to do that before landing this patch, assuming it is possible at all.
Comment 15 Nikolas Zimmermann 2012-05-29 01:32:54 PDT
(In reply to comment #14)
> (From update of attachment 144200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144200&action=review
> 
> Looks good.
> 
> > LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:23
> >        RenderSVGRect {rect} at (0,0) size 480x360 [stroke={[type=SOLID] [color=#000000]}] [x=1.00] [y=1.00] [width=478.00] [height=358.00]
> 
> I wonder, can this test be made a reftest? Would save some rebaselining. I leave it up to you if you want to do that before landing this patch, assuming it is possible at all.

Great idea, I'll remove all platform specific results of this test and make it a reftest.
The other linking-* tests can be converted easily as well, but I'll leave that for another patch.
Comment 16 Nikolas Zimmermann 2012-05-29 01:42:23 PDT
Committed r118735: <http://trac.webkit.org/changeset/118735>