Bug 27243 – SVG JS bindings "context" pointer needs to move onto binding impls

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVG JS bindings "context" pointer needs to move onto binding impls

Right now many SVG JS bindings have an extra context() parameter which is stored on the binding object.  That puts some SVG bindings up against the cell size limit.  The current plan for bug 27088 is to add a JSGlobaObject* on DOMObject.  That would put these SVG binding classes over the CELL_SIZE limit.

sub IsSVGTypeNeedingContextParameter
{
    my $implClassName = shift;
    if ($implClassName =~ /SVG/ and not $implClassName =~ /Element/) {
        return 1 unless $implClassName =~ /SVGPaint/ or $implClassName =~ /SVGColor/ or $implClassName =~ /SVGDocument/;
    }
    return 0;
}

[Eric Seidel (no email)]

This might also be a good time to convert more of these custom impls into JSSVGPODTypes.

[Nikolas Zimmermann]

SVGAngle/SVGPreserveAspectRatio have been converted, the last two remaining types. Working on a patch to fix this bug.

[Nikolas Zimmermann]

*** Bug 27278 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 45344 [details]
Initial patch

This patch is NOT ready for review yet, I just want to see wheter V8 compiles or not ;-)

[WebKit Review Bot]

Attachment 45344 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/JSSVGPathSegCustom.cpp:23:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Skipping input 'WebCore/bindings/js/DOMObjectWithSVGContext.h': Can't open for reading
Total errors found: 1

[Nikolas Zimmermann]

Created attachment 45345 [details]
Updated patch

Still not r?, add some minor style changes to let EWS go on.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45345 [details] without any errors.

[Nikolas Zimmermann]

Created attachment 45353 [details]
Patch v1

I am not sure how to test the x-frame prototype chain stuff. I guess there are existing tests for HTML, Sam, please enlighten me :-)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45353 [details] without any errors.

[Nikolas Zimmermann]

Okay, I have tried abarths testcases on bug 27088 with and without this patch and it makes no difference (all SVG* objects say 'PASS'). Also the layout tests show no changes.

Now DOMObjectWithSVGContext is gone, and no one is using deprecatedLexicalGlobalObject anymore from SVG ... so I am wondering what I actually fixed here, besides removing the DOMOBjectWithSVGContext hack?

Can anyone come up with a testcase which fails without this bug? (Adam Barth pointed out that Eric might know :-)

[Sam Weinig]

Comment on attachment 45353 [details]
Patch v1

This looks good to me, though at some point I think we should take it a step further and move this stuff completely out of the bindings, and into the core implementation.  As for tests, fast/dom/prototype-inheritance.html and fast/dom/prototype-inheritance-2.html should be good tests to look at to test this, but they are probably not currently being hit, because the SVG objects don't all have interfaces on the window or because it takes more effort to make one of these guys that simply iterating the window object. I would say r=me, but I think you should try and make a test first.

[Eric Seidel (no email)]

bug 20430 is the reason why so many SVG constructor objects are missing.  I really should finish the patch I started for that so long ago...

[Nikolas Zimmermann]

Created attachment 47164 [details]
Updated patch

Updated this patch against ToT.

I am working on further patches in that area, that will touch all JSSVGPOD* files (as I'm going to rewrite the ANIMATED_PROPERTY_* declaration macros for svg/). This patch is blocking my further work, I am unable to create a testcase, and I only did this patch to cleanup code, there is no bug about prototype chain problems, and I don't fully understand what this patch is supposed to fix - for me it's just cleanup :-)

Let's get this landed please, Sam was already okay with the code, just a testcase should be created. No one came up with a testcase, so please let's move forward even without a testcase.

[Eric Seidel (no email)]

Attachment 47164 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/204409

[Sam Weinig]

(In reply to comment #13)
> Created an attachment (id=47164) [details]
> Updated patch
> 
> Updated this patch against ToT.
> 
> I am working on further patches in that area, that will touch all JSSVGPOD*
> files (as I'm going to rewrite the ANIMATED_PROPERTY_* declaration macros for
> svg/). This patch is blocking my further work, I am unable to create a
> testcase, and I only did this patch to cleanup code, there is no bug about
> prototype chain problems, and I don't fully understand what this patch is
> supposed to fix - for me it's just cleanup :-)
> 
> Let's get this landed please, Sam was already okay with the code, just a
> testcase should be created. No one came up with a testcase, so please let's
> move forward even without a testcase.

That is okay with me.  Creating the test case has been hard, but it is not crucial in this edge case.  I will continue to work on it for the time being.

[Nikolas Zimmermann]

(In reply to comment #14)
> Attachment 47164 [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/204409

Hmm, is it really not building? The log is empty, and it worked for me - tried a from scratch build on Leopard.

[Sam Weinig]

Comment on attachment 47164 [details]
Updated patch

r=me assuming you make sure everything builds.

[Nikolas Zimmermann]

(In reply to comment #17)
> (From update of attachment 47164 [details])
> r=me assuming you make sure everything builds.

Thanks a lot! During the Xcode project updates, I forgot to say the "Private" role for JSSVGContextCache.h - now WebKit builds fine.

[Nikolas Zimmermann]

Landed in r53666.